[PATCH] D82503: [llvm-objcopy][MachO] Fix segment size alignment

[Alexander Shaposhnikov via Phabricator via llvm-commits]

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


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/segment-size-alignment.test:59
+## xcrun -sdk iphoneos clang -arch armv7 main.c -o main.exe
+--- !mach-o
+FileHeader:
----------------
jhenderson wrote:
> alexshap wrote:
> > jhenderson wrote:
> > > Do we really need such verbose inputs for the test case? This test is purely about the segment address/size values. Everything else seems to be noise to me, and makes it hard to identify what in the input YAML is actually important.
> > > 
> > > I'm not convinced it's important for the input to be a fully runnable executable.
> > Testing real executables makes several aspects easier:
> > the binaries are valid / they can be run, the issue with misaligned segments doesn't actually manifest itself until smb runs the binary (in particular, all the tools would perfectly consume it without any issues). 
> > More importantly, If the test fails one has to inspect the input, and if the input is invalid it prevents you from using a debugger and other tools.
> > This is the main reason why I'm advocating for having normal binaries for this test rather than something artificial. 
> > A few other considerations: segments (including __LINKEDIT) have to be non-trivial to actually verify those computations / catch issues => their content essentially shouldn't be removed. What can be removed (relatively easy): some load commands (e.g. LC_DYLD_INFO_ONLY, LC_LOAD_DYLINKER, LC_ENCRYPTION_INFO, LC_LOAD_DYLIB), this comes at the cost of the ability to debug / inspect the binaries mentioned above. What do you think @jhenderson ?
> As a developer, I may not have access to Mach-O using hardware to verify whether something runs or to run it in a debugger. It's quite possible however that I might be making changes in a related area, and accidentally break this test. With such a verbose input, it makes it harder for me to figure out what the characteristics of this input are that might be causing my change to fail. A more concise output makes it easier to spot the difference. Obviously, if the behaviour interacts closely with multiple areas of an object, each of those areas will need to be appropriately fleshed out.
> 
> As for inspecting what's wrong with a binary, I'd expect to be able to do that with tools such as llvm-readobj, even if it's malformed in some way. I'd expect such tools to be tolerant of things being malformed, reporting warnings as necessary, so that I can look at the bit that is really important. This is something that has been changed a lot in the ELF side of llvm-readobj, but less so in other tools.
> 
> I think there is a place for tests that take a binary produced by the tools and makes sure its runnable. I just don't think that place is in the low-level LLVM lit tests. Perhaps it belongs in a different suite.
> 
> FWIW, I think the state of Mach-O yaml2obj doesn't help, since it requires very verbose output, much of which could probably be omitted with a bit of improvement to yaml2obj. I think taking the time to do that could be a worthwhile investment, but it isn't something I'll be doing.
> 
> That all being said, I don't actually maintain the Mach-O port, I just help with the high-level reviewing, so I'd defer to others like @smeenai and yourself, as to what your preference is.
@jhenderson - having concise tests is definitely what I support, in this particular case there is a trade-off (described above), honestly I don't have a strong opinion, maybe it should be reduced / unrelated load commands stripped away - the root cause has already been investigated and now we can just formally test the correct behavior. At the moment this procedure - the "surgery" of the input binary involves some manual calculations which I don't like (since they are error-prone), probably this also has contributed to the decision.

As a side note - I personally value your input a lot, I think over time it has helped improve a lot of things around binary-level tools, libObject, ObjectYAML, especially regarding the test coverage, even though in a few cases I could have disagreed with some details.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82503