Skip to content

[lld-macho] Mark local personality functions as INDIRECT_SYMBOL_LOCAL #95171

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 13, 2024

Conversation

BertalanD
Copy link
Member

This expands on the fix in 4e572db. The issue is pretty similar: we might put symbols in the GOT which don't need run-time binding, locally defined personality symbols in this case. We should set their indirect symbol table entries to INDIRECT_SYMBOL_LOCAL to help strip remove these local names from the symbol table.

Checking if the symbol is private-extern doesn't cover all cases; it can also be a non-weak extern function too, for instance; use the needsBinding() helper to determine it. This was the case for the personality function in statically linked Rust executables.

The extra non-LOCAL symbols triggered a bug in Apple's strip implementation. As the indirect value for the personality function was not set to the flag, but the symbol didn't require binding, it tried to make the symbol local, overwriting the GOT entry with the function's address in the process. This normally wouldn't be a problem, but if chained fixups are used, the fixup also encodes the offset to the next fixup, and it effectively zeroed this offset out, causing the remaining relocations on the page to not be performed by dyld.

This caused the crash in https://issues.chromium.org/issues/325410295

The change in tests is a bit ugly, as a lot of symbol information is now removed by turning more symbols LOCAL.

This expands on the fix in 4e572db. The issue is pretty similar: we
might put symbols in the GOT which don't need run-time binding, locally
defined personality symbols in this case. We should set their indirect
symbol table entries to `INDIRECT_SYMBOL_LOCAL` to help `strip` remove
these local names from the symbol table.

Checking if the symbol is private-extern doesn't cover all cases; it can
also be a non-weak extern function too, for instance; use the
`needsBinding()` helper to determine it. This was the case for the
personality function in statically linked Rust executables.

The extra non-`LOCAL` symbols triggered a bug in Apple's `strip`
implementation. As the indirect value for the personality function was
not set to the flag, but the symbol didn't require binding, it tried to
make the symbol local, overwriting the GOT entry with the function's
address in the process. This normally wouldn't be a problem, but if
chained fixups are used, the fixup also encodes the offset to the next
fixup, and it effectively zeroed this offset out, causing the remaining
relocations on the page to not be performed by dyld.

This caused the crash in https://issues.chromium.org/issues/325410295

The change in tests is a bit ugly, as a lot of symbol information is now
removed by turning more symbols `LOCAL`.
@llvmbot
Copy link
Member

llvmbot commented Jun 12, 2024

@llvm/pr-subscribers-lld-macho

Author: Daniel Bertalan (BertalanD)

Changes

This expands on the fix in 4e572db. The issue is pretty similar: we might put symbols in the GOT which don't need run-time binding, locally defined personality symbols in this case. We should set their indirect symbol table entries to INDIRECT_SYMBOL_LOCAL to help strip remove these local names from the symbol table.

Checking if the symbol is private-extern doesn't cover all cases; it can also be a non-weak extern function too, for instance; use the needsBinding() helper to determine it. This was the case for the personality function in statically linked Rust executables.

The extra non-LOCAL symbols triggered a bug in Apple's strip implementation. As the indirect value for the personality function was not set to the flag, but the symbol didn't require binding, it tried to make the symbol local, overwriting the GOT entry with the function's address in the process. This normally wouldn't be a problem, but if chained fixups are used, the fixup also encodes the offset to the next fixup, and it effectively zeroed this offset out, causing the remaining relocations on the page to not be performed by dyld.

This caused the crash in https://issues.chromium.org/issues/325410295

The change in tests is a bit ugly, as a lot of symbol information is now removed by turning more symbols LOCAL.


Full diff: https://github.com/llvm/llvm-project/pull/95171.diff

6 Files Affected:

  • (modified) lld/MachO/SyntheticSections.cpp (+1-4)
  • (modified) lld/test/MachO/arm64-reloc-pointer-to-got.s (+2-2)
  • (modified) lld/test/MachO/compact-unwind-both-local-and-dylib-personality.s (+11-11)
  • (modified) lld/test/MachO/compact-unwind.s (+2-2)
  • (modified) lld/test/MachO/dead-strip.s (+1-1)
  • (modified) lld/test/MachO/invalid/compact-unwind-personalities.s (+3-3)
