This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Support -dyld_env
ClosedPublic

Authored by oontvoo on Sep 16 2022, 10:40 AM.

Details

Reviewers
ributzka
int3
Group Reviewers
Restricted Project
Commits
rG016c2f5e3233: [lld-macho] Support -dyld_env
Summary

This arg is undocumented but from looking at the code + experiment, it's used to add additional DYLD_ENVIRONMENT load commands to the output.

Diff Detail

Event Timeline

oontvoo created this revision.Sep 16 2022, 10:40 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
oontvoo requested review of this revision.Sep 16 2022, 10:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2022, 10:40 AM
thakis added a subscriber: thakis.Sep 16 2022, 12:27 PM
thakis added inline comments.
lld/MachO/Driver.cpp
1414

This looks involved enough that it could go into a parse...() helper in DriverUtils.

1418

Do we have to save() this? We don't change it, do we?

1419

ld64 doesn't seem to check for this (?)

lld/MachO/Writer.cpp
420

s/rpath_command/dyld_env_command/ probably?

849

no need for the if, the loop does 0 iterations if dyldEnvs is empty

lld/test/MachO/dyld-env.s
2

why does this need shell?

11

check that this errors when making dylibs

(also, test fails on CI)

oontvoo updated this revision to Diff 460847.Sep 16 2022, 12:28 PM

required x86-64 (instead of just x86) because the cmdsize is platform dependent

BertalanD added inline comments.
lld/MachO/Driver.cpp
1413

This will ignore all but the last -dyld_env argument. Does ld64 let you specify it more than once?

int3 added a subscriber: int3.Sep 16 2022, 1:06 PM
int3 added inline comments.
lld/test/MachO/dyld-env.s
2

not sure how platform-dependence factors in here. shouldn't it be enough that we are specifying -arch x86_64?

thevinster added inline comments.
lld/MachO/Driver.cpp
1417

Should something be validated here? Is it limited to a list of known possible values that could be enumerated or is it valid to have DYLD_FOO_PATH=foo embedded in the binary?

lld/test/MachO/dyld-env.s
2

I believe shell is needed to run the symlinks. Alternatively, we could make this test not be supported on windows.

oontvoo updated this revision to Diff 461273.Sep 19 2022, 10:57 AM
oontvoo marked 10 inline comments as done.

addressed review comments:

  • relax the value validations
  • added supports for specifying -dyld_env multiple times
  • more tests
oontvoo marked an inline comment as done.Sep 19 2022, 10:58 AM
oontvoo added inline comments.
lld/MachO/Driver.cpp
1413

Yes - added support for that. Thanks!

1417

relaxed the validations (per thakis's comment below), so I guess we don't even need to check for this

lld/test/MachO/dyld-env.s
2

no, it was platform-dependence because of the test path (which is part of the DYLD_FRAMEWORK_PATH value. I've changed the tests to use relative paths instead.

2

Yes - it was needed because of ln, but I guess we dont really need it in this tests. (ie., the paths don't need to exist...)
Removed all the shell stuff.

int3 accepted this revision.Sep 19 2022, 11:10 AM

lgtm

lld/MachO/Driver.cpp
1414–1415

total nit but I think it's clearer this way. I don't actually know off the top of my head what strchr returns in error cases (though I can certainly infer based on how it's used here)

1416

grammar nit: malformed -dyld_env value or -dyld_env's argument is malformed

1418

if we construct envPair as a StringRef then I don't think we'll need the Twine() here

lld/MachO/Options.td
1215–1221

can we place this along with the rest of the grp_rare options? No need for the leading/trailing newlines too

This revision is now accepted and ready to land.Sep 19 2022, 11:10 AM
thakis added inline comments.Sep 19 2022, 12:33 PM
lld/MachO/Writer.cpp
849

(not done?)

oontvoo updated this revision to Diff 461335.Sep 19 2022, 1:18 PM
oontvoo marked 4 inline comments as done.

addressed review comments

lld/MachO/Writer.cpp
849

Done! (for real)

int3 added inline comments.Sep 19 2022, 4:15 PM
lld/MachO/Options.td
1215–1220

looks like trailing spaces

oontvoo updated this revision to Diff 461545.Sep 20 2022, 5:45 AM
oontvoo marked an inline comment as done.

removed trailing spaces

The windows CI failures look unrelated, so I'm gonna go ahead and commit this. Thanks, all, for the reviews!

This revision was landed with ongoing or failed builds.Sep 20 2022, 7:17 AM
This revision was automatically updated to reflect the committed changes.