[lld] 8ec1033 - [lld-macho] Deduplicate CFStrings during ICF
Jez Ng via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 8 05:34:17 PST 2022
Author: Jez Ng
Date: 2022-03-08T08:34:03-05:00
New Revision: 8ec10339330011baf22551ee59f2397ff3f1f499
URL: https://github.com/llvm/llvm-project/commit/8ec10339330011baf22551ee59f2397ff3f1f499
DIFF: https://github.com/llvm/llvm-project/commit/8ec10339330011baf22551ee59f2397ff3f1f499.diff
LOG: [lld-macho] Deduplicate CFStrings during ICF
`__cfstring` has embedded addends that foil ICF's hashing / equality
checks. (We can ignore embedded addends when doing ICF because the same
information gets recorded in our Reloc structs.) Therefore, in order to
properly dedup CFStrings, we create a mutable copy of the CFString and
zero out the embedded addends before performing any hashing / equality
checks.
(We did in fact have a partial implementation of CFString deduplication
already. However, it only worked when the cstrings they point to are at
identical offsets in their object files.)
I anticipate this approach can be extended to other similar
statically-allocated struct sections in the future.
In addition, we previously treated all references with differing addends
as unequal. This is not true when the references are to literals:
different addends may point to the same literal in the output binary. In
particular, `__cfstring` has such references to `__cstring`. I've
adjusted ICF's `equalsConstant` logic accordingly, and I've added a few
more tests to make sure the addend-comparison code path is adequately
covered.
Fixes https://github.com/llvm/llvm-project/issues/51281.
Reviewed By: #lld-macho, Roger
Differential Revision: https://reviews.llvm.org/D120137
Added:
lld/test/MachO/icf-undef.s
Modified:
lld/MachO/ICF.cpp
lld/test/MachO/cfstring-dedup.s
lld/test/MachO/icf.s
Removed:
################################################################################
diff --git a/lld/MachO/ICF.cpp b/lld/MachO/ICF.cpp
index b760effa7321f..7e23d067b8130 100644
--- a/lld/MachO/ICF.cpp
+++ b/lld/MachO/ICF.cpp
@@ -111,8 +111,6 @@ bool ICF::equalsConstant(const ConcatInputSection *ia,
return false;
if (ra.offset != rb.offset)
return false;
- if (ra.addend != rb.addend)
- return false;
if (ra.referent.is<Symbol *>() != rb.referent.is<Symbol *>())
return false;
@@ -125,16 +123,16 @@ bool ICF::equalsConstant(const ConcatInputSection *ia,
const auto *sb = rb.referent.get<Symbol *>();
if (sa->kind() != sb->kind())
return false;
- if (!isa<Defined>(sa)) {
- // ICF runs before Undefineds are reported.
- assert(isa<DylibSymbol>(sa) || isa<Undefined>(sa));
- return sa == sb;
- }
+ // ICF runs before Undefineds are treated (and potentially converted into
+ // DylibSymbols).
+ if (isa<DylibSymbol>(sa) || isa<Undefined>(sa))
+ return sa == sb && ra.addend == rb.addend;
+ assert(isa<Defined>(sa));
const auto *da = cast<Defined>(sa);
const auto *db = cast<Defined>(sb);
if (!da->isec || !db->isec) {
assert(da->isAbsolute() && db->isAbsolute());
- return da->value == db->value;
+ return da->value + ra.addend == db->value + rb.addend;
}
isecA = da->isec;
valueA = da->value;
@@ -151,7 +149,7 @@ bool ICF::equalsConstant(const ConcatInputSection *ia,
assert(isecA->kind() == isecB->kind());
// We will compare ConcatInputSection contents in equalsVariable.
if (isa<ConcatInputSection>(isecA))
- return true;
+ return ra.addend == rb.addend;
// Else we have two literal sections. References to them are equal iff their
// offsets in the output section are equal.
return isecA->getOffset(valueA + ra.addend) ==
@@ -389,6 +387,17 @@ void macho::foldIdenticalSections() {
}
}
parallelForEach(hashable, [](ConcatInputSection *isec) {
+ // __cfstring has embedded addends that foil ICF's hashing / equality
+ // checks. (We can ignore embedded addends when doing ICF because the same
+ // information gets recorded in our Reloc structs.) We therefore create a
+ // mutable copy of the CFString and zero out the embedded addends before
+ // performing any hashing / equality checks.
+ if (isCfStringSection(isec)) {
+ MutableArrayRef<uint8_t> copy = isec->data.copy(bAlloc());
+ for (const Reloc &r : isec->relocs)
+ target->relocateOne(copy.data() + r.offset, r, /*va=*/0, /*relocVA=*/0);
+ isec->data = copy;
+ }
assert(isec->icfEqClass[0] == 0); // don't overwrite a unique ID!
// Turn-on the top bit to guarantee that valid hashes have no collisions
// with the small-integer unique IDs for ICF-ineligible sections
diff --git a/lld/test/MachO/cfstring-dedup.s b/lld/test/MachO/cfstring-dedup.s
index 1a043064b6e0d..73aa50fa98b04 100644
--- a/lld/test/MachO/cfstring-dedup.s
+++ b/lld/test/MachO/cfstring-dedup.s
@@ -37,6 +37,10 @@
#--- foo1.s
.cstring
+L_.str.0:
+ .asciz "bar"
+## This string is at a
diff erent offset than the corresponding "foo" string in
+## foo2.s. Make sure that we treat references to either string as equivalent.
L_.str:
.asciz "foo"
@@ -57,7 +61,7 @@ _named_cfstring:
.quad 3 ## strlen
.section __TEXT,__ustring
-l_.str.2:
+l_.ustr:
.short 102 ## f
.short 111 ## o
.short 0 ## \0
@@ -77,7 +81,7 @@ L__unnamed_cfstring_.2:
.quad ___CFConstantStringClassReference
.long 2000 ## utf-16
.space 4
- .quad l_.str.2
+ .quad l_.ustr
.quad 4 ## strlen
.text
@@ -116,7 +120,7 @@ _named_cfstring:
.section __TEXT,__ustring
.p2align 1
-l_.str.2:
+l_.ustr:
.short 102 ## f
.short 111 ## o
.short 0 ## \0
@@ -129,7 +133,7 @@ L__unnamed_cfstring_.2:
.quad ___CFConstantStringClassReference
.long 2000 ## utf-16
.space 4
- .quad l_.str.2
+ .quad l_.ustr
.quad 4 ## strlen
.text
diff --git a/lld/test/MachO/icf-undef.s b/lld/test/MachO/icf-undef.s
new file mode 100644
index 0000000000000..ef445fa583455
--- /dev/null
+++ b/lld/test/MachO/icf-undef.s
@@ -0,0 +1,28 @@
+# REQUIRES: x86
+# RUN: rm -rf %t; split-file %s %t
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin19.0.0 %t/test.s -o %t/test.o
+
+## Check that we correctly dedup sections that reference dynamic-lookup symbols.
+# RUN: %lld -lSystem -dylib --icf=all -undefined dynamic_lookup -o %t/test %t/test.o
+# RUN: llvm-objdump --macho --syms %t/test | FileCheck %s
+
+## Check that we still raise an error when using regular undefined symbol
+## treatment.
+# RUN: not %lld -lSystem -dylib --icf=all -o /dev/null %t/test.o 2>&1 | \
+# RUN: FileCheck %s --check-prefix=ERR
+
+# CHECK: [[#%x,ADDR:]] l F __TEXT,__text _foo
+# CHECK: [[#ADDR]] l F __TEXT,__text _bar
+
+# ERR: error: undefined symbol: _undef
+
+#--- test.s
+
+.subsections_via_symbols
+
+_foo:
+ callq _undef + 1
+
+_bar:
+ callq _undef + 1
diff --git a/lld/test/MachO/icf.s b/lld/test/MachO/icf.s
index 5e61e941bffa2..4405bf8c13746 100644
--- a/lld/test/MachO/icf.s
+++ b/lld/test/MachO/icf.s
@@ -22,11 +22,13 @@
# CHECK: [[#%x,DYLIB_REF_2:]] l F __TEXT,__text _dylib_ref_1
# CHECK: [[#%x,DYLIB_REF_2:]] l F __TEXT,__text _dylib_ref_2
# CHECK: [[#%x,DYLIB_REF_3:]] l F __TEXT,__text _dylib_ref_3
+# CHECK: [[#%x,DYLIB_REF_4:]] l F __TEXT,__text _dylib_ref_4
# CHECK: [[#%x,ALT:]] l F __TEXT,__text _alt
# CHECK: [[#%x,WITH_ALT_ENTRY:]] l F __TEXT,__text _with_alt_entry
# CHECK: [[#%x,WITH_ALT_ENTRY:]] l F __TEXT,__text _no_alt_entry
# CHECK: [[#%x,DEFINED_REF_WITH_ADDEND_2:]] l F __TEXT,__text _defined_ref_with_addend_1
# CHECK: [[#%x,DEFINED_REF_WITH_ADDEND_2:]] l F __TEXT,__text _defined_ref_with_addend_2
+# CHECK: [[#%x,DEFINED_REF_WITH_ADDEND_3:]] l F __TEXT,__text _defined_ref_with_addend_3
# CHECK: [[#%x,RECURSIVE:]] l F __TEXT,__text _recursive
# CHECK: [[#%x,CALL_RECURSIVE_2:]] l F __TEXT,__text _call_recursive_1
# CHECK: [[#%x,CALL_RECURSIVE_2:]] l F __TEXT,__text _call_recursive_2
@@ -55,11 +57,13 @@
# CHECK: callq 0x[[#%x,DYLIB_REF_2:]] <_dylib_ref_2>
# CHECK: callq 0x[[#%x,DYLIB_REF_2:]] <_dylib_ref_2>
# CHECK: callq 0x[[#%x,DYLIB_REF_3:]] <_dylib_ref_3>
+# CHECK: callq 0x[[#%x,DYLIB_REF_4:]] <_dylib_ref_4>
# CHECK: callq 0x[[#%x,ALT:]] <_alt>
# CHECK: callq 0x[[#%x,WITH_ALT_ENTRY:]] <_with_alt_entry>
# CHECK: callq 0x[[#%x,WITH_ALT_ENTRY:]] <_with_alt_entry>
# CHECK: callq 0x[[#%x,DEFINED_REF_WITH_ADDEND_2:]] <_defined_ref_with_addend_2>
# CHECK: callq 0x[[#%x,DEFINED_REF_WITH_ADDEND_2:]] <_defined_ref_with_addend_2>
+# CHECK: callq 0x[[#%x,DEFINED_REF_WITH_ADDEND_3:]] <_defined_ref_with_addend_3>
# CHECK: callq 0x[[#%x,RECURSIVE:]] <_recursive>
# CHECK: callq 0x[[#%x,CALL_RECURSIVE_2:]] <_call_recursive_2>
# CHECK: callq 0x[[#%x,CALL_RECURSIVE_2:]] <_call_recursive_2>
@@ -132,6 +136,11 @@ _dylib_ref_3:
mov ___inf at GOTPCREL(%rip), %rax
callq ___inf
+## No fold: referent dylib addend
diff ers
+_dylib_ref_4:
+ mov ___nan + 1 at GOTPCREL(%rip), %rax
+ callq ___inf + 1
+
## We can merge two sections even if one of them has an alt entry. Just make
## sure we don't merge the alt entry symbol with a regular symbol.
.alt_entry _alt
@@ -150,6 +159,10 @@ _defined_ref_with_addend_1:
_defined_ref_with_addend_2:
callq _with_alt_entry + 4
+# No fold: addend
diff ers
+_defined_ref_with_addend_3:
+ callq _with_alt_entry + 8
+
## _recursive has the same body as its next two callers, but cannot be folded
## with them.
_recursive:
@@ -251,11 +264,13 @@ _main:
callq _dylib_ref_1
callq _dylib_ref_2
callq _dylib_ref_3
+ callq _dylib_ref_4
callq _alt
callq _with_alt_entry
callq _no_alt_entry
callq _defined_ref_with_addend_1
callq _defined_ref_with_addend_2
+ callq _defined_ref_with_addend_3
callq _recursive
callq _call_recursive_1
callq _call_recursive_2
More information about the llvm-commits
mailing list