diff --git a/lld/MachO/SyntheticSections.cpp b/lld/MachO/SyntheticSections.cpp
index b3fe223938bf5..6a7284fefcf29 100644
--- a/lld/MachO/SyntheticSections.cpp
+++ b/lld/MachO/SyntheticSections.cpp
@@ -1478,11 +1478,8 @@ void IndirectSymtabSection::finalizeContents() {
 }
 
 static uint32_t indirectValue(const Symbol *sym) {
-  if (sym->symtabIndex == UINT32_MAX)
+  if (sym->symtabIndex == UINT32_MAX || !needsBinding(sym))
     return INDIRECT_SYMBOL_LOCAL;
-  if (auto *defined = dyn_cast<Defined>(sym))
-    if (defined->privateExtern)
-      return INDIRECT_SYMBOL_LOCAL;
   return sym->symtabIndex;
 }
 
diff --git a/lld/test/MachO/arm64-reloc-pointer-to-got.s b/lld/test/MachO/arm64-reloc-pointer-to-got.s
index 3d9eba0555c8a..2551d71256f4c 100644
--- a/lld/test/MachO/arm64-reloc-pointer-to-got.s
+++ b/lld/test/MachO/arm64-reloc-pointer-to-got.s
@@ -8,8 +8,8 @@
 ## POINTER_TO_GOT, we should still be able to relax this GOT_LOAD reference to
 ## it.
 # CHECK:      _main:
-# CHECK-NEXT: adrp x8, [[#]] ;
-# CHECK-NEXT: ldr  x8, [x8] ; literal pool symbol address: _foo
+# CHECK-NEXT: adrp x8, [[#]] ; 0x100004000
+# CHECK-NEXT: ldr  x8, [x8]
 # CHECK-NEXT: ret
 
 # CHECK: Idx   Name          Size     VMA              Type
diff --git a/lld/test/MachO/compact-unwind-both-local-and-dylib-personality.s b/lld/test/MachO/compact-unwind-both-local-and-dylib-personality.s
index 35f39ba5fb1e2..caf774c7c2a19 100644
--- a/lld/test/MachO/compact-unwind-both-local-and-dylib-personality.s
+++ b/lld/test/MachO/compact-unwind-both-local-and-dylib-personality.s
@@ -45,16 +45,16 @@
 # A:      Indirect symbols for (__DATA_CONST,__got) 4 entries
 # A-NEXT: address                    index name
 # A:      0x[[#%x,GXX_PERSONALITY_LO:]] [[#]] ___gxx_personality_v0
-# A:      0x[[#%x,PERSONALITY_1:]]      [[#]] _personality_1
-# A:      0x[[#%x,PERSONALITY_2:]]      [[#]] _personality_2
-# A:      0x[[#%x,GXX_PERSONALITY_HI:]] [[#]] ___gxx_personality_v0
+# A:      0x[[#%x,PERSONALITY_1:]]      LOCAL
+# A:      0x[[#%x,PERSONALITY_2:]]      LOCAL
+# A:      0x[[#%x,GXX_PERSONALITY_HI:]] LOCAL
 
 # BC:      Indirect symbols for (__DATA_CONST,__got)
 # BC-NEXT: address                    index name
 # BC:      0x[[#%x,GXX_PERSONALITY_LO:]] LOCAL
-# C:       0x[[#%x,GXX_PERSONALITY_HI:]] [[#]] ___gxx_personality_v0
-# BC:      0x[[#%x,PERSONALITY_1:]]      [[#]] _personality_1
-# BC:      0x[[#%x,PERSONALITY_2:]]      [[#]] _personality_2
+# C:       0x[[#%x,GXX_PERSONALITY_HI:]] LOCAL
+# BC:      0x[[#%x,PERSONALITY_1:]]      LOCAL
+# BC:      0x[[#%x,PERSONALITY_2:]]      LOCAL
 # BC-EMPTY:
 
 # CHECK:        Personality functions: (count = 3)
