Page MenuHomePhabricator

[asan] Avoid putting globals in a comdat section.
ClosedPublic

Authored by pgousseau on Tue, Feb 2, 2:52 AM.

Details

Summary

Putting globals in a comdat for dead-stripping changes the semantic and can potentially cause false negative odr violations at link time.
If odr indicators are used, we keep the comdat sections, as link time odr violations will be dectected for the odr indicator symbols.

This fixes PR 47925

Diff Detail

Event Timeline

pgousseau created this revision.Tue, Feb 2, 2:52 AM
pgousseau requested review of this revision.Tue, Feb 2, 2:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Feb 2, 2:52 AM
pgousseau updated this revision to Diff 321040.Wed, Feb 3, 2:45 AM

fix lint issue

I guess odr-indicator=1 is for reliable ODR violation detection and odr-indicator=0 of smaller binary size.

I guess odr-indicator=1 is for reliable ODR violation detection and odr-indicator=0 of smaller binary size.

Thank you for the comment, yes, this change can potentially break the assumption that odr-indicator=0 reduces binary size, as it will prevent certain linker (eg bfd 2.30), to dead strip unused asan globals’ metadata. Do I understand your point correctly?

But I think correct semantic should take priority over binary size optimizations, please let me know your thoughts on this.

I guess odr-indicator=1 is for reliable ODR violation detection and odr-indicator=0 of smaller binary size.

Thank you for the comment, yes, this change can potentially break the assumption that odr-indicator=0 reduces binary size, as it will prevent certain linker (eg bfd 2.30), to dead strip unused asan globals’ metadata. Do I understand your point correctly?

But I think correct semantic should take priority over binary size optimizations, please let me know your thoughts on this.

Sanitizers don't mind false-negatives if it can noticeably help with some performance metrics. But I can't reproduce expected size regression.

vitalybuka added inline comments.Fri, Feb 19, 2:11 PM
llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
2093

do we need the same for others?

I guess odr-indicator=1 is for reliable ODR violation detection and odr-indicator=0 of smaller binary size.

Thank you for the comment, yes, this change can potentially break the assumption that odr-indicator=0 reduces binary size, as it will prevent certain linker (eg bfd 2.30), to dead strip unused asan globals’ metadata. Do I understand your point correctly?

But I think correct semantic should take priority over binary size optimizations, please let me know your thoughts on this.

Sanitizers don't mind false-negatives if it can noticeably help with some performance metrics. But I can't reproduce expected size regression.

Using the SingleSource test suite, I found 15 tests out of 281 had a size regression. The size increase was at most 0.2%, is this an acceptable regression?
Using gnu ld and clang options below:

-O3 -fomit-frame-pointer -DNDEBUG -fsanitize=address -fno-sanitize=leak -fdata-sections -fno-common -fsanitize-address-globals-dead-stripping
Tests: 281
Metric: size

Program                                        results.ld.old results.ld.new diff
 test-suite...nchmarks/Stanford/FloatMM.test    4249440        4256232        0.2%
 test-suite...enchmarks/Stanford/Queens.test    4249240        4252216        0.1%
 test-suite...e/Benchmarks/Misc/flops-4.test    4248192        4251080        0.1%
 test-suite.../Benchmarks/Stanford/Perm.test    4245368        4248152        0.1%
 test-suite...enchmarks/Stanford/RealMM.test    4249440        4252136        0.1%
 test-suite...Benchmarks/Stanford/IntMM.test    4253536        4256232        0.1%
 test-suite...hmarks/Stanford/Quicksort.test    4245448        4248128        0.1%
 test-suite...marks/Stanford/Bubblesort.test    4245504        4248096        0.1%
 test-suite...enchmarks/Stanford/Towers.test    4250184        4252776        0.1%
 test-suite...chmarks/Stanford/Treesort.test    4245600        4248184        0.1%
 test-suite...Benchmarks/Stanford/Oscar.test    4253864        4256392        0.1%
 test-suite...enchmarks/Stanford/Puzzle.test    4254240        4256560        0.1%
 test-suite...rce/Benchmarks/Misc/flops.test    4259048        4260688        0.0%
 test-suite...arks/Misc-C++/oopack_v1p8.test    4326088        4326504        0.0%
 test-suite...ms_struct-bitfield-init-1.test    4241904        4241992        0.0%
llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
2093

In the COFF case, I found that setting COMDAT for global metadata does not hide ODR violations at link time, so I did not touch the COFF case.
I also checked that COMDAT was necessary for the MSVC linker to perform dead stripping.

Thanks for benchmarks.
LGTM
@eugenis ?

eugenis accepted this revision.Tue, Feb 23, 11:26 AM

I think this is acceptable, LGTM.

This revision is now accepted and ready to land.Tue, Feb 23, 11:26 AM
pgousseau closed this revision.Wed, Feb 24, 4:16 AM

Thank you vitalybuka eugenis for reviewing! I have pushed the change at https://reviews.llvm.org/rG27830bc2b1b8
Sorry I forgot to add the reviewers and phabricator url to the commit message -_-