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

[Nico Weber via Phabricator via llvm-commits]

thakis 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));
----------------
BertalanD wrote:
> 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.
Huh, TIL, I suppose.

What I used to believe up to now: There are two separate things: in-memory byte order, and order in which bitfields get assigned to their underlying words.

The c99 standard (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1124.pdf), 6.7.2.1.10: says about the latter "The order of allocation of bit-fields within a unit (high-order to low-order or low-order to high-order) is implementation-defined."

So I thought that the order bitfields are assigned is implementation-defined, but independent of byte order.

But sysv abi (https://www.uclibc.org/docs/psABI-x86_64.pdf) paragraph 3.1 says: "bit-fields are allocated from right to left". So I thought, in practice, this settles things: Bit fields are assigned right-to-left (in a register, say), and then they assume system endianness when written to memory, conceptually.

(In theory, gcc has all kinds of options here: https://gcc.gnu.org/onlinedocs/gccint/Storage-Layout.html BITS_BIG_ENDIAN, PCC_BITFIELD_TYPE_MATTERS, TARGET_MS_BITFIELD_LAYOUT_P, …)

But https://godbolt.org/z/6sqo7nEd4 shows that this clearly isn't true. (Also, what's with the `65535` in the big-endian ppc64 output there? That looks completely wrong?)

And CGRecordLayoutBuilder.cpp has:

```
void CGRecordLowering::setBitFieldInfo(
...
  // Reverse the bit offsets for big endian machines. Because we represent
  // a bitfield as a single large integer load, we can imagine the bits
  // counting from the most-significant-bit instead of the
  // least-significant-bit.
  if (DataLayout.isBigEndian())
    Info.Offset = Info.StorageSize - (Info.Offset + Info.Size);
```

And gcc seems to match clang's behavior (see godbolt link), so that seems very intentional.

Does anyone reading this know what specifies this?

…aha, the ppc sysv abi (http://refspecs.linux-foundation.org/elf/elfspec_ppc.pdf) does say: " Bit-fields are allocated from right to left (least to most significant) on Little-Endian implementations and from left to right (most to least significant) on Big-Endian implementations."

I wish I hadn't learned that!


================
Comment at: llvm/lib/Object/MachOObjectFile.cpp:5004
+
+      LibOrdinal = getEncodedOrdinal<uint8_t>(RawValue[0] & 0xFF);
+      WeakImport = (RawValue[0] >> 8) & 1;
----------------
The in-register view (i.e. not in-memory, we can ignore the byte-swapping of big-endian vs little-endian here) of

```
struct dyld_chained_import {
  uint32_t lib_ordinal : 8;
  uint32_t weak_import : 1;
  uint32_t name_offset : 23;
};
```

is:

* In little-endian (bits assigned right-to-left):
  * most significant 23 bits are name_offset
  * then 1 bit weak_import
  * then 8 bit lib_ordinal in the lowest 8 bits

* In big-endian (bits assigned left-to-right):
  * 8 bit lib_ordinal in the highest 8 bits
  * then 1 bit weak_import
  * least significant 23 bits are name_offset

So I think even with the manual masking, this currently gets things wrong.

Also, it's weird that we don't use the dyld_chained_import structs at all. Strange for greppability, using something like http://llvm-cs.pcc.me.uk/ for cross-references, etc.

What do you think about adding back the Swap<> functions and manually swapping the bitfields around too after swapping the bytes? That assumes sysv abi, but that'll work almost everywhere (can't think of a place where it wouldn't? Maybe windows big endian? Didn't try that, but I wouldn't be surprised if clang-cl didn't work great there either), and if someone needs this to work on some obscure system, they can worry about it then?


================
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;
----------------
BertalanD wrote:
> 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.
(FWIW, I didn't know about `SignExtend32` either while I wrote the "looks weird" paragraph. I thought about how this could be written differently, then ran `rg -i signextend llvm` to see how other code handles this and then found SignExtend32 in there and figured it matches well enough.)


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

https://reviews.llvm.org/D131982