This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Don't warn on non-existent system libraries
ClosedPublic

Authored by thakis on Nov 20 2020, 1:07 PM.

Details

Summary

Now, new mach-o lld no longer warns if the isysroot has just
usr/lib and System/Library/Frameworks but is missing usr/local/lib
and System/Frameworks.

This matches ld64 and old mach-o lld and fixes a regression from D85992.

It also fixes the only test failure in check-lld when running it
on an M1 Mac.

Diff Detail

Event Timeline

thakis requested review of this revision.Nov 20 2020, 1:07 PM
thakis created this revision.
thakis added inline comments.Nov 20 2020, 4:53 PM
lld/test/MachO/syslibroot.test
31

This patch is also fixing this test ffailing on one of my macs, in a fairly normal setup (commandline tools installed, xcode not installed, m1).

smeenai added inline comments.Nov 20 2020, 5:25 PM
lld/test/MachO/syslibroot.test
31

I don't quite understand this. A final syslibroot of just / is supposed to ignore any previously specified roots (a hack which we inherit from ld64), and I believe that's what this test was for. I don't see how your code change affects this test?

thakis added inline comments.Nov 20 2020, 5:39 PM
lld/test/MachO/syslibroot.test
31

Without my patch, the output of the command under test is (on my linux box):

$ out/gn/bin/lld -flavor darwinnew -v -syslibroot /var/empty -syslibroot /
lld: warning: directory not found for option -F/Library/Frameworks
lld: warning: directory not found for option -F/System/Library/Frameworks
LLD 12.0.0
Library search paths:
	/usr/lib
	/usr/local/lib
Framework search paths:

After my change, it is just:

$ out/gn/bin/lld -flavor darwinnew -v -syslibroot /var/empty -syslibroot /
LLD 12.0.0
Library search paths:
	/usr/lib
	/usr/local/lib
Framework search paths:

Previously, the "/usr/local/lib" in the CHECK-SYSLIBROOT-IGNORED line matched against the "warning: directory not found" line, but now that the warning is gone the test fails instead. I don't know if it was supposed to match against that warning line (the "mach here is fuzzy" comment isn't quite clear to me), but after this change it can't match the warning line any longer.

thakis added inline comments.Nov 20 2020, 5:43 PM
lld/test/MachO/syslibroot.test
31

(Sorry, this output was for the other test further. But I'm guessing the test here currently passes on Windows for the same reason -- and it fails on my m1 mac because it has /usr/lib but not /usr/local/lib so it currently emits a warning for /usr/local/lib and then prints /usr/lib as search path, and that means /usr/local/lib and /usr/lib are not in the order the CHECK lines here currently expect. For the M1, this is indeed independent of my patch – the test fails at head, without the change.)

int3 added a subscriber: compnerd.

@compnerd, want to have a look at this since you wrote the original test?

thakis edited the summary of this revision. (Show Details)Nov 30 2020, 6:31 AM
compnerd requested changes to this revision.Nov 30 2020, 11:03 AM
compnerd added inline comments.
lld/test/MachO/syslibroot.test
31

The match there was to prevent the accidental match. I think that we should retain the test, but remove the match against the warning. We should verify that we ignore the non-default-search paths here.

This revision now requires changes to proceed.Nov 30 2020, 11:03 AM
thakis updated this revision to Diff 308446.Nov 30 2020, 12:05 PM

update test as discussed over chat

thakis marked an inline comment as done.Nov 30 2020, 12:07 PM
compnerd accepted this revision.Nov 30 2020, 12:53 PM
This revision is now accepted and ready to land.Nov 30 2020, 12:53 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptNov 30 2020, 1:07 PM