[lld-macho] Find objects in library search path #78628

Un1q32 · 2024-01-18T21:25:21Z

Find object files in library search path just like Apple's linker, this makes building with some older MacOS SDKs easier since clang runs with -lcrt1.10.6.o

github-actions · 2024-01-18T21:25:41Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-01-18T21:26:12Z

@llvm/pr-subscribers-lld

Author: None (OldWorldOrdr)

Changes

Find object files in library search path just like Apple's linker, this makes building with some older MacOS SDKs easier since clang runs with -lcrt1.10.6.o

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

1 Files Affected:

(modified) lld/MachO/Driver.cpp (+9-4)

diff --git a/lld/MachO/Driver.cpp b/lld/MachO/Driver.cpp
index 401459a054394ec..f04165f5c026153 100644
--- a/lld/MachO/Driver.cpp
+++ b/lld/MachO/Driver.cpp
@@ -95,11 +95,16 @@ static std::optional<StringRef> findLibrary(StringRef name) {
               findPathCombination("lib" + name, config->librarySearchPaths,
                                   {".tbd", ".dylib", ".so"}))
         return path;
-      return findPathCombination("lib" + name, config->librarySearchPaths,
-                                 {".a"});
+      else if (std::optional<StringRef> path = findPathCombination(
+                   "lib" + name, config->librarySearchPaths, {".a"}))
+        return path;
+      return findPathCombination(name, config->librarySearchPaths, {""});
     }
-    return findPathCombination("lib" + name, config->librarySearchPaths,
-                               {".tbd", ".dylib", ".so", ".a"});
+    if (std::optional<StringRef> path =
+            findPathCombination("lib" + name, config->librarySearchPaths,
+                                {".tbd", ".dylib", ".so", ".a"}))
+      return path;
+    return findPathCombination(name, config->librarySearchPaths, {""});
   };
 
   std::optional<StringRef> path = doFind();

llvmbot · 2024-01-18T21:26:12Z

@llvm/pr-subscribers-lld-macho

Author: None (OldWorldOrdr)

Changes

Find object files in library search path just like Apple's linker, this makes building with some older MacOS SDKs easier since clang runs with -lcrt1.10.6.o

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

1 Files Affected:

(modified) lld/MachO/Driver.cpp (+9-4)

diff --git a/lld/MachO/Driver.cpp b/lld/MachO/Driver.cpp
index 401459a054394e..f04165f5c02615 100644
--- a/lld/MachO/Driver.cpp
+++ b/lld/MachO/Driver.cpp
@@ -95,11 +95,16 @@ static std::optional<StringRef> findLibrary(StringRef name) {
               findPathCombination("lib" + name, config->librarySearchPaths,
                                   {".tbd", ".dylib", ".so"}))
         return path;
-      return findPathCombination("lib" + name, config->librarySearchPaths,
-                                 {".a"});
+      else if (std::optional<StringRef> path = findPathCombination(
+                   "lib" + name, config->librarySearchPaths, {".a"}))
+        return path;
+      return findPathCombination(name, config->librarySearchPaths, {""});
     }
-    return findPathCombination("lib" + name, config->librarySearchPaths,
-                               {".tbd", ".dylib", ".so", ".a"});
+    if (std::optional<StringRef> path =
+            findPathCombination("lib" + name, config->librarySearchPaths,
+                                {".tbd", ".dylib", ".so", ".a"}))
+      return path;
+    return findPathCombination(name, config->librarySearchPaths, {""});
   };
 
   std::optional<StringRef> path = doFind();

smeenai

Thanks :)

This needs a test case. You'll find lots of existing test cases under lld/test/MachO which should be good templates for this (and there's likely an existing test case you can extend to cover your new logic).

smeenai · 2024-01-19T00:33:27Z

lld/MachO/Driver.cpp

+      else if (std::optional<StringRef> path = findPathCombination(
+                   "lib" + name, config->librarySearchPaths, {".a"}))
+        return path;
+      return findPathCombination(name, config->librarySearchPaths, {""});


How come this needs to be two separate calls to findPathCombination, instead of keeping one call and making the argument {".a", ""})? IIUC, the difference will be in a scenario where your search path has /foo and /bar, the files /foo/crt1.o and /bar/crt1.o.a exist, and you do -lcrt1.o. Your current logic will find /bar/crt1.o.a, whereas a single call would find /foo/crt1.o. What does ld64 do? If you do need the separate calls to match ld64's behavior then it's worth adding a comment to explain that.

are you suggesting the code be

return findPathCombination("lib" + name, config->librarySearchPaths, {".a", ""});

Won't that look for libcrt1.o instead of crt1.o?

You're right; that was a think-o on my part :) That being said, the scenario in my first comment (except with /bar/libcrt1.o.a) still applies; it's an edge case, but if we're matching ld64 behavior, we might as well match it as closely as reasonable.

a quick test with both libcrt1.o.a and crt1.o shows ld64 prefers crt1.o

Hmm. In that case, we probably want to instead change findPathCombination to take a list of prefixes, so that we can match ld64's search behavior?

removing the object and trying again gives an error, ld64 wont link with the .a file at all

Huh, that's interesting. I wonder if they have some special-cased logic around these specific extensions, or if they just won't append the .a if there's already any other extension present...

Looks like ld64 checks if the library name ends in .o and if so, assumes it's a Csu support file, and looks for a file in the library search path with that exact name, not searching for any libcrt1.o.a.

If a Csu support file is not found ld64 automatically changes the target to MacOS 10.8 which stopped requiring Csu support files, then continues.

Apples new linker does the same, but it has no concept of Csu support files and just links with it like it were a normal file, exiting with an error if it's not found.

