Issue 8666045: Switch InternalAlloc to use custom allocator instead of libc malloc

Issue 8666045: Switch InternalAlloc to use custom allocator instead of libc malloc (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
11 years ago by samsonov

Modified:
10 years, 10 months ago

Reviewers:
dvyukov

CC:
kcc1

Base URL:
https://llvm.org/svn/llvm-project/compiler-rt/trunk/

Visibility:
Public.

More Reviews

Patch Set 1 : z #

Total comments: 6

Created: 11 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+142 lines, -25 lines)			Patch
M	lib/sanitizer_common/sanitizer_allocator.cc	View	1 chunk	+72 lines, -18 lines	2 comments	Download
A	lib/sanitizer_common/sanitizer_allocator_internal.h	View	1 chunk	+54 lines, -0 lines	4 comments	Download
M	lib/sanitizer_common/sanitizer_common.h	View	1 chunk	+0 lines, -4 lines	0 comments	Download
M	lib/sanitizer_common/tests/sanitizer_allocator_test.cc	View	2 chunks	+5 lines, -0 lines	0 comments	Download
M	lib/tsan/rtl/tsan_mman.cc	View	4 chunks	+6 lines, -2 lines	0 comments	Download
M	lib/tsan/rtl/tsan_platform_linux.cc	View	2 chunks	+3 lines, -1 line	0 comments	Download
M	lib/tsan/rtl/tsan_rtl.h	View	2 chunks	+2 lines, -0 lines	0 comments	Download

Messages

Total messages: 4

Expand All Messages | Collapse All Messages

dvyukov

Much better now https://codereview.appspot.com/8666045/diff/2001/lib/sanitizer_common/sanitizer_allocator.cc File lib/sanitizer_common/sanitizer_allocator.cc (right): https://codereview.appspot.com/8666045/diff/2001/lib/sanitizer_common/sanitizer_allocator.cc#newcode59 lib/sanitizer_common/sanitizer_allocator.cc:59: if (atomic_compare_exchange_strong(&internal_allocator_initialized, this must be either ...

11 years ago (2013-04-30 06:55:57 UTC) #1

samsonov

https://codereview.appspot.com/8666045/diff/2001/lib/sanitizer_common/sanitizer_allocator.cc File lib/sanitizer_common/sanitizer_allocator.cc (right): https://codereview.appspot.com/8666045/diff/2001/lib/sanitizer_common/sanitizer_allocator.cc#newcode59 lib/sanitizer_common/sanitizer_allocator.cc:59: if (atomic_compare_exchange_strong(&internal_allocator_initialized, On 2013/04/30 06:55:57, dvyukov wrote: > this ...

10 years, 12 months ago (2013-05-06 07:08:45 UTC) #2

dvyukov

On 2013/05/06 07:08:45, samsonov wrote: > https://codereview.appspot.com/8666045/diff/2001/lib/sanitizer_common/sanitizer_allocator.cc > File lib/sanitizer_common/sanitizer_allocator.cc (right): > > https://codereview.appspot.com/8666045/diff/2001/lib/sanitizer_common/sanitizer_allocator.cc#newcode59 > ...

10 years, 12 months ago (2013-05-06 15:18:18 UTC) #3

On 2013/05/06 07:08:45, samsonov wrote:
>
https://codereview.appspot.com/8666045/diff/2001/lib/sanitizer_common/sanitiz...
> File lib/sanitizer_common/sanitizer_allocator.cc (right):
> 
>
https://codereview.appspot.com/8666045/diff/2001/lib/sanitizer_common/sanitiz...
> lib/sanitizer_common/sanitizer_allocator.cc:59: if
> (atomic_compare_exchange_strong(&internal_allocator_initialized,
> On 2013/04/30 06:55:57, dvyukov wrote:
> > this must be either double-checked locking or better explicitly initialize
it
> in
> > tool's Init(). hopefully it does not have any dependencies.
> > have you tried explicit init?
> Do you mean the following?
> 
> if (!internal_allocator_initialized) {
>   if (atomic_compare_exchane...()) {
>     internal_allocator_instance->Init();
>   }
> }

Yes, something like that, but using atomic_load(memory_order_acquire) for the
first check. And you can use a mutex to wait for initialization:

if (atomic_load(&inited, memory_order_acquire) == 0) {
  Lock l(&mtx);
  if (inited == 0) {
    Init();
    atomic_store(&inited, 1, memory_order_release);
  }
}

> In case of explicit Init() we must make sure that we call it early enough so
> that
> internal allocator is not used before it. This is doable, but adds an opaque
> dependency.


Yes, but as the result we will understand how our tools work.
What do you mean by the "opaque dependency"?


> Also, UBSan doesn't have explicit __ubsan_init(), and it uses code
> from sanitizer_common.

It's not a good idea to spread lazy initialization over sanitizer common because
of that.
It's better to add ubsan_init(). You can use the lazy initialization there for
now.



>
https://codereview.appspot.com/8666045/diff/2001/lib/sanitizer_common/sanitiz...
> File lib/sanitizer_common/sanitizer_allocator_internal.h (right):
> 
>
https://codereview.appspot.com/8666045/diff/2001/lib/sanitizer_common/sanitiz...
> lib/sanitizer_common/sanitizer_allocator_internal.h:35: static const uptr
> kInternalAllocatorSpace = 0x690000000000ULL;
> On 2013/04/30 06:55:57, dvyukov wrote:
> > is it used in asan?
> > this won't work on windows
> 
> ASan doesn't work on 64-bit Windows anyway.

please close this review

Expand All Messages | Collapse All Messages