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

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

I know nothing about MachO, few suggestions about the code are below.
I wonder if new tests should be splitted and placed to test\tools\yaml2obj and test\tools\obj2yaml folders
(it is where the active development of these tools goes and where we have tests for relocations for another targets).



================
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.
----------------
Should it be called `offset`?


================
Comment at: llvm/include/llvm/ObjectYAML/MachOYAML.h:34
+  // Symbol index if r_extern == 1 else section index.
+  uint32_t symbolnum;
+  bool is_pcrel;
----------------
I'd rename to something like `sym_or_sec_index` probably as `symbolnum` doesn't seem to be a right name given the comment saying it can hold a section index.


================
Comment at: llvm/include/llvm/ObjectYAML/MachOYAML.h:37
+  // Real length = 2 ^ length.
+  uint8_t length;
+  bool is_extern;
----------------
Should it be called `log2length` or alike?


================
Comment at: llvm/include/llvm/ObjectYAML/MachOYAML.h:58
   Optional<llvm::yaml::BinaryRef> content;
+  std::vector<Relocation> relocations;
 };
----------------
Can you add a test when a YAML description has a `relocations` property which is empty? 

`relocations: []`



================
Comment at: llvm/lib/ObjectYAML/MachOEmitter.cpp:63
   MachO::mach_header_64 Header;
+  bool FoundLinkEditSeg = false;
 };
----------------
Add a comment?


================
Comment at: llvm/lib/ObjectYAML/MachOEmitter.cpp:330
+static MachO::any_relocation_info
+makeRelocationInfo(const MachOYAML::Relocation &R, bool IsObjLittleEndian) {
+  assert(!R.is_scattered && "non-scattered relocation expected");
----------------
LLVM often use `IsLE`, I'd suggest to follow as it is shorter and more common.


================
Comment at: llvm/lib/ObjectYAML/MachOEmitter.cpp:339
+  const unsigned IsExtern = R.is_extern;
+  const unsigned Type = R.type;
+  if (IsObjLittleEndian)
----------------
I think you can inline these 5 `const unsigned` variables.


================
Comment at: llvm/lib/ObjectYAML/MachOEmitter.cpp:357
+  const unsigned Log2Size = R.length;
+  const unsigned IsPCRel = R.is_pcrel;
+  MRE.r_word0 = ((FixupOffset << 0) | (Type << 24) | (Log2Size << 28) |
----------------
The same.


================
Comment at: llvm/test/ObjectYAML/MachO/relocations_arm64.yaml:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objdump --macho --reloc %t | FileCheck %s --check-prefix=OBJDUMP-VERIFY
----------------
Should instead we add 2 test cases, one to test\tools\obj2yaml and one to test\tools\yaml2obj?
(I'd expect to see tests for these tools)


================
Comment at: llvm/tools/obj2yaml/macho2yaml.cpp:151
     } else {
-      Sections.push_back(constructSection(*Curr));
+      // For MachO section indices start from 1.
+      if (Expected<MachOYAML::Section> S =
----------------
This comment is duplicated twice in this method.


================
Comment at: llvm/tools/obj2yaml/macho2yaml.cpp:156
+      else
+        return S.takeError();
     }
----------------
May be generalize this piece a bit? It does not seem to be a perfomance critical place, so you probably can do:

```
  SectionType Sec;
  memcpy((void *)&Sec, Curr, sizeof(SectionType));
  if (Obj.isLittleEndian() != sys::IsLittleEndianHost)
    MachO::swapStruct(Sec);

  // For MachO section indices start from 1.
  if (Expected<MachOYAML::Section> S =
          constructSection(Sec, Sections.size() + 1))
    Sections.push_back(std::move(*S));
  else
    return S.takeError();
```


================
Comment at: llvm/tools/obj2yaml/macho2yaml.cpp:249
   std::unique_ptr<DWARFContext> DICtx = DWARFContext::create(Obj);
-  if (auto Err = dwarf2yaml(*DICtx, Y->DWARF))
+  if (Error Err = dwarf2yaml(*DICtx, Y->DWARF))
     return std::move(Err);
----------------
Unrelated. (Seems this file might benefit from a independent NFC change converting autos to real types).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77844