[PATCH] D132036: [llvm-objdump] Add -dyld_info to llvm-otool

[Nico Weber via Phabricator via llvm-commits]

thakis accepted this revision.
thakis added a comment.
This revision is now accepted and ready to land.

Sorry for the delay.



================
Comment at: llvm/lib/Object/MachOObjectFile.cpp:2116
+  size_t Idx = 0;
+  for (auto LoadCmd : load_commands()) {
+    switch (LoadCmd.C.cmd) {
----------------
The name-based version of this has to loop, but couldn't this version just do:

  auto &LoadCmd = LoadCommands[SegmentIndex];

instead of the loop? (Range check before that, of course.)


================
Comment at: llvm/lib/Object/MachOObjectFile.cpp:3391
+  // For dyld_chained_ptr_64_{bind,rebase}, the most significant bit determines
+  // whether it's a bind or a rebase.
+  bool IsBind = Field(63, 1);
----------------
Put this comment up with the struct definitions too.


================
Comment at: llvm/lib/Object/MachOObjectFile.cpp:3432
+         SegInfo.PageStarts[PageIndex] == MachO::DYLD_CHAINED_PTR_START_NONE)
+    ++PageIndex;
+
----------------
We have this loop three times. Maybe move it into a `FindNextPageWithFixups()` helper?


================
Comment at: llvm/lib/Object/MachOObjectFile.cpp:3390-3392
+  auto Field = [this](uint8_t Right, uint8_t Count) {
+    return (RawValue & ((1ull << Count) - 1) << Right) >> Right;
+  };
----------------
BertalanD wrote:
> BertalanD wrote:
> > reminder to self: simplify this with `maskTrailingOnes`.
> nevermind. That wouldn't make the code any clearer.
> 
> This weird lambda is needed because bitfields are a pain to work with when endianness is involved.
My opinion here is the same as on D131982 (but I agree it makes sense to do the same thing here as over there, and we currently do the manual bit fiddling there).


================
Comment at: llvm/test/tools/llvm-objdump/MachO/dyld-info.test:4
+RUN: llvm-otool -dyld_info %p/Inputs/chained-fixups.macho-x86_64 | \
+RUN:     FileCheck -DNAME=%p/Inputs/chained-fixups.macho-x86_64 %s
 
----------------
Should we have a test for `llvm-otool -dyld_info` running on a file with opcode fixups (instead of chained fixups) too? (ok to punt this to a follow-up)


================
Comment at: llvm/test/tools/llvm-objdump/MachO/dyld-info.test:10-16
+CHECK-NEXT: __DATA_CONST __const 0x{{0*}}3E0   0x8010000000000001 bind   0x{{0*}}0      libdylib       _weakImport (weak import)
+CHECK-NEXT: __DATA_CONST __const 0x{{0*}}3E8   0x8000000000000000 bind   0x{{0*}}0      flat-namespace _dynamicLookup
+CHECK-NEXT: __DATA       __data  0x{{0*}}3F0   0x00200000000003F0 rebase                               0x{{0*}}3F0
+CHECK-NEXT: __DATA       __data  0x{{0*}}400   0x8000000000000004 bind   0x{{0*}}0      weak           _weak
+CHECK-NEXT: __DATA       __data  0x{{0*}}1410  0x8000000000000003 bind   0x{{0*}}0      weak           _weakLocal
+CHECK-NEXT: __DATA       __data  0x{{0*}}3410  0x8010000000000002 bind   0x{{0*}}0      libdylib       _dylib
+CHECK-NEXT: __DATA       __data  0x{{0*}}3418  0x800000002A000002 bind   0x{{0*}}2A     libdylib       _dylib
----------------
BertalanD wrote:
> All the `{{0*}}` are a bit awkward, but I want this test to pass with cctools `otool` as well. It has a bunch of extra logic in the printing code to use the width of the largest number, while this patch always uses a fixed width; so the test needs to accept an arbitrary number of leading zeros.
Maybe just do the `FIXME: Align the columns properly`? That's what, like 6 lines?


================
Comment at: llvm/tools/llvm-objdump/MachODump.cpp:1191
 
 static void printMachOChainedFixups(object::MachOObjectFile *Obj) {
   Error Err = Error::success();
----------------
We should probably rename this function to `printMachODyldInfo` now.


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

https://reviews.llvm.org/D132036