[PATCH] D77844: [ObjectYAML][MachO] Add support for relocations

Tue Apr 14 13:31:09 PDT 2020

smeenai added a subscriber: lhames.
smeenai added inline comments.

================
Comment at: llvm/lib/ObjectYAML/MachOEmitter.cpp:266
+  assert(!R.is_scattered && "non-scattered relocation expected");
+  MachO::any_relocation_info MRE;
+  MRE.r_word0 = R.address;
----------------
alexshap wrote:
> smeenai wrote:
> > Any reason to prefer `any_relocation_info` to `relocation_info`? Assigning to the individual fields in it would be much clearer than all the shifting and bit arithmetic below IMO.
> thank you for review!
> relocation_info appears to break the pattern used across the codebase
> when we populate a struct and then if necessary call  MachO::swapStruct (there is no "swapStruct" for relocation_info), it seems to me it would make things inconsistent / a tiny bit harder to reason about.
> 
So I'm not actually sure `swapStruct` is doing the right thing for `any_relocation_info`, looking at this more closely.

Firstly, [Apple's Mach-O headers](https://opensource.apple.com/source/cctools/cctools-949.0.1/include/mach-o/reloc.h.auto.html) don't define the bitfield in `relocation_info` differently for big vs. little-endian machines, but [LLVM's header does](http://llvm.org/doxygen/BinaryFormat_2MachO_8h_source.html#l00949), and that discrepancy seems potentially bad. @lhames changed that in rL358839; perhaps he can comment on that?

Secondly, for `scattered_relocation_info`, the intent of the big-endian vs. little-endian definitions of the bitfield appears to be that the bitfield has the same layout on both, e.g. that `r_scattered` is always the most significant bit. However, [`swapStruct` for `any_relocation_info`](http://llvm.org/doxygen/BinaryFormat_2MachO_8h_source.html#l01258) just indiscriminately byte-swaps both words in the `any_relocation_info`, which doesn't seem correct.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77844