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

[Lang Hames via Phabricator via llvm-commits]

lhames 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:
> alexshap wrote:
> > 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. 
> 0/ https://en.cppreference.com/w/cpp/language/bit_field see the section Notes - so it looks like in general the way how bit fields are packed is not defined by the standard, though I'm not 100% sure and would not claim anything here.
> 
> 1/ In general, the picture appears to be not super clean and straightforward, e.g. see the comments in 
> <mach-o/reloc.h>. 
> 
> 2/ To figure out how exactly relocations need to be serialized into the binary file I was looking at how LLVM's libObject parses MachO object files in the first place.
> See 
> https://llvm.org/doxygen/MachOObjectFile_8cpp_source.html#l00068 , 
> https://llvm.org/doxygen/MachOObjectFile_8cpp_source.html#l04403 ,
> https://llvm.org/doxygen/MachOObjectFile_8cpp_source.html#l00153 .
> 
> 3/ I've added a test with a big endian binary file and I've also verified that
> otool -r / llvm-objdump --macho --reloc / obj2yaml are all consistent and can correctly handle the binaries
> created by yaml2obj.
> 
> 
> 
> Firstly, Apple's Mach-O headers don't define the bitfield in relocation_info differently for big vs. little-endian machines, but LLVM's header does, and that discrepancy seems potentially bad. @lhames changed that in rL358839; perhaps he can comment on that?

Huh. Re-reading reloc.h it looks like my change was broken. I wonder that why that didn't blow more tests up. :/

I'll see if I can fix this today.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77844