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

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap marked an inline comment as done.
alexshap 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;
----------------
smeenai wrote:
> 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.
@smeenai thanks for checking it, I'll take a closer look at it and add a test for a different endianness. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77844