This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy][MachO] Copy LC_LOAD_WEAK_DYLIB load commands
ClosedPublic

Authored by alexander-shaposhnikov on Apr 21 2020, 5:26 PM.

Details

Summary

LC_LOAD_WEAK_DYLIB is analogous to LC_LOAD_DYLIB and doesn't require any special handling.

Test plan: make check-all

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: abrachet. · View Herald Transcript
MaskRay added inline comments.Apr 21 2020, 6:25 PM
llvm/test/tools/llvm-objcopy/MachO/weak_load_lc.test
1 ↗(On Diff #259135)

The code looks good, but how is the test named? The actual command is named LC_LOAD_WEAK_DYLIB, and I did not expect that the name components are shuffled as weak load lc...

If the test makes more sense as an addition to basic-executable-copy.test, please do so, because we probably don't want to add a test for each such command. It seems to me that basic-executable-copy.test is a bit complicated now but I don't know whether splitting it is feasible.

alexander-shaposhnikov marked an inline comment as done.Apr 21 2020, 7:30 PM
alexander-shaposhnikov added inline comments.
llvm/test/tools/llvm-objcopy/MachO/weak_load_lc.test
1 ↗(On Diff #259135)

@MaskRay - I thought about consolidating tests with "relatively simple" load commands in the past (and it might have already been discussed somewhere at some point, i don't remember), however, it's not that straightforward (and if not done right will make things worse not better). There are a few concerns to keep in mind: (1) for some load commands there are 32/64-bit variants - they can't be combined in one binary, (2) some load commands are mutually exclusive, even though often it's not documented, some Apple's tools will complain on this, thus it'll make tests which expect a valid binary actually use an invalid one, (3) for some load commands which are currently copied over without modifications this is just a temporary solution and the approach might change in the future, (4) adding / removing a load command from YAML is NOT a local operation, thus if someone wants to combine the tests I'd like to see a clear proposal on the logical structure / organization of the tests before doing such refactoring. (5) There is a group of basic* tests, I'd prefer not to change them.
This is my 0.02$.

Having said that, it's a reasonable question and with a good plan this refactoring can be useful (though not a top pri right now).

jhenderson accepted this revision.Apr 22 2020, 12:22 AM

I'm good with this, although I might suggest a different naming scheme with this and related tests, namely to start with "lc"/"load-command" or similar (perhaps lc-weak-dylib.test for this test). That way, all load command tests will appear adjacent to each other in the directory list, because they'll all have the same prefix, making it easier to find tests. Please also use '-' instead of '_' in test names, for consistency with existing tests, and for easier typing!

This revision is now accepted and ready to land.Apr 22 2020, 12:22 AM
MaskRay added inline comments.Apr 22 2020, 4:25 PM
llvm/test/tools/llvm-objcopy/MachO/lc-weak-dylib.test.test
1

Should the file be

lc-weak-dylib.test

or

lc-load-weak-dylib.test

?

alexander-shaposhnikov marked an inline comment as done.Apr 22 2020, 4:53 PM
alexander-shaposhnikov added inline comments.
llvm/test/tools/llvm-objcopy/MachO/lc-weak-dylib.test.test
1

personally i think if we prefer dashes in such cases it should be lc-load-weak-dylib.test,
i don't mind renaming this test again, however, the above comment was suggesting the following:
"perhaps lc-weak-dylib.test for this test".
I think lc-load-weak-dylib.test is a better name.

Rename the test again

MaskRay accepted this revision.Apr 22 2020, 8:51 PM

LG once the strange test file name *.test.test is fixed.

llvm/test/tools/llvm-objcopy/MachO/lc-weak-dylib.test.test
1

I meant, the test is named *.test.test now. Hope this is not intentional.

alexander-shaposhnikov marked an inline comment as done.Apr 22 2020, 11:04 PM
alexander-shaposhnikov added inline comments.
llvm/test/tools/llvm-objcopy/MachO/lc-weak-dylib.test.test
1

@MaskRay this might be a bug of Phabricator, it might be confused by "continuous file renaming", though i don't know. For me the latest revision (I've not uploaded anything new) looks like this:


(and of course the file name is correct)

jhenderson accepted this revision.Apr 23 2020, 12:23 AM

FWIW, the test name looks fine to me (i.e. doesn't have '.test.test').

For the record, my previous example was just to do with the dashes and "lc" prefix rather than suffix. The current name does indeed make more sense than my rough suggestion.

This revision was automatically updated to reflect the committed changes.