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

[Daniel Bertalan via Phabricator via llvm-commits]

BertalanD added inline comments.


================
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));
----------------
thakis wrote:
> 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)
I'm not convinced that swapping the bytes is enough for endianness conversion. https://www.naic.edu/~phil/notes/bitfieldStorage.html suggests that the bits within the bytes would also need to be reversed. I'm going to update this diff to use bit masks and shifts like D132036.


================
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;
----------------
thakis wrote:
> 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 :)
I didn't know about `SignExtend32`, thank you for mentioning it. Your version does indeed look nicer than what I wrote.


================
Comment at: llvm/lib/Object/MachOObjectFile.cpp:5021
+    } else {
+      llvm_unreachable("Unknown imports format");
+    }
----------------
thakis wrote:
> nit: should this return a malformedError instead? it's data-dependent, so it's not truly unreachable. (but up to you)
I'll add an error message to where we're computing `ImportSize`, and then this branch will be truly unreachable.


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

https://reviews.llvm.org/D131982