This is an archive of the discontinued LLVM Phabricator instance.

[asan] Put ctor/dtor in comdat.
ClosedPublic

Authored by eugenis on Mar 24 2017, 3:31 PM.

Details

Reviewers
kcc
pcc
rnk
Summary

When possible, put ASan ctor/dtor in comdat.

The only reason not to is global registration, which can be
TU-specific. This is not the case when there are no instrumented
globals. This is also limited to ELF targets, because MachO does
not have comdat, and COFF linkers may GC comdat constructors.

The benefit of this is a lot less __asan_init() calls: one per DSO
instead of one per TU. It's also necessary for the upcoming
gc-sections-for-globals change on Linux, where multiple references to
section start symbols trigger quadratic behaviour in gold linker.

Diff Detail

Repository
rL LLVM

Event Timeline

eugenis created this revision.Mar 24 2017, 3:31 PM
pcc edited edge metadata.Mar 24 2017, 3:39 PM

If you put the constructor in a comdat, will it be preserved on Windows? I believe that the COFF equivalent of .init_array is not treated as a GC root by the linker.

eugenis updated this revision to Diff 93022.Mar 24 2017, 4:25 PM
eugenis edited the summary of this revision. (Show Details)

disabled on win32

rnk edited edge metadata.Mar 24 2017, 4:36 PM
In D31358#710413, @pcc wrote:

If you put the constructor in a comdat, will it be preserved on Windows? I believe that the COFF equivalent of .init_array is not treated as a GC root by the linker.

We actually already require that the user link some small amount of code into every DLL with asan instrumented code, so in theory we could do the initialization once in that code, and not emit this constructor everywhere on Windows. I don't have time to push that through right now, but that would be ideal.

pcc accepted this revision.Mar 24 2017, 5:41 PM

LGTM with nits

lib/Transforms/Instrumentation/AddressSanitizer.cpp
1440–1441

Could be a little simpler to dyn_cast and return if null.

1783

You could just set *CtorComdat to false here instead of the above.

This revision is now accepted and ready to land.Mar 24 2017, 5:41 PM
eugenis marked an inline comment as done.Mar 24 2017, 5:59 PM
eugenis added inline comments.
lib/Transforms/Instrumentation/AddressSanitizer.cpp
1783

I don't want this function to rely on the previous value of *CtorComdat.

eugenis updated this revision to Diff 93026.Mar 24 2017, 6:00 PM
pcc added a comment.Mar 24 2017, 6:11 PM

Still LGTM

eugenis closed this revision.Mar 24 2017, 6:14 PM

r298756
Thanks!