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

[int3]

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.

[int3]

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

[int3]

rebase

[ruiu]

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]

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]

In D77087#1953957, @int3 wrote:

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]

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]

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

[ruiu]

What do you mean by "upstream"?

[int3]

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]

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]

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]

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]

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]

remove error check

[int3]

update

[ruiu]

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]

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]

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]

Looks great!

[int3]

use LLD_IN_TEST

[int3]

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]

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

[int3]

rebase

[int3]

rebase

lld/MachO/Writer.cpp

void Writer::createLoadCommands() {
   uint64_t dylibOrdinal = 1;
   for (InputFile *file : inputFiles) {
     if (auto *dylibFile = dyn_cast<DylibFile>(file)) {
       headerSection->addLoadCommand(make<LCLoadDylib>(dylibFile->dylibName));
       dylibFile->ordinal = dylibOrdinal++;
     }
   }
 
-  // TODO: dyld requires libSystem to be loaded. libSystem is a universal
-  // binary and we don't have support for that yet, so mock it out here.
+  // dyld requires us to load libSystem. This environment variable is used in
+  // tests, as we may also run those tests on non-OSX systems which do not
+  // have libSystem.
+  if (StringRef(getenv("LLD_IN_TEST")) == "1" && dylibOrdinal == 1 &&
+      config->outputType == MH_EXECUTE) {
     headerSection->addLoadCommand(
         make<LCLoadDylib>("/usr/lib/libSystem.B.dylib"));
   }
+}
 
 void Writer::createHiddenSections() {
   headerSection = createInputSection<MachHeaderSection>();
   bindingSection = createInputSection<BindingSection>();
   stringPoolSection = createInputSection<StringPoolSection>();
   symtabSection = createInputSection<SymtabSection>(*stringPoolSection);
   exportSection = createInputSection<ExportSection>();