[PATCH] D78602: [llvm-objcopy][MachO] Copy LC_LOAD_WEAK_DYLIB load commands

Tue Apr 21 19:30:31 PDT 2020

alexshap marked an inline comment as done.
alexshap added inline comments.

================
Comment at: llvm/test/tools/llvm-objcopy/MachO/weak_load_lc.test:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy %t %t.copy
----------------
MaskRay wrote:
> 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.
@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).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78602/new/

https://reviews.llvm.org/D78602