[PATCH] D78799: [llvm-objcopy][MachO] Fix segment's vmsize

[Alexander Shaposhnikov via Phabricator via llvm-commits]

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


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOLayoutBuilder.cpp:161
           Sec->Offset = 0;
-          VMSize += Sec->Size;
         } else {
----------------
alexshap wrote:
> alexshap wrote:
> > @MaskRay: the issue was here. (VMSize += Sec->Size) - this is not correct (and it results in producing a broken binary),
> > also you can  take a look at the newly added test and the comment therein. I've already mentioned in the past reviews that for tests containing sections it's error-prone to modify MachO yaml manually, even removing one seemingly unrelated load command triggers the recalculation of the offsets mentioned inside other load commands + the test is not really large, it's the minimal example which triggers the issue (because of the page alignment). Having a valid binary also enables us to use the existing tools from Xcode to validate / introspect the binary if necessary.
> to the best of my knowledge this test is not supposed to be reorganized, and if i'm not mistaken it should not even change (because it copies the binary without modifications). P.S. the test fails (as expected) without the fix.
just to be clear: (what i mentioned on one of the previous diffs) i think one can refactor / consolidate the tests which verify that some load commands are supported and simply copied-over. Tests like this one (which are supposed to catch a particular issue 
(on some reduced example) and the tests which verify a particular functionality should not be merged etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78799