So I am not sure if there is any value in using aliases without indicators.

I think this should be fine, aliases don't need names at all.

Hm, I'd rather separate private aliases and odr indicators because they are separate (although connected) features:

lgtm

Oh, sorry, forgot about this :(. Please rebase and upload new version.
BTW, please don't forget to add 'llvm-commits' to subscribers so your patch will be visible in ML.

Looks good. Will commit this tomorrow if nobody objects.

Looks good, but let's wait for code owners to accept this as well.

Now, the cfg for this example:

void test_for_implicit_scope() {
  for (A a; A b = a;)
    A c;
}

looks like this:

Rebased and updated. Fix the issue with ordering between ScopeEnds and implicit destructors.

In D16403#1001466, @NoQ wrote:

I poked Devin offline and we agreed that the overall approach is good to go. Maxim, thank you for picking it up!

We still don't have scopes for segments of code that don't have any variables in them, so i guess it's not yet in the shape where it is super useful for loops, but it's already useful for finding use of stale stack variables, which was the whole point originally, so i think this should definitely land soon.

In D16403#993512, @m.ostapenko wrote:

In D16403#992452, @NoQ wrote:

Am i understanding correctly that while destroying a you can still use the storage of b safely? Or should a go out of scope before b gets destroyed?

AFAIU, when we destroying a we can still use the storage of b because nothing can be allocated into b's storage between calling destructors for b and a. So, imho the sequence should look like:

[B4.5].~A() (Implicit destructor)
[B5.3].~A() (Implicit destructor)
CFGScopeEnd(b)
CFGScopeEnd(a)

Thought about it a bit more and this still doesn't look correct to me. Like, a.~A() is a function call. It can do a lot of unexpected stuff to registers and stack space before jumping into the function. And given that a and b are in different scopes (a is in loop scope, b is in single iteration scope), storage of b is not protected from such stuff during call to the destructor of a. So there's definitely something fishy here. I guess scope ends and destructors would have to be properly interleaved in all cases of exiting multiple scopes.

In D16403#1011218, @NoQ wrote:

In D16403#1010096, @szepet wrote:

In D16403#992454, @NoQ wrote:

@szepet: so i see that LoopExit goes in the beginning of the cleanup block after the loop, while various ScopeEnds go after the LoopExit. Would loop unrolling be significantly broken if you simply subscribe to ScopeEnd instead of LoopExit and avoid cleaning up the loop state until destructors are processed? I might not be remembering correctly - is LoopExit only used for cleanup, or do we have more work to be done here?

I guess your following comment just answers this:

In D16403#1001466, @NoQ wrote:

We still don't have scopes for segments of code that don't have any variables in them, so i guess it's not yet in the shape where it is super useful for loops, but it's already useful for finding use of stale stack variables, which was the whole point originally, so i think this should definitely land soon.

It could be, however, we would lose cases like:

int i = 0;
int a[32];
for(i = 0;i<32;++i) {a[i] = i;}

Since there is no variable which has the scope of the loop, ScopeEnd would be not enough. Sure, we could remove this case, however, the aim is to extend the loop-patterns for completely unrolling. Another thing that there are the patches which would enhance the covered cases by LoopExit (D39398) and add the LoopEntrance to the CFG(D41150) as well. So, at this point, I feel like it would be a huge step back not to use these elements. (Sorry Maxim that we are discussing this here^^)

Yeah, i mean, like, if we change the scope markers to also appear even when no variables are present in the scope, then it would be possible to replace loop markers with some of the scope markers, right?

Fix scope ends order (as discussed above) and adjust a testcase.

In D16403#992452, @NoQ wrote:

Thank you, this explanation looks very reasonable.

All right, so right after the termination of the loop we have

[B1]
1: ForStmt (LoopExit)
2: [B4.5].~A() (Implicit destructor)
3: [B5.3].~A() (Implicit destructor)
4: CFGScopeEnd(a)
5: CFGScopeEnd(b)

... where [B4.5] is A b = a; and [B5.3] is A a;. Am i understanding correctly that while destroying a you can still use the storage of b safely? Or should a go out of scope before b gets destroyed?

Actually upload the files.

Hi Artem,

Rebased and ping.

Some code cleanup + updated test case.

Hi Devin,

Hi, I've committed this on behalf of Denis. @denis13 Will ping you if some buildbots complain about this change.

Adding maintainers + some comments.

Ping^4

Ping^3

Rebased and ping.

Updated some comments. Could someone take a look please?

Rebased and removed a bunch of stale changes. Also added a check for goto's: if we see GotoStmt and have cfg-scopes == true, make badCFG = true and retry without scopes enabled. This check will be removed once GotoStmt will become supported.

In D16403#808104, @NoQ wrote:

I think the remaining switch-related code seems to be about C++17 switch condition variables, i.e. switch (int x = ...)(?)

Updating the diff. I've dropped SwitchStmt support, let's implement in separately as well as GotoStmt.

Hi Artem, I'm sorry for a long delay (nasty corporate issues).

So, updating the diff. This is still a very experimental version and any feedback would be greatly appreciated.
Current patch should support basic {If, While, For, Compound, Switch}Stmts as well as their interactions with {Break, Continue, Return}Stmts.
GotoStmt and CXXForRangeStmt are not supported at this moment.

unless someone has objections (@bshastry ?) I'll commandeer this revision and post updated patch shortly (it still needs some polishing though).

In D34210#780520, @fjricci wrote:

Currently, the way that we tell users to gate on sanitizer-specific behavior is with __has_feature(foo_sanitizer), as far as I know, it's the only way to do so. LSan provides several API functions for users, ie __lsan_ignore_object. If a user program wants to use these API functions in their program, they need a way to check that LSan is enabled at compilation time (even if LSan doesn't actually modify the compile step). I'm not sure of a better or more consistent way to allow that to happen.

