[PATCH] D95859: [asan] Avoid putting globals in a comdat section.

[pierre gousseau via Phabricator via llvm-commits]

pgousseau added a comment.

In D95859#2575618 <https://reviews.llvm.org/D95859#2575618>, @vitalybuka wrote:

> In D95859#2568230 <https://reviews.llvm.org/D95859#2568230>, @pgousseau wrote:
>
>> In D95859#2568106 <https://reviews.llvm.org/D95859#2568106>, @vitalybuka wrote:
>>
>>> 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%



================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:2093
 
     SetComdatForGlobalMetadata(G, Metadata, "");
   }
----------------
vitalybuka wrote:
> do we need the same for others?
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.



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95859/new/

https://reviews.llvm.org/D95859