@@ -70,11 +70,11 @@
 # D:      Indirect symbols for (__DATA_CONST,__got) 6 entries
 # D-NEXT: address                    index name
 # D:      0x[[#%x,GXX_PERSONALITY_HI:]] [[#]] ___gxx_personality_v0
-# D:      0x[[#%x,PERSONALITY_1:]] [[#]] _personality_1
-# D:      0x[[#%x,PERSONALITY_2:]] [[#]] _personality_2
-# D:      0x[[#%x,PERSONALITY_3:]] [[#]] _personality_3
-# D:      0x[[#%x,PERSONALITY_4:]] [[#]] _personality_4
-# D:      0x[[#%x,GXX_PERSONALITY_LO:]] [[#]] ___gxx_personality_v0
+# D:      0x[[#%x,PERSONALITY_1:]]      LOCAL
+# D:      0x[[#%x,PERSONALITY_2:]]      LOCAL
+# D:      0x[[#%x,PERSONALITY_3:]]      LOCAL
+# D:      0x[[#%x,PERSONALITY_4:]]      LOCAL
+# D:      0x[[#%x,GXX_PERSONALITY_LO:]] LOCAL
 
 # D: Contents of __unwind_info section:
 # D:  Personality functions: (count = 1)
diff --git a/lld/test/MachO/compact-unwind.s b/lld/test/MachO/compact-unwind.s
index 27e4b44dc0b09..6516f7162e290 100644
--- a/lld/test/MachO/compact-unwind.s
+++ b/lld/test/MachO/compact-unwind.s
@@ -29,12 +29,12 @@
 # FIRST:      Indirect symbols for (__DATA_CONST,__got)
 # FIRST-NEXT: address                    index name
 # FIRST-DAG:  0x[[#%x,GXX_PERSONALITY:]] [[#]] ___gxx_personality_v0
-# FIRST-DAG:  0x[[#%x,MY_PERSONALITY:]]
+# FIRST-DAG:  0x[[#%x,MY_PERSONALITY:]] LOCAL
 
 # SECOND:      Indirect symbols for (__DATA_CONST,__got)
 # SECOND-NEXT: address                    index name
 # SECOND-DAG:  0x[[#%x,GXX_PERSONALITY:]] [[#]] ___gxx_personality_v0
-# SECOND-DAG:  0x[[#%x,MY_PERSONALITY:]]  [[#]] _my_personality
+# SECOND-DAG:  0x[[#%x,MY_PERSONALITY:]] LOCAL
 
 # CHECK:      SYMBOL TABLE:
 # CHECK-DAG:  [[#%x,MAIN:]]       g  F __TEXT,__text _main
diff --git a/lld/test/MachO/dead-strip.s b/lld/test/MachO/dead-strip.s
index f60305a4ec494..d4350154695ff 100644
--- a/lld/test/MachO/dead-strip.s
+++ b/lld/test/MachO/dead-strip.s
@@ -35,7 +35,7 @@
 # EXEC-DAG:   g {{.*}} __mh_execute_header
 # EXECDATA-LABEL: Indirect symbols
 # EXECDATA-NEXT:  name
-# EXECDATA-NEXT:  _ref_com
+# EXECDATA-NEXT:  LOCAL
 # EXECDATA-LABEL: Contents of (__DATA,__ref_section) section
 # EXECDATA-NEXT:   04 00 00 00 00 00 00 00 05 00 00 00 00 00 00 00
 # EXECDATA-LABEL: Exports trie:
diff --git a/lld/test/MachO/invalid/compact-unwind-personalities.s b/lld/test/MachO/invalid/compact-unwind-personalities.s
index 25ee4bd17929f..9cc474de83638 100644
--- a/lld/test/MachO/invalid/compact-unwind-personalities.s
+++ b/lld/test/MachO/invalid/compact-unwind-personalities.s
@@ -14,9 +14,9 @@
 # DWARF: Indirect symbols
 # DWARF: address            index name
 # DWARF: 0x[[#%x,GXX_PERSONALITY:]]     {{.*}} ___gxx_personality_v0