In D33325#773332, @eugenis wrote:

In D33325#773322, @m.ostapenko wrote:

Oh, sorry.

In D33325#773286, @eugenis wrote:

This change is causing problems on Linux. The immediate issue is Tcl, which creates new threads from a pthread_atfork() child handler. The handler is called from libc's fork(), and pthread_create tries to acquire the StackDepot lock before it is released in the fork interceptor.

Won't this deadlock with MSan as well? AFAIK it also locks StackDepot in fork interceptor...

Maybe it does not use StackDepot in pthread_create?

Tcl is broken, of course:

If a fork() call in a multi-threaded process leads to a child fork handler calling any function that is not async-signal-safe, the behavior is undefined.

But in general, even malloc() called from pthread_atfork() will deadlock with this change.
It looks like the right way to do this is call pthread_atfork() as early as possible to setup lock/unlock handlers for the allocator and the stack depot.

Perhaps we can do it from asan_init? From man:
"The order of calls to pthread_atfork() is significant. The parent and child fork handlers shall be called in the order in which they were established by calls to pthread_atfork(). The prepare fork handlers shall be called in the opposite order."

So in this case our unlock routines will be called before others. This still can deadlock when preloading asan to noinstrumented binary though.

Sounds like that's the best we can do. If we are lucky, sanitizer initialization will happen early enough due to ENSURE_ASAN_INIT in one of the interceptors.

In D33784#774238, @eugenis wrote:

A crazy thought - what happens if we add DF_1_INITFIRST to the asan library?
-Wl,-z,initfirst

Please add a testcase. For me, following commands work:
$ cat foo.c

#include <stdlib.h>

In D33325#773286, @eugenis wrote:

This change is causing problems on Linux. The immediate issue is Tcl, which creates new threads from a pthread_atfork() child handler. The handler is called from libc's fork(), and pthread_create tries to acquire the StackDepot lock before it is released in the fork interceptor.

In D33784#770431, @eugenis wrote:

Yeah... that's weird.
Can we simply do ENSURE_ASAN_INITED(); thing in the malloc interceptor?

In D33784#770392, @eugenis wrote:

Are you saying that LD_PRELOAD-ed library constructors are run after some other library constructors, and that other library is not a dependency of the LD_PRELOAD-ed one? I've never seen that. Is that on linux/glibc?

Let me share more background.
The problem occurs in such rare cases when we have one instrumented library and uninstrumented binary with lots of dependencies from other (uninstrumented) shared libs. In this case we can use LD_PRELOAD trick to preload shared ASan runtime to uninstrumented binary, but since rtld actually doesn't specify order of constructors execution, we can end up with overflow in static alloc_memory_for_dlsym pool if some constructor has malloc with some big size and this constructor is called before __asan_init. The right solution would be just build the executable with ASan, but unfortunately sometimes developers can't do this.

