[lld-macho] Fix ICF differentiation of safe_thunks relocs #111811
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
alx32
changed the title
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
|
@llvm/pr-subscribers-lld Author: None (alx32) ChangesIn If two functions are identical except for references to different To address this issue, we modify the ICF comparison to explicitly check for references to Full diff: https://github.com/llvm/llvm-project/pull/111811.diff 2 Files Affected:
diff --git a/lld/MachO/ICF.cpp b/lld/MachO/ICF.cpp
index 2ff962b06e3679..aedaecfdeb2c01 100644
--- a/lld/MachO/ICF.cpp
+++ b/lld/MachO/ICF.cpp
@@ -147,6 +147,17 @@ bool ICF::equalsConstant(const ConcatInputSection *ia,
isecB = rb.referent.get<InputSection *>();
}
+ // Typically, we should not encounter sections marked with `keepUnique` at
+ // this point as they would have resulted in different hashes and therefore
+ // no need for a full comparison.
+ // However, in `safe_thunks` mode, it's possible for two different
+ // relocations to reference identical `keepUnique` functions that will be
+ // distinguished later via thunks - so we need to handle this case
+ // explicitly.
+ if ((isecA != isecB) && ((isecA->keepUnique && isCodeSection(isecA)) ||
+ (isecB->keepUnique && isCodeSection(isecB))))
+ return false;
+
if (isecA->parent != isecB->parent)
return false;
// Sections with identical parents should be of the same kind.
diff --git a/lld/test/MachO/icf-safe-thunks.ll b/lld/test/MachO/icf-safe-thunks.ll
index 238e90f952e160..da2a8e46a5e9b5 100644
--- a/lld/test/MachO/icf-safe-thunks.ll
+++ b/lld/test/MachO/icf-safe-thunks.ll
@@ -22,6 +22,13 @@
; CHECK-ARM64-NEXT: _func_3identical_v3_canmerge:
; CHECK-ARM64-NEXT: mov {{.*}}, #0x21
;
+; CHECK-ARM64: _func_call_thunked_1_nomerge:
+; CHECK-ARM64-NEXT: stp x29
+;
+; CHECK-ARM64: _func_call_thunked_2_nomerge:
+; CHECK-ARM64-NEXT: _func_call_thunked_2_merge:
+; CHECK-ARM64-NEXT: stp x29
+;
; CHECK-ARM64: _call_all_funcs:
; CHECK-ARM64-NEXT: stp x29
;
@@ -43,6 +50,9 @@
; CHECK-ARM64-MAP-NEXT: 0x00000010 [ 2] _func_3identical_v1_canmerge
; CHECK-ARM64-MAP-NEXT: 0x00000000 [ 2] _func_3identical_v2_canmerge
; CHECK-ARM64-MAP-NEXT: 0x00000000 [ 2] _func_3identical_v3_canmerge
+; CHECK-ARM64-MAP-NEXT: 0x00000020 [ 2] _func_call_thunked_1_nomerge
+; CHECK-ARM64-MAP-NEXT: 0x00000020 [ 2] _func_call_thunked_2_nomerge
+; CHECK-ARM64-MAP-NEXT: 0x00000000 [ 2] _func_call_thunked_2_merge
; CHECK-ARM64-MAP-NEXT: 0x00000034 [ 2] _call_all_funcs
; CHECK-ARM64-MAP-NEXT: 0x00000050 [ 2] _take_func_addr
; CHECK-ARM64-MAP-NEXT: 0x00000004 [ 2] _func_2identical_v2
@@ -125,6 +135,30 @@ entry:
ret void
}
+; Function Attrs: mustprogress nofree noinline norecurse nounwind ssp memory(readwrite, argmem: none) uwtable(sync)
+define void @func_call_thunked_1_nomerge() local_unnamed_addr #0 {
+entry:
+ tail call void @func_2identical_v1()
+ store volatile i8 77, ptr @g_val, align 1, !tbaa !5
+ ret void
+}
+
+; Function Attrs: mustprogress nofree noinline norecurse nounwind ssp memory(readwrite, argmem: none) uwtable(sync)
+define void @func_call_thunked_2_nomerge() local_unnamed_addr #0 {
+entry:
+ tail call void @func_2identical_v2()
+ store volatile i8 77, ptr @g_val, align 1, !tbaa !5
+ ret void
+}
+
+; Function Attrs: mustprogress nofree noinline norecurse nounwind ssp memory(readwrite, argmem: none) uwtable(sync)
+define void @func_call_thunked_2_merge() local_unnamed_addr #0 {
+entry:
+ tail call void @func_2identical_v2()
+ store volatile i8 77, ptr @g_val, align 1, !tbaa !5
+ ret void
+}
+
; Function Attrs: mustprogress nofree noinline norecurse nounwind ssp uwtable(sync)
define void @call_all_funcs() local_unnamed_addr #1 {
entry:
@@ -227,6 +261,16 @@ attributes #1 = { mustprogress nofree noinline norecurse nounwind ssp uwtable(sy
; g_val = 33;
; }
;
+; ATTR void func_call_thunked_1_nomerge() {
+; func_2identical_v1();
+; g_val = 77;
+; }
+;
+; ATTR void func_call_thunked_2_nomerge() {
+; func_2identical_v2();
+; g_val = 77;
+; }
+;
; ATTR void call_all_funcs() {
; func_unique_1();
; func_unique_2_canmerge();
|
|
@llvm/pr-subscribers-lld-macho Author: None (alx32) ChangesIn If two functions are identical except for references to different To address this issue, we modify the ICF comparison to explicitly check for references to Full diff: https://github.com/llvm/llvm-project/pull/111811.diff 2 Files Affected:
diff --git a/lld/MachO/ICF.cpp b/lld/MachO/ICF.cpp
index 2ff962b06e3679..aedaecfdeb2c01 100644
--- a/lld/MachO/ICF.cpp
+++ b/lld/MachO/ICF.cpp
@@ -147,6 +147,17 @@ bool ICF::equalsConstant(const ConcatInputSection *ia,
isecB = rb.referent.get<InputSection *>();
}
+ // Typically, we should not encounter sections marked with `keepUnique` at
+ // this point as they would have resulted in different hashes and therefore
+ // no need for a full comparison.
+ // However, in `safe_thunks` mode, it's possible for two different
+ // relocations to reference identical `keepUnique` functions that will be
+ // distinguished later via thunks - so we need to handle this case
+ // explicitly.
+ if ((isecA != isecB) && ((isecA->keepUnique && isCodeSection(isecA)) ||
+ (isecB->keepUnique && isCodeSection(isecB))))
+ return false;
+
if (isecA->parent != isecB->parent)
return false;
// Sections with identical parents should be of the same kind.
diff --git a/lld/test/MachO/icf-safe-thunks.ll b/lld/test/MachO/icf-safe-thunks.ll
index 238e90f952e160..da2a8e46a5e9b5 100644
--- a/lld/test/MachO/icf-safe-thunks.ll
+++ b/lld/test/MachO/icf-safe-thunks.ll
@@ -22,6 +22,13 @@
; CHECK-ARM64-NEXT: _func_3identical_v3_canmerge:
; CHECK-ARM64-NEXT: mov {{.*}}, #0x21
;
+; CHECK-ARM64: _func_call_thunked_1_nomerge:
+; CHECK-ARM64-NEXT: stp x29
+;
+; CHECK-ARM64: _func_call_thunked_2_nomerge:
+; CHECK-ARM64-NEXT: _func_call_thunked_2_merge:
+; CHECK-ARM64-NEXT: stp x29
+;
; CHECK-ARM64: _call_all_funcs:
; CHECK-ARM64-NEXT: stp x29
;
@@ -43,6 +50,9 @@
; CHECK-ARM64-MAP-NEXT: 0x00000010 [ 2] _func_3identical_v1_canmerge
; CHECK-ARM64-MAP-NEXT: 0x00000000 [ 2] _func_3identical_v2_canmerge
; CHECK-ARM64-MAP-NEXT: 0x00000000 [ 2] _func_3identical_v3_canmerge
+; CHECK-ARM64-MAP-NEXT: 0x00000020 [ 2] _func_call_thunked_1_nomerge
+; CHECK-ARM64-MAP-NEXT: 0x00000020 [ 2] _func_call_thunked_2_nomerge
+; CHECK-ARM64-MAP-NEXT: 0x00000000 [ 2] _func_call_thunked_2_merge
; CHECK-ARM64-MAP-NEXT: 0x00000034 [ 2] _call_all_funcs
; CHECK-ARM64-MAP-NEXT: 0x00000050 [ 2] _take_func_addr
; CHECK-ARM64-MAP-NEXT: 0x00000004 [ 2] _func_2identical_v2
@@ -125,6 +135,30 @@ entry:
ret void
}
+; Function Attrs: mustprogress nofree noinline norecurse nounwind ssp memory(readwrite, argmem: none) uwtable(sync)
+define void @func_call_thunked_1_nomerge() local_unnamed_addr #0 {
+entry:
+ tail call void @func_2identical_v1()
+ store volatile i8 77, ptr @g_val, align 1, !tbaa !5
+ ret void
+}
+
+; Function Attrs: mustprogress nofree noinline norecurse nounwind ssp memory(readwrite, argmem: none) uwtable(sync)
+define void @func_call_thunked_2_nomerge() local_unnamed_addr #0 {
+entry:
+ tail call void @func_2identical_v2()
+ store volatile i8 77, ptr @g_val, align 1, !tbaa !5
+ ret void
+}
+
+; Function Attrs: mustprogress nofree noinline norecurse nounwind ssp memory(readwrite, argmem: none) uwtable(sync)
+define void @func_call_thunked_2_merge() local_unnamed_addr #0 {
+entry:
+ tail call void @func_2identical_v2()
+ store volatile i8 77, ptr @g_val, align 1, !tbaa !5
+ ret void
+}
+
; Function Attrs: mustprogress nofree noinline norecurse nounwind ssp uwtable(sync)
define void @call_all_funcs() local_unnamed_addr #1 {
entry:
@@ -227,6 +261,16 @@ attributes #1 = { mustprogress nofree noinline norecurse nounwind ssp uwtable(sy
; g_val = 33;
; }
;
+; ATTR void func_call_thunked_1_nomerge() {
+; func_2identical_v1();
+; g_val = 77;
+; }
+;
+; ATTR void func_call_thunked_2_nomerge() {
+; func_2identical_v2();
+; g_val = 77;
+; }
+;
; ATTR void call_all_funcs() {
; func_unique_1();
; func_unique_2_canmerge();
|
alx32
force-pushed
the
13_icf_safe_thunks_unique_child
branch
from
8feac95 to
85b148b
Compare
kyulee-com
approved these changes
Oct 10, 2024
DanielCChen
pushed a commit
to DanielCChen/llvm-project
that referenced
this pull request
Oct 16, 2024
In `--icf=safe_thunks` mode, the linker differentiates `keepUnique` functions by creating thunks during a post-processing step after Identical Code Folding (ICF). While this ensures that `keepUnique` functions themselves are not incorrectly merged, it overlooks functions that reference these `keepUnique` symbols. If two functions are identical except for references to different `keepUnique` functions, the current ICF algorithm incorrectly considers them identical because it doesn't account for the future differentiation introduced by thunks. This leads to incorrect deduplication of functions that should remain distinct. To address this issue, we modify the ICF comparison to explicitly check for references to `keepUnique` functions during deduplication. By doing so, functions that reference different `keepUnique` symbols are correctly identified as distinct, preventing erroneous merging and ensuring the correctness of the linked output.
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.
In
--icf=safe_thunksmode, the linker differentiateskeepUniquefunctions by creating thunks during a post-processing step after Identical Code Folding (ICF). While this ensures thatkeepUniquefunctions themselves are not incorrectly merged, it overlooks functions that reference thesekeepUniquesymbols.If two functions are identical except for references to different
keepUniquefunctions, the current ICF algorithm incorrectly considers them identical because it doesn't account for the future differentiation introduced by thunks. This leads to incorrect deduplication of functions that should remain distinct.To address this issue, we modify the ICF comparison to explicitly check for references to
keepUniquefunctions during deduplication. By doing so, functions that reference differentkeepUniquesymbols are correctly identified as distinct, preventing erroneous merging and ensuring the correctness of the linked output.