-# DWARF: 0x[[#%x,PERSONALITY_1:]]     {{.*}} _personality_1
-# DWARF: 0x[[#%x,PERSONALITY_2:]]     {{.*}} _personality_2
-# DWARF: 0x[[#%x,PERSONALITY_3:]]     {{.*}} _personality_3
+# DWARF: 0x[[#%x,PERSONALITY_1:]]       LOCAL
+# DWARF: 0x[[#%x,PERSONALITY_2:]]       LOCAL
+# DWARF: 0x[[#%x,PERSONALITY_3:]]       LOCAL
 # DWARF: .eh_frame contents:
 # DWARF: Personality Address: [[#%.16x,GXX_PERSONALITY]]
 # DWARF: Personality Address: [[#%.16x,PERSONALITY_1]]

@llvmbot
Copy link
Member

llvmbot commented Jun 12, 2024

@llvm/pr-subscribers-lld

Author: Daniel Bertalan (BertalanD)

Changes

This expands on the fix in 4e572db. The issue is pretty similar: we might put symbols in the GOT which don't need run-time binding, locally defined personality symbols in this case. We should set their indirect symbol table entries to INDIRECT_SYMBOL_LOCAL to help strip remove these local names from the symbol table.

Checking if the symbol is private-extern doesn't cover all cases; it can also be a non-weak extern function too, for instance; use the needsBinding() helper to determine it. This was the case for the personality function in statically linked Rust executables.

The extra non-LOCAL symbols triggered a bug in Apple's strip implementation. As the indirect value for the personality function was not set to the flag, but the symbol didn't require binding, it tried to make the symbol local, overwriting the GOT entry with the function's address in the process. This normally wouldn't be a problem, but if chained fixups are used, the fixup also encodes the offset to the next fixup, and it effectively zeroed this offset out, causing the remaining relocations on the page to not be performed by dyld.

This caused the crash in https://issues.chromium.org/issues/325410295

The change in tests is a bit ugly, as a lot of symbol information is now removed by turning more symbols LOCAL.


Full diff: https://github.com/llvm/llvm-project/pull/95171.diff

6 Files Affected:

  • (modified) lld/MachO/SyntheticSections.cpp (+1-4)
  • (modified) lld/test/MachO/arm64-reloc-pointer-to-got.s (+2-2)
  • (modified) lld/test/MachO/compact-unwind-both-local-and-dylib-personality.s (+11-11)
  • (modified) lld/test/MachO/compact-unwind.s (+2-2)
  • (modified) lld/test/MachO/dead-strip.s (+1-1)
  • (modified) lld/test/MachO/invalid/compact-unwind-personalities.s (+3-3)
diff --git a/lld/MachO/SyntheticSections.cpp b/lld/MachO/SyntheticSections.cpp
index b3fe223938bf5..6a7284fefcf29 100644
--- a/lld/MachO/SyntheticSections.cpp
+++ b/lld/MachO/SyntheticSections.cpp
@@ -1478,11 +1478,8 @@ void IndirectSymtabSection::finalizeContents() {
 }
 
 static uint32_t indirectValue(const Symbol *sym) {
-  if (sym->symtabIndex == UINT32_MAX)
+  if (sym->symtabIndex == UINT32_MAX || !needsBinding(sym))
     return INDIRECT_SYMBOL_LOCAL;
-  if (auto *defined = dyn_cast<Defined>(sym))
-    if (defined->privateExtern)
-      return INDIRECT_SYMBOL_LOCAL;
   return sym->symtabIndex;
 }
 
