This arg is undocumented but from looking at the code + experiment, it's used to add additional DYLD_ENVIRONMENT load commands to the output.
Details
- Reviewers
ributzka int3 - Group Reviewers
Restricted Project - Commits
- rG016c2f5e3233: [lld-macho] Support -dyld_env
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Time | Test | |
---|---|---|
200 ms | x64 debian > lld.MachO::dyld-env.s |
Event Timeline
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 | ||
1 | why does this need shell? | |
10 | check that this errors when making dylibs |
lld/MachO/Driver.cpp | ||
---|---|---|
1413 | This will ignore all but the last -dyld_env argument. Does ld64 let you specify it more than once? |
lld/test/MachO/dyld-env.s | ||
---|---|---|
3 | not sure how platform-dependence factors in here. shouldn't it be enough that we are specifying -arch x86_64? |
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 | ||
1 | I believe shell is needed to run the symlinks. Alternatively, we could make this test not be supported on windows. |
addressed review comments:
- relax the value validations
- added supports for specifying -dyld_env multiple times
- more tests
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 | ||
1 | 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...) | |
3 | 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. |
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 |
lld/MachO/Writer.cpp | ||
---|---|---|
849 | (not done?) |
lld/MachO/Options.td | ||
---|---|---|
1215–1220 | looks like trailing spaces |
The windows CI failures look unrelated, so I'm gonna go ahead and commit this. Thanks, all, for the reviews!
This will ignore all but the last -dyld_env argument. Does ld64 let you specify it more than once?