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

[alexander-shaposhnikov]

Summary

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

Test plan: make check-all

[MaskRay]

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]

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).

[alexander-shaposhnikov]

rename the test

[jhenderson]

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!

[alexander-shaposhnikov]

Rename the test

[MaskRay]

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

1 ↗

(On Diff #259422)

Should the file be

lc-weak-dylib.test

or

lc-load-weak-dylib.test

?

[alexander-shaposhnikov]

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

1 ↗

(On Diff #259422)

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.

[alexander-shaposhnikov]

Rename the test again

[MaskRay]

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

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

1 ↗

(On Diff #259422)

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

[alexander-shaposhnikov]

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

1 ↗

(On Diff #259422)

@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]

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.

llvm/test/tools/llvm-objcopy/MachO/load_weak_dylib_lc.test

+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy %t %t.copy
+# RUN: cmp %t %t.copy
+
+--- !mach-o
+FileHeader:
+  magic:           0xFEEDFACF
+  cputype:         0x0100000C
+  cpusubtype:      0x00000000
+  filetype:        0x00000002
+  ncmds:           1
+  sizeofcmds:      80
+  flags:           0x00002000
+  reserved:        0x00000000
+LoadCommands:
+ - cmd:             LC_LOAD_WEAK_DYLIB
+   cmdsize:         80
+   dylib:
+     name:            24
+     timestamp:       2
+     current_version: 65536
+     compatibility_version: 65536
+   PayloadString:   '/System/Library/Frameworks/Network.framework/Network'
+   ZeroPadBytes:    4
+...

llvm/tools/llvm-objcopy/MachO/MachOLayoutBuilder.cpp

for (LoadCommand &LC : O.LoadCommands) {
     case MachO::LC_SEGMENT_64:
     case MachO::LC_VERSION_MIN_MACOSX:
     case MachO::LC_VERSION_MIN_IPHONEOS:
     case MachO::LC_VERSION_MIN_TVOS:
     case MachO::LC_VERSION_MIN_WATCHOS:
     case MachO::LC_BUILD_VERSION:
     case MachO::LC_ID_DYLIB:
     case MachO::LC_LOAD_DYLIB:
+    case MachO::LC_LOAD_WEAK_DYLIB:
     case MachO::LC_UUID:
     case MachO::LC_SOURCE_VERSION:
       // Nothing to update.
       break;
     default:
       // Abort if it's unsupported in order to prevent corrupting the object.
       return createStringError(llvm::errc::not_supported,
                                "unsupported load command (cmd=0x%x)", cmd);