This is an archive of the discontinued LLVM Phabricator instance.

Support for __asan_apply_to_globals in ASAN runtime library
ClosedPublic

Authored by rgov on Feb 2 2016, 10:11 PM.

Details

Summary

These are the changes to the ASAN runtime library that are needed for D16737: Support for dead code stripping on Mach-O platforms.

Diff Detail

Event Timeline

rgov updated this revision to Diff 46745.Feb 2 2016, 10:11 PM
rgov retitled this revision from to Support for __asan_apply_to_globals in ASAN runtime library.
rgov updated this object.
samsonov edited edge metadata.Feb 5 2016, 2:49 PM

Please update ASan ABI version in lib/asan/asan_init_version.h. See the comment about test you might want to add in http://reviews.llvm.org/D16737

rgov updated this revision to Diff 47064.Feb 5 2016, 4:23 PM
rgov edited edge metadata.

Update ABI version per @samsonov.

rgov updated this revision to Diff 47091.Feb 6 2016, 12:03 PM

Add a test case.

rgov updated this revision to Diff 47092.Feb 6 2016, 12:06 PM

Update comment in test case to be more accurate.

glider accepted this revision.Feb 9 2016, 7:55 AM
glider edited edge metadata.

LGTM

This revision is now accepted and ready to land.Feb 9 2016, 7:55 AM
glider added a comment.Feb 9 2016, 7:57 AM

Um, by the way, does the test pass without the LLVM-side change? I suspect no.

rgov added a comment.Feb 9 2016, 9:46 AM

Thanks for reviewing. It does need the LLVM change first. So I will wait to commit until after D16737: Support for dead code stripping on Mach-O platforms is approved.

LGTM

lib/asan/asan_init_version.h
33

This will be v8.

test/asan/TestCases/Darwin/dead-strip.c
22

You can avoid using custom pragmas by smth. like

int main(int argc, char *argv[]) {

alive[argc] = 0;
return 0;

}

samsonov accepted this revision.Feb 9 2016, 4:42 PM
samsonov edited edge metadata.
rgov updated this revision to Diff 47551.Feb 10 2016, 3:22 PM
rgov edited edge metadata.

I discovered that this change was incomplete due to duplicate registration of globals. I've changed this to use a per-library flag that indicates whether globals have been registered already, and ignores duplicate calls.

In the future I'd like to investigate simply eliding the unnecessary ctors, but this fix basically makes them harmless, in the same way that multiple calls to __asan_init do not cause problems.

This also happens to resolve the expected failure of the initialization-bug.cc test case. I do not know if a pathological binary could still execute a ctor that accesses globals before they are registered.

samsonov added inline comments.Feb 10 2016, 3:34 PM
lib/asan/asan_globals.cc
220

Let's just make two functions instead of passing one or the other callback here.
I agree that you can use the same "flag" here: first function will set it from false to true, another one - from true to false.

WDYT?

rgov added inline comments.Feb 10 2016, 3:36 PM
lib/asan/asan_globals.cc
220

I have no preference, other than that it is already written as-is :)

I'm not sure about calling it register_module_globals since module seems to mean translation unit and not shared object—unless I'm mistaken.

samsonov added inline comments.Feb 11 2016, 6:24 PM
lib/asan/asan_globals.cc
220

Yes, "module" has way too many meanings :), "library"?
I'd actually prefer to have two different functions, as this would make the intent cleaner. Sorry for bringing it up that late in the review process :/

rgov updated this revision to Diff 50130.Mar 9 2016, 5:54 AM

Split into two functions per @samsonov's request. Just went with asan_register_module_globals and we can decide to rename it later if we care to.

samsonov added inline comments.Mar 9 2016, 7:54 PM
lib/asan/asan_init_version.h
31–33

Please fix a comment

lib/asan/asan_internal.h
84

It's not really a flag, it's a library/executable marker. You may clarify the way it's used here. E.g. why is it uptr*, not void*?

test/asan/TestCases/Darwin/dead-strip.c
23

So, is it possible to make this test pragma-less? In this way we will also ensure that this invalid load will not be optimized away by the compiler (i.e. run this test with -O3 in addition to -O0).

rgov closed this revision.Mar 28 2016, 2:03 PM

Committed as r264644.