[sancov] Use comdats when one already exists #131929
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This code avoids adding comdat groups to interposable linkage types (weak, linkonce (non-ODR)) to avoid changing semantics, since comdat elimination happens before weak/strong prevailaing symbol resolution. However, if the function is already in a comdat, we can add to the group without changing the semantics of the linked program. Fixes an issue uncovered in PR llvm#126240
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Reid Kleckner (rnk) ChangesThis code avoids adding comdat groups to interposable linkage types (weak, linkonce (non-ODR)) to avoid changing semantics, since comdat elimination happens before weak/strong prevailaing symbol resolution. However, if the function is already in a comdat, we can add to the group without changing the semantics of the linked program. Fixes an issue uncovered in PR #126240 Full diff: https://github.com/llvm/llvm-project/pull/131929.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp b/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
index 398f83e3b3462..c5100ded8696c 100644
--- a/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
+++ b/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
@@ -745,7 +745,7 @@ GlobalVariable *ModuleSanitizerCoverage::CreateFunctionLocalArrayInSection(
Constant::getNullValue(ArrayTy), "__sancov_gen_");
if (TargetTriple.supportsCOMDAT() &&
- (TargetTriple.isOSBinFormatELF() || !F.isInterposable()))
+ (F.hasComdat() || TargetTriple.isOSBinFormatELF() || !F.isInterposable()))
if (auto Comdat = getOrCreateFunctionComdat(F, TargetTriple))
Array->setComdat(Comdat);
Array->setSection(getSectionName(Section));
diff --git a/llvm/test/Instrumentation/SanitizerCoverage/interposable-symbol.ll b/llvm/test/Instrumentation/SanitizerCoverage/interposable-symbol.ll
index 29a7562ec8748..e8b23096ff33a 100644
--- a/llvm/test/Instrumentation/SanitizerCoverage/interposable-symbol.ll
+++ b/llvm/test/Instrumentation/SanitizerCoverage/interposable-symbol.ll
@@ -2,6 +2,8 @@
; RUN: opt < %s -passes='module(sancov-module)' -sanitizer-coverage-level=3 -sanitizer-coverage-trace-pc-guard -mtriple x86_64-linux-gnu -S | FileCheck %s --check-prefixes=CHECK,ELF
; RUN: opt < %s -passes='module(sancov-module)' -sanitizer-coverage-level=3 -sanitizer-coverage-trace-pc-guard -mtriple x86_64-windows-msvc -S | FileCheck %s --check-prefixes=CHECK,COFF
+$WeakComdat = comdat any
+
define void @Vanilla() {
entry:
ret void
@@ -29,14 +31,22 @@ entry:
ret void
}
+define weak void @WeakComdat() comdat {
+entry:
+ ret void
+}
+
+
; CHECK: $Vanilla = comdat nodeduplicate
; ELF: $LinkOnceOdr = comdat nodeduplicate
; COFF: $LinkOnceOdr = comdat any
+; CHECK: $WeakComdat = comdat any
; CHECK: @__sancov_gen_ = private global [1 x i32] zeroinitializer, section {{.*}}, comdat($Vanilla), align 4{{$}}
; CHECK-NEXT: @__sancov_gen_.1 = private global [1 x i32] zeroinitializer, section {{.*}}, align 4{{$}}
; CHECK-NEXT: @__sancov_gen_.2 = private global [1 x i32] zeroinitializer, section {{.*}}, align 4{{$}}
; CHECK-NEXT: @__sancov_gen_.3 = private global [1 x i32] zeroinitializer, section {{.*}}, comdat($LinkOnceOdr), align 4{{$}}
; CHECK-NEXT: @__sancov_gen_.4 = private global [1 x i32] zeroinitializer, section {{.*}}, comdat($WeakOdr), align 4{{$}}
+; CHECK-NEXT: @__sancov_gen_.5 = private global [1 x i32] zeroinitializer, section {{.*}}, comdat($WeakComdat), align 4{{$}}
; CHECK: define void @Vanilla() comdat {
; ELF: define linkonce void @LinkOnce() comdat {
@@ -46,3 +56,4 @@ entry:
; CHECK: declare extern_weak void @ExternWeak()
; CHECK: define linkonce_odr void @LinkOnceOdr() comdat {
; CHECK: define weak_odr void @WeakOdr() comdat {
+; CHECK: define weak void @WeakComdat() comdat {
|
|
@llvm/pr-subscribers-llvm-transforms Author: Reid Kleckner (rnk) ChangesThis code avoids adding comdat groups to interposable linkage types (weak, linkonce (non-ODR)) to avoid changing semantics, since comdat elimination happens before weak/strong prevailaing symbol resolution. However, if the function is already in a comdat, we can add to the group without changing the semantics of the linked program. Fixes an issue uncovered in PR #126240 Full diff: https://github.com/llvm/llvm-project/pull/131929.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp b/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
index 398f83e3b3462..c5100ded8696c 100644
--- a/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
+++ b/llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
@@ -745,7 +745,7 @@ GlobalVariable *ModuleSanitizerCoverage::CreateFunctionLocalArrayInSection(
Constant::getNullValue(ArrayTy), "__sancov_gen_");
if (TargetTriple.supportsCOMDAT() &&
- (TargetTriple.isOSBinFormatELF() || !F.isInterposable()))
+ (F.hasComdat() || TargetTriple.isOSBinFormatELF() || !F.isInterposable()))
if (auto Comdat = getOrCreateFunctionComdat(F, TargetTriple))
Array->setComdat(Comdat);
Array->setSection(getSectionName(Section));
diff --git a/llvm/test/Instrumentation/SanitizerCoverage/interposable-symbol.ll b/llvm/test/Instrumentation/SanitizerCoverage/interposable-symbol.ll
index 29a7562ec8748..e8b23096ff33a 100644
--- a/llvm/test/Instrumentation/SanitizerCoverage/interposable-symbol.ll
+++ b/llvm/test/Instrumentation/SanitizerCoverage/interposable-symbol.ll
@@ -2,6 +2,8 @@
; RUN: opt < %s -passes='module(sancov-module)' -sanitizer-coverage-level=3 -sanitizer-coverage-trace-pc-guard -mtriple x86_64-linux-gnu -S | FileCheck %s --check-prefixes=CHECK,ELF
; RUN: opt < %s -passes='module(sancov-module)' -sanitizer-coverage-level=3 -sanitizer-coverage-trace-pc-guard -mtriple x86_64-windows-msvc -S | FileCheck %s --check-prefixes=CHECK,COFF
+$WeakComdat = comdat any
+
define void @Vanilla() {
entry:
ret void
@@ -29,14 +31,22 @@ entry:
ret void
}
+define weak void @WeakComdat() comdat {
+entry:
+ ret void
+}
+
+
; CHECK: $Vanilla = comdat nodeduplicate
; ELF: $LinkOnceOdr = comdat nodeduplicate
; COFF: $LinkOnceOdr = comdat any
+; CHECK: $WeakComdat = comdat any
; CHECK: @__sancov_gen_ = private global [1 x i32] zeroinitializer, section {{.*}}, comdat($Vanilla), align 4{{$}}
; CHECK-NEXT: @__sancov_gen_.1 = private global [1 x i32] zeroinitializer, section {{.*}}, align 4{{$}}
; CHECK-NEXT: @__sancov_gen_.2 = private global [1 x i32] zeroinitializer, section {{.*}}, align 4{{$}}
; CHECK-NEXT: @__sancov_gen_.3 = private global [1 x i32] zeroinitializer, section {{.*}}, comdat($LinkOnceOdr), align 4{{$}}
; CHECK-NEXT: @__sancov_gen_.4 = private global [1 x i32] zeroinitializer, section {{.*}}, comdat($WeakOdr), align 4{{$}}
+; CHECK-NEXT: @__sancov_gen_.5 = private global [1 x i32] zeroinitializer, section {{.*}}, comdat($WeakComdat), align 4{{$}}
; CHECK: define void @Vanilla() comdat {
; ELF: define linkonce void @LinkOnce() comdat {
@@ -46,3 +56,4 @@ entry:
; CHECK: declare extern_weak void @ExternWeak()
; CHECK: define linkonce_odr void @LinkOnceOdr() comdat {
; CHECK: define weak_odr void @WeakOdr() comdat {
+; CHECK: define weak void @WeakComdat() comdat {
|
petrhosek
approved these changes
Mar 18, 2025
Fznamznon
approved these changes
Mar 19, 2025
Fznamznon
added a commit
that referenced
this pull request
Mar 31, 2025
Whereas it is UB in terms of the standard to delete an array of objects via pointer whose static type doesn't match its dynamic type, MSVC supports an extension allowing to do it. Aside from array deletion not working correctly in the mentioned case, currently not having this extension implemented causes clang to generate code that is not compatible with the code generated by MSVC, because clang always puts scalar deleting destructor to the vftable. This PR aims to resolve these problems. It was reverted due to link time errors in chromium with sanitizer coverage enabled, which is fixed by #131929 . The second commit of this PR also contains a fix for a runtime failure in chromium reported in #126240 (comment) . Fixes #19772
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Mar 31, 2025
…tors (#133451) Whereas it is UB in terms of the standard to delete an array of objects via pointer whose static type doesn't match its dynamic type, MSVC supports an extension allowing to do it. Aside from array deletion not working correctly in the mentioned case, currently not having this extension implemented causes clang to generate code that is not compatible with the code generated by MSVC, because clang always puts scalar deleting destructor to the vftable. This PR aims to resolve these problems. It was reverted due to link time errors in chromium with sanitizer coverage enabled, which is fixed by llvm/llvm-project#131929 . The second commit of this PR also contains a fix for a runtime failure in chromium reported in llvm/llvm-project#126240 (comment) . Fixes llvm/llvm-project#19772
SchrodingerZhu
pushed a commit
to SchrodingerZhu/llvm-project
that referenced
this pull request
Mar 31, 2025
…133451) Whereas it is UB in terms of the standard to delete an array of objects via pointer whose static type doesn't match its dynamic type, MSVC supports an extension allowing to do it. Aside from array deletion not working correctly in the mentioned case, currently not having this extension implemented causes clang to generate code that is not compatible with the code generated by MSVC, because clang always puts scalar deleting destructor to the vftable. This PR aims to resolve these problems. It was reverted due to link time errors in chromium with sanitizer coverage enabled, which is fixed by llvm#131929 . The second commit of this PR also contains a fix for a runtime failure in chromium reported in llvm#126240 (comment) . Fixes llvm#19772
KornevNikita
pushed a commit
to intel/llvm
that referenced
this pull request
May 27, 2025
Whereas it is UB in terms of the standard to delete an array of objects via pointer whose static type doesn't match its dynamic type, MSVC supports an extension allowing to do it. Aside from array deletion not working correctly in the mentioned case, currently not having this extension implemented causes clang to generate code that is not compatible with the code generated by MSVC, because clang always puts scalar deleting destructor to the vftable. This PR aims to resolve these problems. It was reverted due to link time errors in chromium with sanitizer coverage enabled, which is fixed by llvm/llvm-project#131929 . The second commit of this PR also contains a fix for a runtime failure in chromium reported in llvm/llvm-project#126240 (comment) . Fixes llvm/llvm-project#19772
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This code avoids adding comdat groups to interposable linkage types (weak, linkonce (non-ODR)) to avoid changing semantics, since comdat elimination happens before weak/strong prevailaing symbol resolution. However, if the function is already in a comdat, we can add to the group without changing the semantics of the linked program.
Fixes an issue uncovered in PR #126240