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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/MachOYAML.h:32
+  // Offset in the section to what is being relocated.
+  llvm::yaml::Hex32 address;
+  // Symbol index if r_extern == 1 else section index.
----------------
alexshap wrote:
> grimar wrote:
> > Should it be called `offset`?
> @grimar -  I agree, however, the rationale here was to reuse the existing names, even though they are awful, to avoid causing further pain when people try to match different structures from different codebases.
> 
> http://llvm.org/doxygen/MachORelocation_8h_source.html 
> https://opensource.apple.com/source/cctools/cctools-949.0.1/include/mach-o/reloc.h.auto.html
I see. Perhaps it is OK then. Sticking to official naming (here and below) probably makes sense.


================
Comment at: llvm/include/llvm/ObjectYAML/MachOYAML.h:36
+  bool is_pcrel;
+  // Real length = 2 ^ length.
+  uint8_t length;
----------------
As far I understand the values that are >=4 are invalid. I'd suggest to use /* 0=byte, 1=word, 2=long, 3=quad */ comment then (taken from https://opensource.apple.com/source/cctools/cctools-949.0.1/include/mach-o/reloc.h.auto.html), it is currently not obvious it that the value is expected to be in [0..3] I think.

Perhaps worth adding a test with `length == max(uint8_t)`, `length == 5` to show the behavior. Seems that with the value of 5, this will produce the relocation like if the `length` was `1`? Ideally you might want to validate fields and report an error, e.g (from ELF):

```
 StringRef MappingTraits<ELFYAML::Symbol>::validate(IO &IO,
                                                    ELFYAML::Symbol &Symbol) {
   if (Symbol.Index && Symbol.Section.data())
     return "Index and Section cannot both be specified for Symbol";
   return StringRef();
 }
```

yaml2obj is often used to produce broken objects, but in this case the behavior is unexpected (i.e. if it produced a relocation with the length 5, it would be fine), so probably needs to be reported.

Fields validating is probably not that important to do for the initial implementation though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77844