[lld][MachO] Fix symbol insertion in `transplantSymbolsAtOffset` #120737

carlocab · 2024-12-20T14:12:44Z

The existing comparison does not insert symbols in the intended place.

Closes #120559.

llvmbot · 2024-12-20T14:13:17Z

@llvm/pr-subscribers-lld

Author: Carlo Cabrera (carlocab)

Changes

The existing comparison does not insert symbols in the intended place.

Unfortunately, it is not very clear how to test this. Suggestions
appreciated.

Closes #120559.

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

1 Files Affected:

(modified) lld/MachO/SymbolTable.cpp (+1-1)

diff --git a/lld/MachO/SymbolTable.cpp b/lld/MachO/SymbolTable.cpp
index f0a92da8777e13..36e421b52abcac 100644
--- a/lld/MachO/SymbolTable.cpp
+++ b/lld/MachO/SymbolTable.cpp
@@ -69,7 +69,7 @@ static void transplantSymbolsAtOffset(InputSection *fromIsec,
   // Ensure the symbols will still be in address order after our insertions.
   auto insertIt = llvm::upper_bound(toIsec->symbols, toOff,
                                     [](uint64_t off, const Symbol *s) {
-                                      return cast<Defined>(s)->value < off;
+                                      return cast<Defined>(s)->value > off;
                                     });
   llvm::erase_if(fromIsec->symbols, [&](Symbol *s) {
     auto *d = cast<Defined>(s);

llvmbot · 2024-12-20T14:13:18Z

@llvm/pr-subscribers-lld-macho

Author: Carlo Cabrera (carlocab)

Changes

The existing comparison does not insert symbols in the intended place.

Unfortunately, it is not very clear how to test this. Suggestions
appreciated.

Closes #120559.

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

1 Files Affected:

(modified) lld/MachO/SymbolTable.cpp (+1-1)

diff --git a/lld/MachO/SymbolTable.cpp b/lld/MachO/SymbolTable.cpp
index f0a92da8777e13..36e421b52abcac 100644
--- a/lld/MachO/SymbolTable.cpp
+++ b/lld/MachO/SymbolTable.cpp
@@ -69,7 +69,7 @@ static void transplantSymbolsAtOffset(InputSection *fromIsec,
   // Ensure the symbols will still be in address order after our insertions.
   auto insertIt = llvm::upper_bound(toIsec->symbols, toOff,
                                     [](uint64_t off, const Symbol *s) {
-                                      return cast<Defined>(s)->value < off;
+                                      return cast<Defined>(s)->value > off;
                                     });
   llvm::erase_if(fromIsec->symbols, [&](Symbol *s) {
     auto *d = cast<Defined>(s);

The existing comparison does not insert symbols in the intended place. Unfortunately, it is not very clear how to test this. Suggestions appreciated. Closes llvm#120559.

carlocab · 2024-12-20T14:15:14Z

These comments probably best describe the problem and why this is the correct fix: #120559 (comment), #120559 (comment)

alx32 · 2024-12-20T16:50:57Z

About adding a test for this - lld/test/MachO/alias-symbols.s already triggers the faulty insert. Currently all the offsets are 0's so the bug doesn't get reproduced.
I think we should be able to slightly tweak alias-symbols.s to insert a symbol with a non-zero offset and then add an assert(std::is_sorted) at the end of transplantSymbolsAtOffset and that will be our test. Have you looked into weather this is possible ?

carlocab · 2024-12-20T17:07:11Z

I hadn't looked at alias-symbols.s since that wasn't one of the failing tests flagged at #120559, but the approach you suggest sounds promising.

I'll give it a try, but would appreciate as many hints as possible. Not really familiar with how symbols are laid out in x86 assembly.

bjope · 2024-12-20T17:32:45Z

Nice idea with the assert. Adding
assert(llvm::is_sorted(toIsec->symbols) && "Symbols should still be sorted at exit.");
at end of transplantSymbolsAtOffset seem to result in assertion failure for the lld/test/MachO/symtab.s.

So that might be enough, considering that there already is a test case that would trigger such an assert.

bjope · 2024-12-20T17:35:44Z

Nice idea with the assert. Adding assert(llvm::is_sorted(toIsec->symbols) && "Symbols should still be sorted at exit."); at end of transplantSymbolsAtOffset seem to result in assertion failure for the lld/test/MachO/symtab.s.

So that might be enough, considering that there already is a test case that would trigger such an assert.

Ooops, was a bit fast. That is_sorted call needs to supply the comparison function.
Guess the problem is to understand which comparison function to provide given that the fault is in the comparison function :-)

carlocab · 2024-12-20T18:23:55Z

Guess the problem is to understand which comparison function to provide given that the fault is in the comparison function :-)

This comment notwithstanding, I added an assertion anyway since it seems like an improvement. Interestingly, the assertion I just added does not fire with Apple Clang/libc++ even if I drop the s/</>/ change from the first commit here.

alx32 · 2024-12-20T18:27:51Z

the assertion I just added does not fire with Apple Clang/libc++ even if I drop the s/</>/ change from the first commit here.

Does it fire with any other build config when run against the current test set ?
BTW the suggestion about modifying lld/test/MachO/alias-symbols.s was to have it trigger the assert with Clang/libc++ with the current test set.

carlocab · 2024-12-20T18:36:33Z

Does it fire with any other build config when run against the current test set ?

It should fire with GCC, except GCC crashes before the assert is reached.

BTW the suggestion about modifying lld/test/MachO/alias-symbols.s was to have it trigger the assert with Clang/libc++ with the current test set.

Yes, understood, but I'm still looking into the right way to modify alias-symbols.s. As mentioned above, tips appreciated!

alx32 · 2024-12-20T18:44:54Z

It should fire with GCC, except GCC crashes before the assert is reached.

GCC doesn't crash without -DLLVM_ENABLE_EXPENSIVE_CHECKS=ON - right ?
Does the assert fire with GCC without the -DLLVM_ENABLE_EXPENSIVE_CHECKS=ON flag ?

Yes, understood, but I'm still looking into the right way to modify alias-symbols.s. As mentioned #120737 (comment), tips appreciated!

Currently I think _weak_2 symbol is the one being inserted. I would either:

Modify the _weak_2 symbol to have a value of 1.
Add another _weak_3 that has the value of 1.

Given the tests have .subsections_via_symbols this will be a bit tricky - would need to play around with multiple options to get this to happen. It's not obvious right away what would work here.

carlocab · 2024-12-20T19:53:15Z

GCC doesn't crash without -DLLVM_ENABLE_EXPENSIVE_CHECKS=ON - right ?

Right.

Does the assert fire with GCC without the -DLLVM_ENABLE_EXPENSIVE_CHECKS=ON flag ?

No, weirdly.

However, it seems we can assert on std::is_partitioned instead. This fires with both Clang and GCC before fixing the comparison lambda.

Currently I think _weak_2 symbol is the one being inserted. I would either:

Modify the _weak_2 symbol to have a value of 1.

Add another _weak_3 that has the value of 1.

Given the tests have .subsections_via_symbols this will be a bit tricky - would need to play around with multiple options to get this to happen. It's not obvious right away what would work here.

Thanks; I'll give this a try after the weekend.

carlocab · 2024-12-21T13:56:59Z

lld/MachO/SymbolTable.cpp

+  assert(llvm::is_sorted(toIsec->symbols,
+                         [](const Defined *s, const Defined *t) {
+                           return s->value < t->value;
+                         }) &&
+         "Symbols should still be sorted at exit.");


I'll probably get rid of this one, since I can't get it to fire. My theory is that the failure of this assertion can only be a consequence of UB, so the compiler just optimises this away. (No real evidence to support this though, so could be wrong!)

Does is_partitioned trigger without expensive checks ? On which tests does it trigger ?

I've verified that is_partitioned triggers with expensive checks with Clang, and without expensive checks with GCC.

The failing tests (for both) are

Failed Tests (3): lld :: MachO/local-alias-to-weak.s lld :: MachO/symtab.s lld :: MachO/weak-definition-gc.s

I haven't yet verified the assertion failure for Clang without expensive checks. The build is running now; I'll report back soon.

Verified with Clang without expensive checks and the comparison in symSucceedsOff reversed:

Failed Tests (3): lld :: MachO/local-alias-to-weak.s lld :: MachO/symtab.s lld :: MachO/weak-definition-gc.s

To be sure, all errors are of the form

Assertion failed: (std::is_partitioned(toIsec->symbols.begin(), toIsec->symbols.end(), [toOff, symSucceedsOff](const Symbol *s) { return !symSucceedsOff(toOff, s); }) && "Symbols in toIsec must be partitioned by toOff."), function transplantSymbolsAtOffset, file SymbolTable.cpp, line 78.

Great, thanks for checking this.

This should help catch problems with symbol insertion.

carlocab · 2024-12-21T15:50:43Z

Tried this:

diff --git a/lld/MachO/SymbolTable.cpp b/lld/MachO/SymbolTable.cpp
index a61e60a944fb..4b6e462f6bdd 100644
--- a/lld/MachO/SymbolTable.cpp
+++ b/lld/MachO/SymbolTable.cpp
@@ -68,7 +68,7 @@ static void transplantSymbolsAtOffset(InputSection *fromIsec,
                                       uint64_t fromOff, uint64_t toOff) {
   // Ensure the symbols will still be in address order after our insertions.
   auto symSucceedsOff = [](uint64_t off, const Symbol *s) {
-    return cast<Defined>(s)->value > off;
+    return cast<Defined>(s)->value < off;
   };
   assert(std::is_partitioned(toIsec->symbols.begin(), toIsec->symbols.end(),
                              [symSucceedsOff, toOff](const Symbol *s) {
@@ -96,6 +96,11 @@ static void transplantSymbolsAtOffset(InputSection *fromIsec,
     }
     return true;
   });
+  assert(llvm::is_sorted(toIsec->symbols,
+                         [](const Defined *s, const Defined *t) {
+                           return s->value < t->value;
+                         }) &&
+         "Symbols should still be sorted at exit.");
 }
 
 Defined *SymbolTable::addDefined(StringRef name, InputFile *file,
diff --git a/lld/test/MachO/alias-symbols.s b/lld/test/MachO/alias-symbols.s
index 44a388c35722..231ada5aa4c8 100644
--- a/lld/test/MachO/alias-symbols.s
+++ b/lld/test/MachO/alias-symbols.s
@@ -124,7 +124,9 @@ _strong:
 _weak_1:
   .space 1
 _weak_2:
-  .space 1
+  .long 1
+_weak_3:
+  .long 1
 _dead:
   .space 1
 _pext:

Still no failure from lld/test/MachO/alias-symbols.s. Given that the is_partitioned assertion isn't even firing here, I wonder if trying to modify alias-symbols.s is going to be a lot more complicated than working with the other tests that are failing.

alx32

LGTM

carlocab · 2024-12-22T13:41:02Z

To be sure that we're not in a "works on my machine" situation, I pushed 0db5264e5918221edec3dacd2e8c0c6328769517 to ensure that the is_partitioned assertion triggers in CI. Sure enough, it does:

https://buildkite.com/llvm-project/github-pull-requests/builds/131531#0193ee8b-b917-40e0-a963-6ac1f5e01685
https://buildkite.com/llvm-project/github-pull-requests/builds/131531#0193ee8b-b918-4754-9f43-fc24ea57463b

Going to revert that commit then merge.

carlocab requested review from alx32, BertalanD, bjope, ellishg, int3, karka228 and nico December 20, 2024 14:12

llvmbot added lld lld:MachO labels Dec 20, 2024

[lld][MachO] Fix symbol insertion in transplantSymbolsAtOffset

7353c9b

The existing comparison does not insert symbols in the intended place. Unfortunately, it is not very clear how to test this. Suggestions appreciated. Closes llvm#120559.

carlocab force-pushed the fix-transplantSymbolsAtOffset branch from 627f54b to 7353c9b Compare December 20, 2024 14:14

carlocab changed the title ~~[lld][MachO] Fix symbol inseration in transplantSymbolsAtOffset~~ [lld][MachO] Fix symbol insertion in transplantSymbolsAtOffset Dec 20, 2024

carlocab force-pushed the fix-transplantSymbolsAtOffset branch 2 times, most recently from cbadcfb to b99bbc0 Compare December 20, 2024 19:55

carlocab commented Dec 21, 2024

View reviewed changes

Add assert to transplantSymbolsAtOffset

e77e289

This should help catch problems with symbol insertion.

carlocab force-pushed the fix-transplantSymbolsAtOffset branch from b99bbc0 to e77e289 Compare December 21, 2024 15:35

alx32 approved these changes Dec 22, 2024

View reviewed changes

carlocab marked this pull request as draft December 22, 2024 13:25

carlocab force-pushed the fix-transplantSymbolsAtOffset branch from 0db5264 to e77e289 Compare December 22, 2024 13:42

carlocab marked this pull request as ready for review December 22, 2024 13:42

carlocab merged commit a0f0a69 into llvm:main Dec 22, 2024
13 checks passed

carlocab deleted the fix-transplantSymbolsAtOffset branch December 22, 2024 13:50

[lld][MachO] Fix symbol insertion in transplantSymbolsAtOffset #120737

[lld][MachO] Fix symbol insertion in transplantSymbolsAtOffset #120737

Uh oh!

Conversation

carlocab commented Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Dec 20, 2024

Uh oh!

llvmbot commented Dec 20, 2024

Uh oh!

carlocab commented Dec 20, 2024

Uh oh!

alx32 commented Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlocab commented Dec 20, 2024

Uh oh!

bjope commented Dec 20, 2024

Uh oh!

bjope commented Dec 20, 2024

Uh oh!

carlocab commented Dec 20, 2024

Uh oh!

alx32 commented Dec 20, 2024

Uh oh!

carlocab commented Dec 20, 2024

Uh oh!

alx32 commented Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlocab commented Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlocab Dec 21, 2024

Choose a reason for hiding this comment

Uh oh!

alx32 Dec 21, 2024

Choose a reason for hiding this comment

Uh oh!

carlocab Dec 21, 2024

Choose a reason for hiding this comment

Uh oh!

carlocab Dec 21, 2024

Choose a reason for hiding this comment

Uh oh!

alx32 Dec 22, 2024

Choose a reason for hiding this comment

Uh oh!

carlocab commented Dec 21, 2024

Uh oh!

alx32 left a comment

Choose a reason for hiding this comment

Uh oh!

carlocab commented Dec 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[lld][MachO] Fix symbol insertion in `transplantSymbolsAtOffset` #120737

[lld][MachO] Fix symbol insertion in `transplantSymbolsAtOffset` #120737

carlocab commented Dec 20, 2024 •

edited

Loading

alx32 commented Dec 20, 2024 •

edited

Loading

alx32 commented Dec 20, 2024 •

edited

Loading

carlocab commented Dec 20, 2024 •

edited

Loading

carlocab commented Dec 22, 2024 •

edited

Loading