diff --git a/lld/test/MachO/arm64-reloc-pointer-to-got.s b/lld/test/MachO/arm64-reloc-pointer-to-got.s
index 3d9eba0555c8a..2551d71256f4c 100644
--- a/lld/test/MachO/arm64-reloc-pointer-to-got.s
+++ b/lld/test/MachO/arm64-reloc-pointer-to-got.s
@@ -8,8 +8,8 @@
 ## POINTER_TO_GOT, we should still be able to relax this GOT_LOAD reference to
 ## it.
 # CHECK:      _main:
-# CHECK-NEXT: adrp x8, [[#]] ;
-# CHECK-NEXT: ldr  x8, [x8] ; literal pool symbol address: _foo
+# CHECK-NEXT: adrp x8, [[#]] ; 0x100004000
+# CHECK-NEXT: ldr  x8, [x8]
 # CHECK-NEXT: ret
 
 # CHECK: Idx   Name          Size     VMA              Type
diff --git a/lld/test/MachO/compact-unwind-both-local-and-dylib-personality.s b/lld/test/MachO/compact-unwind-both-local-and-dylib-personality.s
index 35f39ba5fb1e2..caf774c7c2a19 100644
--- a/lld/test/MachO/compact-unwind-both-local-and-dylib-personality.s
+++ b/lld/test/MachO/compact-unwind-both-local-and-dylib-personality.s
@@ -45,16 +45,16 @@
 # A:      Indirect symbols for (__DATA_CONST,__got) 4 entries
 # A-NEXT: address                    index name
 # A:      0x[[#%x,GXX_PERSONALITY_LO:]] [[#]] ___gxx_personality_v0
-# A:      0x[[#%x,PERSONALITY_1:]]      [[#]] _personality_1
-# A:      0x[[#%x,PERSONALITY_2:]]      [[#]] _personality_2
-# A:      0x[[#%x,GXX_PERSONALITY_HI:]] [[#]] ___gxx_personality_v0
+# A:      0x[[#%x,PERSONALITY_1:]]      LOCAL
+# A:      0x[[#%x,PERSONALITY_2:]]      LOCAL
+# A:      0x[[#%x,GXX_PERSONALITY_HI:]] LOCAL
 
 # BC:      Indirect symbols for (__DATA_CONST,__got)
 # BC-NEXT: address                    index name
 # BC:      0x[[#%x,GXX_PERSONALITY_LO:]] LOCAL
-# C:       0x[[#%x,GXX_PERSONALITY_HI:]] [[#]] ___gxx_personality_v0
-# BC:      0x[[#%x,PERSONALITY_1:]]      [[#]] _personality_1
-# BC:      0x[[#%x,PERSONALITY_2:]]      [[#]] _personality_2
+# C:       0x[[#%x,GXX_PERSONALITY_HI:]] LOCAL
+# BC:      0x[[#%x,PERSONALITY_1:]]      LOCAL
+# BC:      0x[[#%x,PERSONALITY_2:]]      LOCAL
 # BC-EMPTY:
 
 # CHECK:        Personality functions: (count = 3)
@@ -70,11 +70,11 @@
 # D:      Indirect symbols for (__DATA_CONST,__got) 6 entries
 # D-NEXT: address                    index name
 # D:      0x[[#%x,GXX_PERSONALITY_HI:]] [[#]] ___gxx_personality_v0
-# D:      0x[[#%x,PERSONALITY_1:]] [[#]] _personality_1
-# D:      0x[[#%x,PERSONALITY_2:]] [[#]] _personality_2
-# D:      0x[[#%x,PERSONALITY_3:]] [[#]] _personality_3
-# D:      0x[[#%x,PERSONALITY_4:]] [[#]] _personality_4
-# D:      0x[[#%x,GXX_PERSONALITY_LO:]] [[#]] ___gxx_personality_v0
+# D:      0x[[#%x,PERSONALITY_1:]]      LOCAL
+# D:      0x[[#%x,PERSONALITY_2:]]      LOCAL
+# D:      0x[[#%x,PERSONALITY_3:]]      LOCAL
+# D:      0x[[#%x,PERSONALITY_4:]]      LOCAL
+# D:      0x[[#%x,GXX_PERSONALITY_LO:]] LOCAL
 
 # D: Contents of __unwind_info section:
 # D:  Personality functions: (count = 1)