Please add a testcase (you can take it from Vedant's patch in PR).

Rebased. Gentle reminder.

Removing TSan part. The attached testcase still hangs with TSan, but since locking allocator is problematic let's leave TSan alone.

Addressing Dmitry's comments.

In D33325#761941, @dvyukov wrote:

I don't completely understand this. allocator_fork_no_hang.cc is broken and deserves deadlocking. Why are we trying to fix this program only under sanitizers rather than detecting and reporting the bug? Sanitizers are meant to be less forgiving than production environment. Why are we trying to conceal the bug?

Updating. Reproduced the deadlock with TSan and LSan on adjusted testcase. Adding Dmitry since the patch now affects TSan code as well.

Updating. Lock allocator on fork for {A, M, T}San and moving testcase to sanitizer_common. I've also managed to reproduce the deadlock with MSan, but still not with TSan.

In D33325#759703, @eugenis wrote:

Good catch. This is a problem for MSan as well, is not it? Do you mind fixing it in both tools?

Updating. When adapting test/msan/fork.cc for ASan I've noticed that deadlock can also happen if the whole allocator is locked when we calling fork. Corresponding backtrace looks like this:

#0  atomic_exchange<__sanitizer::atomic_uint32_t> () at /home/max/src/llvm/projects/compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang.h:68
#1  Lock () at /home/max/src/llvm/projects/compiler-rt/lib/sanitizer_common/sanitizer_linux.cc:541
#2  0x000000000041e9f9 in GenericScopedLock () at /home/max/src/llvm/projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_mutex.h:187
#3  GetFromAllocator () at /home/max/src/llvm/projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_allocator_primary64.h:134
#4  0x000000000041e99b in Refill () at /home/max/src/llvm/projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_allocator_local_cache.h:108
#5  0x000000000041e550 in Allocate () at /home/max/src/llvm/projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_allocator_local_cache.h:50
#6  0x000000000041e443 in Allocate () at /home/max/src/llvm/projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_allocator_combined.h:68
#7  0x000000000041b6cb in Allocate () at /home/max/src/llvm/projects/compiler-rt/lib/asan/asan_allocator.cc:407
#8  0x00000000004b9148 in __interceptor_malloc () at /home/max/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:67
#9  0x00000000004e878d in child () at fork.cc:47
#10 0x00000000004e8968 in test () at fork.cc:67
#11 0x00000000004e8a38 in main () at fork.cc:78

Thus we need to lock allocator as well.

In D33325#758766, @eugenis wrote:

Look at test/msan/fork.cc, it should be possible to replace copy_uninit_thread2 with an allocation loop.

In D33325#758753, @kcc wrote:

is a test possible?

Add guards against old Glibc versions to testcase.

Remove INIT_MCHECK_MPROBE.

Add mcheck_pedantic.

In D32589#739737, @alekseyshl wrote:

What about mcheck_pedantic and mcheck_check_all?

Remove enum.

Thanks! Watching for buildbots...

lgtm

In D29586#723623, @thakis wrote:

This breaks bootstrap builds. I put the error message in the revert commit description: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20170410/444101.html

Add armeb and thumbeb for completeness.
Just curious, does anyone use sanitizers in these targets? I see no public buildbots for armeb and thumbeb. Anyway, not a big deal of course.

Also check for thumb.

In D29586#718621, @eugenis wrote:

Going back to the issue with interceptor_malloc not saving FP when built with GCC. In clang a function using builtin_frame_address(0) automatically gets a proper frame pointer. As far as I can see, arm-gcc in the android ndk (version 4.9) behaves the same way on a simple test case. Is that some new optimization where a frame address is computed on the fly?

Updating according to Aleksey's comments.

Turned out I was wrong again. Looking to verbose log more carefully, one can see that segfault happens when GetRegistersAndSP fails with errno 3 (ESRCH):

==13657==Attached to thread 14860.
==13657==Attached to thread 14868.
==13657==Attached to thread 13349.
==13657==Attached to thread 13357.
==13657==Could not get registers from thread 13349 (errno 3).
==13657==Unable to get registers from thread 13349.
Tracer caught signal 11: addr=0x7ff4b259b000 pc=0x4239d0 sp=0x7ff4b1d99d90
==13657==Process memory map follows:
...
        0x7ff4b1d9b000-0x7ff4b259b000    0x000000000003 (rw)
        0x7ff4b259b000-0x7ff4b259c000    0x000000000000
...
==13657==End of process memory map.
==13657==Detached from thread 14860.
==13657==Detached from thread 14868.
==13657==Could not detach from thread 13349 (errno 3).
==13657==Detached from thread 13357.
==14860==LeakSanitizer has encountered a fatal error.
==14860==HINT: For debugging, try setting environment variable LSAN_OPTIONS=verbosity=1:log_threads=1
==14860==HINT: LeakSanitizer does not work under ptrace (strace, gdb, etc)

Although LSan successfully attached to thread 13349, it seems that this thread was killed by concurrent SIGKILL signal. Thus stack boundaries extracted by GetRegistersAndSP are already invalid and we access "bad" memory (guard page in this case).
According to ptrace manual, when user tries to get information from ptrace stopped thread he should always be ready to handle ESRCH error:

Rebased. Ping.

Changed flag name.

Looks good, thank you.

In D31430#712412, @fjricci wrote:

In D31430#712410, @m.ostapenko wrote:

In D31430#712387, @fjricci wrote:

Well, the issue is that currently, we aren't respecting the default flags as set in sanitizer_flags.inc when we run on asan. asan_flags.cc overrides the defaults and replaces them with CAN_SANITIZE_LEAKS. This means that leak checking will always run on asan builds, even when the default is set to disabled. This isn't causing issues for linux-i386, as most of the tests pass even though it's technically supposed to be disabled. However, for Darwin, the tests will all fail, so we need to ensure that the default values are respected.

So why not make LSan on Darwin pass these tests instead of disabling them on Linux?

Because it hasn't been fully implemented yet, and we want build-only coverage to ensure that nobody breaks the build in the meantime.

When the defaults are respected, asan will not have leak detection by default on linux-i386, which means that asan tests requiring leak-detection will fail.

Why would we want to disable LSan on x86 Linux by default? It would lead to inconsistency with other architectures default value that may confuse users.

All x86 builds for lsan are currently disabled by default, due to the very high false-negative rate (I believe that patch blames to you actually).

In D31430#712387, @fjricci wrote:

Well, the issue is that currently, we aren't respecting the default flags as set in sanitizer_flags.inc when we run on asan. asan_flags.cc overrides the defaults and replaces them with CAN_SANITIZE_LEAKS. This means that leak checking will always run on asan builds, even when the default is set to disabled. This isn't causing issues for linux-i386, as most of the tests pass even though it's technically supposed to be disabled. However, for Darwin, the tests will all fail, so we need to ensure that the default values are respected.

Could you elaborate, what kind of issue are you trying to solve? LSan in ASan seems to work fine on x86 Linux, it looks like a wrong idea to disable tests there.

In D30504#711277, @thakis wrote:

We have a test in chromium that loads a small so file and calls some of the functions therein. There are no allocations in the so file. The test used to run fine under tsan, now it fails with the error message you added. I'll disable the test under tsan, but maybe others out there where also calling non-allocating so's without problems before this change.

Rebased. Gentle reminder.

In D31300#709284, @kubamracek wrote:

Shouldn't this be "UNSUPPORTED" instead of "XFAIL"?

In D30818#707959, @eugenis wrote:

In D30818#706590, @m.ostapenko wrote:

After more debugging it seems that the issue is even more complicated.

Tracer caught signal 11: addr=0x7fb108da4000 pc=0x423990 sp=0x7fb135ffed90
...
        0x7fb1085a4000-**0x7fb108da4000**	 rw (0x000000000003)

This address does not look accessible to me. Well, that depends on the next mapping.

After more debugging it seems that the issue is even more complicated.

Tracer caught signal 11: addr=0x7fb108da4000 pc=0x423990 sp=0x7fb135ffed90
==9109==Process memory map follows:
	0x000000400000-0x000000441000	/tmp/a.out 0x000000000005
	0x000000641000-0x000000642000	/tmp/a.out 0x000000000001
	0x000000642000-0x000000645000	/tmp/a.out 0x000000000003
...
        0x7fb1085a4000-**0x7fb108da4000**	 rw (0x000000000003)

The faulty address looks fine (addressable and accessible), but the problem seems to be pretty similar to that we saw in fast unwinder some time ago (fixed by Evgeniy with https://reviews.llvm.org/rL219683).
Possible explanation (that we considered when debugging segfault in unwinder) looks like this:

1. Kernel maps stacks from higher addresses to lower (MAP_GROWSDOWN flag in mmap syscall).
2. Kernel maps stacks non-atomically (i.e not all ulilmit amount of stack memory become addressable simultaneously, lower pages become available a little bit later than higher).
3. If we make access to *stack_top (== stack_bot - ulimit) before kernel actually mapped all ulimit range, we'll have segfault.

Ping.

Gentle ping. Is there any chance that LSan for ARM will be accepted?