PR for llvm/llvm-project#67677 #714
Conversation
…dule_ctor (#67745)
On ELF platforms, when there is no global variable and the unique module ID is
non-empty, COMDAT asan.module_ctor is created with no
`__asan_register_elf_globals` calls. If this COMDAT is the prevailing copy
selected by the linker, the linkage unit will have no
`__asan_register_elf_globals` call: the redzone will not be poisoned and ODR
violation checker will not work (#67677).
This behavior is benign for -fno-sanitize-address-globals-dead-stripping because
asan.module_ctor functions that call `__asan_register_globals`
(`InstrumentGlobalsWithMetadataArray`) do not use COMDAT.
To fix #67677:
* Use COMDAT for -fsanitize-address-globals-dead-stripping on ELF platforms.
* Call `__asan_register_elf_globals` even if there is no global variable.
* If the unique module ID is empty, don't call SetComdatForGlobalMetadata:
placing `@.str` in a COMDAT would incorrectly discard internal COMDAT `@.str`
in other compile units.
Alternatively, when there is no global variable, asan.module_ctor is not COMDAT
and does not call `__asan_register_elf_globals`. However, the asan.module_ctor
function cannot be eliminated by the linker.
Tested the following script. Only ELF -fsanitize-address-globals-dead-stripping has changed behaviors.
```
echo > a.cc # no global variable, empty uniqueModuleId
echo 'void f() {}' > b.cc # with global variable, with uniqueModuleId
echo 'int g;' > c.cc # with global variable
for t in x86_64-linux-gnu arm64-apple-macosx x86_64-windows-msvc; do
for gc in -f{,no-}sanitize-address-globals-dead-stripping; do
for f in a.cc b.cc c.cc; do
echo /tmp/Rel/bin/clang -S --target=$t -fsanitize=address $gc $f -o -
/tmp/Rel/bin/clang -S --target=$t -fsanitize=address $gc $f -o - | sed -n '/asan.module_ctor/,/ret/p'
done
done
done
```
(cherry picked from commit 16eed8c906875e748c3cb610f3dc4b875f3882aa)
llvmbot
force-pushed
the
llvm-issue67677
branch
from
067353e to
2f19183
Compare
|
Not sure who can review the backport here @MaskRay? @vitalybuka maybe? |
|
The patch was reverted in 6420d3301cd4f0793adcf11f59e8398db73737d8 |
|
llvm/llvm-project#67677 (comment) mentions a reland of the patch. |
Which was also reverted 6420d3301cd4f0793adcf11f59e8398db73737d8 |
|
What's the state here? Should it not be backported? |
ea1ae115bd835d553d4be85e77cde96092e59348 should be cherry-picked. It's identical to the previous commit. The revert 6420d3301cd4f0793adcf11f59e8398db73737d8 is incorrect for purely internal issues (which I've addressed internally). |
|
Just to be 100% clear: I should cherry-pick |
Yes. Timeline:
There are no dependent patches, (@MaskRay is the authority here; I'm just trying to add clarity in time for this to land with 17.0.3.) |
Agree with the analysis! Yes, it will be very nice to merge ea1ae115bd835d553d4be85e77cde96092e59348 into release/17.x, otherwise many (I have described the garbage collection complexity at https://maskray.me/blog/2023-10-15-address-sanitizer-global-variable-instrumentation#garbage-collection) |
|
cherry-picked and closed. |
resolves llvm/llvm-project#67677