This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Only mock out libSystem.dylib loading in tests
AbandonedPublic

Authored by int3 on Mar 30 2020, 1:33 PM.

Details

Summary

Now that we can read universal binaries (D77006), we can properly link against libSystem, so we don't need to mock out that behavior... except in tests, which may run on non-OS X systems.

Diff Detail

Event Timeline

int3 created this revision.Mar 30 2020, 1:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2020, 1:33 PM
int3 marked 2 inline comments as done.Mar 30 2020, 1:35 PM
int3 added inline comments.
lld/MachO/Writer.cpp
376–383

Instead of checking for MH_EXECUTABLE, ld64 calls a function called needsEntryPointLoadCommand(). Not sure what else that encompasses

lld/test/MachO/lit.local.cfg
1 ↗(On Diff #253685)

I debated between using a flag and an environment variable, but since we already have the LLD_IN_TEST env var, another one seemed appropriate

ruiu added a comment.Mar 31 2020, 1:56 AM

I'm not sure why we need this. Could you elaborate a bit? If all executables need to be linked against /usr/lib/libSystem.B.dylib and if a user didn't pass -lSystem, isn't it still their fault and not ours, no?

int3 added a comment.Mar 31 2020, 7:48 PM

Hm, I was just trying to emulate ld64's behavior, but I agree that this one might not be very useful. Regardless of whether we throw an error, though, having special-case code to emit it at test time seems useful, if only so we can run the resulting executables on macOS.

ruiu added a comment.Mar 31 2020, 8:08 PM

Hm, I was just trying to emulate ld64's behavior, but I agree that this one might not be very useful. Regardless of whether we throw an error, though, having special-case code to emit it at test time seems useful, if only so we can run the resulting executables on macOS.

That might be mildly useful but at the same time this might be perceived as an unnecessary arbitrary limitation if a user is doing something unusual. And as far as I know cross-building a macOS executable is somewhat unusual. :) So I'd pass this until it becomes clear that we need this.

int3 added a comment.Mar 31 2020, 8:48 PM

I wasn't talking about cross-building; I just thought upstream had an expectation that the test suite would run on non-macOS platforms as well. If the tests are to pass on other platforms, then we can't pass -lSystem.B when invoking lld, which means that the resulting binaries in the Outputs folder wouldn't be executable on macOS -- and being able to execute them locally has been really useful for checking my assumptions.

int3 added a comment.Mar 31 2020, 8:50 PM

Maybe I misunderstood though. Are you saying that the error is not useful, but the special-case code for testing is fine?

ruiu added a comment.Mar 31 2020, 8:58 PM

What do you mean by "upstream"?

int3 added a comment.EditedMar 31 2020, 11:30 PM

I meant the people who determine what goes into the LLVM repo, like you I guess :)

On the first dylib linking diff, we had a long discussion about whether we should generate dylib test inputs using yaml2obj or if we should just check in binaries directly. I was under the impression that we were going through that trouble because we wanted the tests to run on all platforms, so using ld64 wasn't an option.

ruiu added a comment.Apr 1 2020, 1:15 AM

The tests for lld/mach-o definitely need to be able to run on non-macOS platforms. We generally run all tests on all kind of bots, so yes, you cannot assume that /usr/lib/libSystem.B.dylib exists. Actually you shouldn't make any assumptions on the existence or absence of any external file.

smeenai added inline comments.Apr 8 2020, 9:47 PM
lld/test/MachO/no-libsystem.s
8–10 ↗(On Diff #253758)

Hah, that seems pretty broken. dyld seems to be happy enough to load an executable that's not linking to libSystem either, though I haven't tested that on Catalina yet.

In this case, I'm not sure how much value this check (or the force link against libSystem) adds, tbh. The vast majority of people will be invoking the linker via the clang driver, which will add libSystem automatically. For the few people invoking the linker directly, if they reference libSystem functions and forget to link it, they'll get pretty obvious undefined symbol errors. This error wouldn't fire as long as they were linking any other dylib anyway, so it doesn't seem to add much value. What do you think?

ruiu added inline comments.Apr 8 2020, 11:12 PM
lld/test/MachO/no-libsystem.s
8–10 ↗(On Diff #253758)

I'll second that. To be honest, this "feature" seems to be a minor detail of ld64 that we probably don't want to focus on at the moment. Once we get lld mach-o working and if users start complaining this feature is missing, we can add that, but my bet is that they wouldn't even notice about the absence of this thing.

int3 marked an inline comment as done.Apr 9 2020, 4:18 PM
int3 added inline comments.
lld/test/MachO/no-libsystem.s
8–10 ↗(On Diff #253758)

Okay, I'm removing the check, but the LLD_FORCE_LOAD_LIBSYSTEM environment variable still seems useful for unit testing so that we can execute the resulting binaries during local development.

int3 updated this revision to Diff 256444.Apr 9 2020, 4:33 PM
int3 retitled this revision from [lld-macho] Require executables to link against libSystem.dylib to [lld-macho] Only mock out libSystem.dylib loading in tests.

remove error check

ruiu added inline comments.Apr 12 2020, 8:25 PM
lld/MachO/Writer.cpp
379–383

I may be missing something, but doesn't this force lld to always link against /usr/lib/libSystem.B.dylib when it is ran in test, which will fail on non-macOS systems that don't have the dylib file?

int3 marked an inline comment as done.Apr 12 2020, 10:46 PM
int3 added inline comments.
lld/MachO/Writer.cpp
379–383

This just adds an additional load command to the output file, but it doesn't actually cause lld to read and link against libSystem. I.e. we won't create a DylibFile / InputFile for libSystem. It's just an output-time hack.

ruiu added inline comments.Apr 12 2020, 11:10 PM
lld/MachO/Writer.cpp
379–383

Ah, I see. So by adding that library to the dependency list, you can execute an output of lld test run right?

LLD_IN_TEST is set when you are running a test. Can you use that existing environment variable instead? I'm a bit worried about defining a new environment variable as users could start using the internal flag.

MaskRay accepted this revision.Apr 12 2020, 11:24 PM

Looks great!

This revision is now accepted and ready to land.Apr 12 2020, 11:24 PM
int3 updated this revision to Diff 256945.Apr 13 2020, 1:59 AM

use LLD_IN_TEST

int3 marked 2 inline comments as done.Apr 13 2020, 2:00 AM
int3 added inline comments.
lld/MachO/Writer.cpp
379–383

So by adding that library to the dependency list, you can execute an output of lld test run right?

Yup, that's right

int3 marked an inline comment as done.Apr 15 2020, 9:50 PM

Looks like we need to do a more elaborate mock of libSystem in our tests in order to reference the dyld_stub_binder symbol. D78270: [lld-macho] Support calls to functions in dylibs changes / obsoletes the approach here