Hey sorry, it’s been several years since I’ve done anything LLVM-related, I don’t think I’m still the right reviewer for this code.

Ah okay, that makes sense

Interesting - I had tried to add has_feature for lsan to clang in the past and couldn’t get the change accepted. Nice to see it works now :)

Didn’t realize I still had open revisions, haven’t worked on lldb in quite some time

I think that should be fine?

lgtm - thanks!

I'm not going to get to this

+static

Don't use getenv when using the preinit array

No, we need it to support configuring the runtime using setenv for the reasons described in the review of D39827.

incorporate D39827

Great, thanks, I'll pull stuff in from that patch.

Remove extra whitespace

Will do.

lgtm

SymbolVendor::FindFunctions will lazily parse functions from the debug info and populate things inside the module, so the lock is required.

I think a better option would be to remove that lock and if it is needed then lock it just for the calls where it necessary. The fact that SymbolVendor locks a mutex inside a Module feels like a pretty bad layering violation for me what can cause many other deadlocks so it would be nice to fix that instead of hacking it around here.

It's definitely possible to re-design the lock holding in such a way that we can keep this locked, but I don't want to go through all the work to do that if there isn't any added value to doing so.

Actually I don't think even that is racy, because we just get a pointer to the const char *, which is immutable anyway.

I guess the question is whether we expect that someone will do something like change the module's filepath while we're printing a log message with that filepath in it.

I think it would be preferable to define this as a separate constant (perhaps kSanitizerVmMemoryOsAllocOnce, like @kubamracek suggested), which is set to VM_MEMORY_OS_ALLOC_ONCE if defined, and some other value (that 73 perhaps?) otherwise. Makes it clear that the constant used by the code isn't necessarily the same as the one set by the system.

Abandoning in favor of D41706

Verified that this fixes my DESTDIR build of standalone compiler-rt.

That patch fixes our build for me.

Yeah, if I try to use an empty CMAKE_INSTALL_PATH for compiler-rt, I end up with:

ping (particularly looking for input from @beanz)

Thanks!

Seems fine to me, and since nobody else has had any issue with it, I'll accept.

Lgtm

An alternative could be to just disable the build of LSan on OS X < 10.9 (or even 10.11)

Because macOS changes quite a bit from version to version, we (or at least I) don't have strong plans to support LSan outside of 10.11+. Not sure what darwin used to store singleton kernel allocations before this page was introduced, but I assume that just not handling those allocations will lead to a ton of false positives. I'm inclined to let this fail on old versions unless there's someone willing to do the work to make sure they're properly supported. cc @kubamracek

lgtm

Awesome, good to know. Thanks!

On platforms where BOOL == signed char, is it actually undefined behavior (or is it just bad programming practice) to store a value other than 0 or 1 in your BOOL? I can't find any language specs suggesting that it is, and given that it's just a typedef for a signed char, I don't see why it would be.

(for posterity, the build is not failing anymore with this patch committed)

Failing build: http://green.lab.llvm.org/green/job/clang-stage2-cmake-RgSan/5341/

@qcolombet - Can you keep an eye on the internal apple builders and let me know if this doesn't fix them? I'll be out of the town the rest of the week though, so I probably won't be able to do much until Monday.

Improve warning messages

Used here: https://reviews.llvm.org/D38703

Clang change here: reviews.llvm.org/D38741

Add warnings

I'm going to be on vacation starting tomorrow (oct 11), and this seems like a pretty unoffensive change to me, so if I don't hear anything positive or negative by the end of the day, I'll commit this to fix the green bootstrapped builders.

I'll upload the clang host_cxx patch shortly, but this shouldn't be blocked on that (since it handles the undefined case).

Address comments

Fairly certain that if this is 10.11-specific, it's either a leak in libxar, or a bug in LeakSanitizer. It doesn't appear to be a leak in objdump itself (particularly when looking at the stack, which has a dispatch_once call in it)

Fix a couple python issues

Fixing a couple issues.

I got this failure to reproduce locally on my macOS 10.11 VM, but it's probably still worth disabling the leak checking on this test to fix the buildbots until I can diagnose the issue.

Oh interesting. I tried using PWD=/, but I didn't try actually cd-ing into that directory. I'll try that - thanks!

Looks good to me