Un1q32 · 2024-01-19T02:39:01Z

Patch reworked to more closely replicate ld64's behavior.

smeenai · 2024-01-19T02:44:26Z

Thanks, the code looks good to me. Could you add a short comment explaining why we're special casing the .o extension, and also add a test case?

Un1q32 · 2024-01-19T06:34:52Z

I've added a basic comment and a test case, I've never written a test with lit before so sorry if its terrible and broken

oontvoo · 2024-01-19T14:14:55Z

lld/test/MachO/link-csu-obj.s

+# RUN: mkdir -p %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %p/Inputs/libhello.s -o %t/hello.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %s -o %t/main.o
+# RUN: %lld -L %t %t/main.o %t/hello.o -o %t/a.out


Should there be some basic verification of the output after the link?

oontvoo · 2024-01-19T14:16:14Z

lld/MachO/Driver.cpp

@@ -90,6 +90,9 @@ static std::optional<StringRef> findLibrary(StringRef name) {
    return entry->second;

  auto doFind = [&] {
+    // Special case for Csu support files.


nit: would be helpful to expand what Csu files are ... (not a very common thing - I had to look it up :) )

Thanks!

Is the new comment ok?

github-actions · 2024-01-19T16:40:46Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Un1q32 · 2024-01-19T17:37:28Z

Updated the test to verify the output

oontvoo

LGTM. Thanks!

thevinster · 2024-01-20T19:58:02Z

Do you have write permissions? Or, were you waiting for someone to help you merge this PR?

Un1q32 · 2024-01-20T20:39:46Z

I'm just some guy I don't have write perms, this is my first time contributing to llvm

smeenai · 2024-01-21T01:03:18Z

lld/test/MachO/link-csu-object.s

+# RUN: mkdir -p %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %p/Inputs/libhello.s -o %t/hello.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %s -o %t/main.o
+# RUN: %lld -L %t %t/main.o %t/hello.o -o %t/a.out


Should one of these be linked with -l instead of by path?

I fixed it in my branch but the pr is already merged

would you mind putting it in another PR? we can get it merged there. Thanks!

would you mind putting it in another PR? we can get it merged there. Thanks!

done #79018

Mistake with #78628 that got caught after being merged

[lld-macho] Find objects in library search path

e73fc2d

llvmbot added lld lld:MachO labels Jan 18, 2024

Merge branch 'llvm:main' into pr1

e0ee69f

smeenai reviewed Jan 19, 2024

View reviewed changes

Un1q32 added 2 commits January 19, 2024 01:25

[lld-macho] find objects in library search path version 2

5b29c5d

add test case

5aab87e

oontvoo reviewed Jan 19, 2024

View reviewed changes

Un1q32 added 2 commits January 19, 2024 12:29

Improve comment

6e8b321

improve test

9850660

oontvoo approved these changes Jan 20, 2024

View reviewed changes

Merge branch 'main' into pr1

951dcee

thevinster merged commit 46a9135 into llvm:main Jan 20, 2024
3 of 4 checks passed

smeenai reviewed Jan 21, 2024

View reviewed changes

Un1q32 mentioned this pull request Jan 22, 2024

fix test #79018

Merged

oontvoo pushed a commit that referenced this pull request Jan 23, 2024

fix test (#79018)

654131f

Mistake with #78628 that got caught after being merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lld-macho] Find objects in library search path #78628

[lld-macho] Find objects in library search path #78628

Un1q32 commented Jan 18, 2024

github-actions bot commented Jan 18, 2024

llvmbot commented Jan 18, 2024

llvmbot commented Jan 18, 2024

smeenai left a comment

smeenai Jan 19, 2024

Un1q32 Jan 19, 2024

smeenai Jan 19, 2024

Un1q32 Jan 19, 2024

smeenai Jan 19, 2024

Un1q32 Jan 19, 2024

smeenai Jan 19, 2024

Un1q32 Jan 19, 2024 •

edited

Un1q32 commented Jan 19, 2024

smeenai commented Jan 19, 2024

Un1q32 commented Jan 19, 2024

oontvoo Jan 19, 2024

oontvoo Jan 19, 2024 •

edited

Un1q32 Jan 19, 2024

github-actions bot commented Jan 19, 2024 •

edited

Un1q32 commented Jan 19, 2024

oontvoo left a comment

thevinster commented Jan 20, 2024 •

edited

Un1q32 commented Jan 20, 2024

smeenai Jan 21, 2024

Un1q32 Jan 21, 2024

Un1q32 Jan 21, 2024

oontvoo Jan 22, 2024

Un1q32 Jan 22, 2024

[lld-macho] Find objects in library search path #78628

[lld-macho] Find objects in library search path #78628

Conversation

Un1q32 commented Jan 18, 2024

github-actions bot commented Jan 18, 2024

llvmbot commented Jan 18, 2024

llvmbot commented Jan 18, 2024

smeenai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Un1q32 Jan 19, 2024 • edited

Choose a reason for hiding this comment

Un1q32 commented Jan 19, 2024

smeenai commented Jan 19, 2024

Un1q32 commented Jan 19, 2024

Choose a reason for hiding this comment

oontvoo Jan 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 19, 2024 • edited

Un1q32 commented Jan 19, 2024

oontvoo left a comment

Choose a reason for hiding this comment

thevinster commented Jan 20, 2024 • edited

Un1q32 commented Jan 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Un1q32 Jan 19, 2024 •

edited

oontvoo Jan 19, 2024 •

edited

github-actions bot commented Jan 19, 2024 •

edited

thevinster commented Jan 20, 2024 •

edited