diff --git a/lld/test/MachO/compact-unwind.s b/lld/test/MachO/compact-unwind.s
index 27e4b44dc0b09..6516f7162e290 100644
--- a/lld/test/MachO/compact-unwind.s
+++ b/lld/test/MachO/compact-unwind.s
@@ -29,12 +29,12 @@
 # FIRST:      Indirect symbols for (__DATA_CONST,__got)
 # FIRST-NEXT: address                    index name
 # FIRST-DAG:  0x[[#%x,GXX_PERSONALITY:]] [[#]] ___gxx_personality_v0
-# FIRST-DAG:  0x[[#%x,MY_PERSONALITY:]]
+# FIRST-DAG:  0x[[#%x,MY_PERSONALITY:]] LOCAL
 
 # SECOND:      Indirect symbols for (__DATA_CONST,__got)
 # SECOND-NEXT: address                    index name
 # SECOND-DAG:  0x[[#%x,GXX_PERSONALITY:]] [[#]] ___gxx_personality_v0
-# SECOND-DAG:  0x[[#%x,MY_PERSONALITY:]]  [[#]] _my_personality
+# SECOND-DAG:  0x[[#%x,MY_PERSONALITY:]] LOCAL
 
 # CHECK:      SYMBOL TABLE:
 # CHECK-DAG:  [[#%x,MAIN:]]       g  F __TEXT,__text _main
diff --git a/lld/test/MachO/dead-strip.s b/lld/test/MachO/dead-strip.s
index f60305a4ec494..d4350154695ff 100644
--- a/lld/test/MachO/dead-strip.s
+++ b/lld/test/MachO/dead-strip.s
@@ -35,7 +35,7 @@
 # EXEC-DAG:   g {{.*}} __mh_execute_header
 # EXECDATA-LABEL: Indirect symbols
 # EXECDATA-NEXT:  name
-# EXECDATA-NEXT:  _ref_com
+# EXECDATA-NEXT:  LOCAL
 # EXECDATA-LABEL: Contents of (__DATA,__ref_section) section
 # EXECDATA-NEXT:   04 00 00 00 00 00 00 00 05 00 00 00 00 00 00 00
 # EXECDATA-LABEL: Exports trie:
diff --git a/lld/test/MachO/invalid/compact-unwind-personalities.s b/lld/test/MachO/invalid/compact-unwind-personalities.s
index 25ee4bd17929f..9cc474de83638 100644
--- a/lld/test/MachO/invalid/compact-unwind-personalities.s
+++ b/lld/test/MachO/invalid/compact-unwind-personalities.s
@@ -14,9 +14,9 @@
 # DWARF: Indirect symbols
 # DWARF: address            index name
 # DWARF: 0x[[#%x,GXX_PERSONALITY:]]     {{.*}} ___gxx_personality_v0
-# DWARF: 0x[[#%x,PERSONALITY_1:]]     {{.*}} _personality_1
-# DWARF: 0x[[#%x,PERSONALITY_2:]]     {{.*}} _personality_2
-# DWARF: 0x[[#%x,PERSONALITY_3:]]     {{.*}} _personality_3
+# DWARF: 0x[[#%x,PERSONALITY_1:]]       LOCAL
+# DWARF: 0x[[#%x,PERSONALITY_2:]]       LOCAL
+# DWARF: 0x[[#%x,PERSONALITY_3:]]       LOCAL
 # DWARF: .eh_frame contents:
 # DWARF: Personality Address: [[#%.16x,GXX_PERSONALITY]]
 # DWARF: Personality Address: [[#%.16x,PERSONALITY_1]]

Copy link
Contributor

@int3 int3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@nico nico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent commit description, thanks!

@BertalanD BertalanD merged commit b5c850f into llvm:main Jun 13, 2024
8 of 10 checks passed
BertalanD added a commit to BertalanD/llvm-project that referenced this pull request Jul 16, 2024
This reverts commit f55b79f.

