[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:36
+  bool is_pcrel;
+  // Real length = 2 ^ length.
+  uint8_t length;
----------------
grimar wrote:
> 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.
> Fields validating is probably not that important to do for the initial implementation though.

And testing broken values either I think. Up to you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77844