[PATCH] D131982: [llvm-objdump] Complete -chained_fixups support

[Nico Weber via Phabricator via llvm-commits]

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

Very nice!



================
Comment at: llvm/include/llvm/BinaryFormat/MachO.h:2131
+inline void swapStruct(dyld_chained_import &C) {
+  uint32_t Raw[1];
+  memcpy(Raw, &C, sizeof(C));
----------------
It'd be nice to use bit_cast from llvm/ADT/bit.h here, which will ensure that Raw is the same size as dyld_chained_import. Here this is fairly easy I think:

```
  auto Raw = bit_cast<uint32_t>(C);
  sys::swapByteOrder(Raw);
  C = bit_cast<dyld_chained_import>(Raw);
```

Below it's a bit more tricky, but I think this might work (all untested though):

```
  auto Raw = bit_cast<std::array<uint32_t, 2>>(C);
  sys::swapByteOrder(Raw[0]);
  sys::swapByteOrder(Raw[1]);
  C = bit_cast<(dyld_chained_import_addend>(Raw);
```

(and analogously for the 64-bit version)


================
Comment at: llvm/lib/Object/MachOObjectFile.cpp:4934
+    return MachO::BIND_SPECIAL_DYLIB_MAIN_EXECUTABLE;
+  else if (Value == static_cast<T>(MachO::BIND_SPECIAL_DYLIB_FLAT_LOOKUP))
+    return MachO::BIND_SPECIAL_DYLIB_FLAT_LOOKUP;
----------------
nit: no else after return

This function looks weird! It almost does `if (V == a) return a` a bunch of times followed by `return V`, and the special cases look pretty pointless at first. (I understand they're needed to sign-extend the 3 special values to int, while the input is 8 or 16 bits.) It feels like there should be a nicer way to write this.

Maybe

```
  if (Value == static_cast<T>(MachO::BIND_SPECIAL_DYLIB_MAIN_EXECUTABLE) ||
      Value == static_cast<T>(MachO::BIND_SPECIAL_DYLIB_FLAT_LOOKUP) ||
       third case)
    return SignExtend32<sizeof(T) * 8>(Value);
  return Value;
```

Not sure if that's actually better, but it makes the difference a bit more explicit maybe.

Up to you :)


================
Comment at: llvm/lib/Object/MachOObjectFile.cpp:4962
+  if (Header.imports_format == MachO::DYLD_CHAINED_IMPORT)
+    ImportSize = sizeof(uint32_t);
+  else if (Header.imports_format == MachO::DYLD_CHAINED_IMPORT_ADDEND)
----------------
Isn't `sizeof(dyld_chained_import)` clearer here? (likewise in the other two cases)


================
Comment at: llvm/lib/Object/MachOObjectFile.cpp:4998
+        return ImportOrErr.takeError();
+      LibOrdinal = GetEncodedOrdinal<uint8_t>(ImportOrErr->lib_ordinal);
+      NameOffset = ImportOrErr->name_offset;
----------------
This has GetEncodedOrdinal with upper-case G while the function above has it with lower-case g. Can this build? Maybe you uploaded a half-committed patch?


================
Comment at: llvm/lib/Object/MachOObjectFile.cpp:5021
+    } else {
+      llvm_unreachable("Unknown imports format");
+    }
----------------
nit: should this return a malformedError instead? it's data-dependent, so it's not truly unreachable. (but up to you)


================
Comment at: llvm/tools/llvm-objdump/MachODump.cpp:1295
+    outs() << "dyld chained import addend64";
+  // FIXME: otool prints the encoded value as well.
+  outs() << '[' << Idx << "]\n";
----------------
It makes sense to me to wait with this until the upstreaming has happened to see how to best implement this then.


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

https://reviews.llvm.org/D131982