The known issues with chained fixups have been addressed by llvm#98913,
llvm#98305, llvm#97156 and llvm#95171.

Compared to the original commit, support for xrOS (which postdates
chained fixups' introduction) was added and an unnecessary test change
was removed.

----------
Original commit message:

Enable chained fixups in lld when all platform and version criteria are
met. This is an attempt at simplifying the logic used in ld 907:

https://github.com/apple-oss-distributions/ld64/blob/93d74eafc37c0558b4ffb88a8bc15c17bed44a20/src/ld/Options.cpp#L5458-L5549

Some changes were made to simplify the logic:
- only enable chained fixups for macOS from 13.0 to avoid the arch check
- only enable chained fixups for iphonesimulator from 16.0 to avoid the
arch check
- don't enable chained fixups for not specifically listed platforms
- don't enable chained fixups for arm64_32
BertalanD added a commit that referenced this pull request Jul 22, 2024
This reverts commit f55b79f.

The known issues with chained fixups have been addressed by #98913,
#98305, #97156 and #95171.

Compared to the original commit, support for xrOS (which postdates
chained fixups' introduction) was added and an unnecessary test change
was removed.

----------
Original commit message:

Enable chained fixups in lld when all platform and version criteria are
met. This is an attempt at simplifying the logic used in ld 907:


https://github.com/apple-oss-distributions/ld64/blob/93d74eafc37c0558b4ffb88a8bc15c17bed44a20/src/ld/Options.cpp#L5458-L5549

Some changes were made to simplify the logic:
- only enable chained fixups for macOS from 13.0 to avoid the arch check
- only enable chained fixups for iphonesimulator from 16.0 to avoid the
arch check
- don't enable chained fixups for not specifically listed platforms
- don't enable chained fixups for arm64_32
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
Summary:
This reverts commit f55b79f.

The known issues with chained fixups have been addressed by #98913,
#98305, #97156 and #95171.

Compared to the original commit, support for xrOS (which postdates
chained fixups' introduction) was added and an unnecessary test change
was removed.

----------
Original commit message:

Enable chained fixups in lld when all platform and version criteria are
met. This is an attempt at simplifying the logic used in ld 907:


https://github.com/apple-oss-distributions/ld64/blob/93d74eafc37c0558b4ffb88a8bc15c17bed44a20/src/ld/Options.cpp#L5458-L5549

Some changes were made to simplify the logic:
- only enable chained fixups for macOS from 13.0 to avoid the arch check
- only enable chained fixups for iphonesimulator from 16.0 to avoid the
arch check
- don't enable chained fixups for not specifically listed platforms
- don't enable chained fixups for arm64_32

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60251071
EthanLuisMcDonough pushed a commit to EthanLuisMcDonough/llvm-project that referenced this pull request Aug 13, 2024
…L` (llvm#95171)

This expands on the fix in 4e572db. The issue is pretty similar: we
might put symbols in the GOT which don't need run-time binding, locally
defined personality symbols in this case. We should set their indirect
symbol table entries to `INDIRECT_SYMBOL_LOCAL` to help `strip` remove
these local names from the symbol table.

Checking if the symbol is private-extern doesn't cover all cases; it can
also be a non-weak extern function too, for instance; use the
`needsBinding()` helper to determine it. This was the case for the
personality function in statically linked Rust executables.

The extra non-`LOCAL` symbols triggered a bug in Apple's `strip`
implementation. As the indirect value for the personality function was
not set to the flag, but the symbol didn't require binding, it tried to
make the symbol local, overwriting the GOT entry with the function's
address in the process. This normally wouldn't be a problem, but if
chained fixups are used, the fixup also encodes the offset to the next
fixup, and it effectively zeroed this offset out, causing the remaining
relocations on the page to not be performed by dyld.

This caused the crash in https://issues.chromium.org/issues/325410295

The change in tests is a bit ugly, as a lot of symbol information is now
removed by turning more symbols `LOCAL`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants