[PATCH] D126286: [JITLink] [ELF/AARCH64] Generic aarch64 patch fixups

[Lang Hames via Phabricator via llvm-commits]

lhames accepted this revision.
lhames added a comment.

LGTM. This is great work -- Thanks @sunho!



================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/ELF_aarch64.h:43
+/// Return the string name of the given ELF aarch64 edge kind.
+const char *getELFAArch64RelocationKindName(Edge::Kind R);
+
----------------
This can also be moved down to the `.cpp` file.


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/ELF_aarch64.h:25
+enum ELFAarch64RelocationKind : Edge::Kind {
+  R_AARCH64_CALL26 = Edge::FirstRelocation,
+};
----------------
sgraenitz wrote:
> This originates from my initial AArch64 patch. Looking at the other backends, no-one uses ALL-CAPS for such an enum anymore.
> 
> While you are here, could you fix it as well? I think the naming convention to follow is the one used in the MachO backends, so this would become `ELFBranch26`. If I understand correctly, ELF_aarch64 relocation resolution would then generalize it into `aarch64::Branch26`. The `getELFAarch64RelocationKindName()` function would need adjustment as well.
This enum should be moved down to the `.cpp` file anyway -- this is an implementation detail of the `ELFAArch64LinkGraphBuilder`.


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/MachO_arm64.h:22
 namespace MachO_arm64_Edges {
-
 enum MachOARM64RelocationKind : Edge::Kind {
+  MachOBranch26 = Edge::FirstRelocation,
----------------
This enum should be sunk into `MachO_arm64.cpp` file now, similar to `MachOLinkGraphBuilder_x86_64::MachONormalizedRelocationType`. That can happen in a follow-up patch though.


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/aarch64.h:105-106
+  case Page21:
+  case TLVPage21:
+  case GOTPage21: {
+    assert((E.getKind() != GOTPage21 || E.getAddend() == 0) &&
----------------
Keeping `TLVPage21` and `GOTPage21` here is a good idea for this initial patch (we want it to be a straightforward hoist of the code from the MachO backend). In a future patch, however, they should be removed from applyFixup. Instead, we'll lower them to simpler edge kinds (e.g. deltas) inside the GOT construction pass. That way applyFixup only handles "simple" edges (expression/operand fixup only), and if we encounter a complex edge we can error out, since it should have been lowered by an earlier pass.


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/aarch64.h:194-197
+  default:
+    return make_error<JITLinkError>(
+        "In graph " + G.getName() + ", section " + B.getSection().getName() +
+        "unsupported edge kind" + getEdgeKindName(E.getKind()));
----------------
This triggered a covered switch warning for me:

```
[660/896] Building CXX object lib/ExecutionEngine/JITLink/CMakeFiles/LLVMJITLink.dir/ELF_aarch64.cpp.o
llvm-github/llvm/lib/ExecutionEngine/JITLink/ELF_aarch64.cpp:110:5: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]
    default:
    ^
llvm-github/llvm/lib/ExecutionEngine/JITLink/ELF_aarch64.cpp:110:5: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]
llvm-github/llvm/lib/ExecutionEngine/JITLink/ELF_aarch64.cpp:67:54: note: in instantiation of member function 'llvm::jitlink::ELFLinkGraphBuilder_aarch64<llvm::object::ELFType<llvm::support::little, true>>::addSingleRelocation' requested here
                                              &Self::addSingleRelocation))
                                                     ^
llvm-github/llvm/lib/ExecutionEngine/JITLink/ELF_aarch64.cpp:148:10: note: in instantiation of member function 'llvm::jitlink::ELFLinkGraphBuilder_aarch64<llvm::object::ELFType<llvm::support::little, true>>::addRelocations' requested here
  return ELFLinkGraphBuilder_aarch64<object::ELF64LE>((*ELFObj)->getFileName(),
```

I think it can just be commented out for now, and re-enabled when the GOT edges are moved out of `applyFixup`.


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

https://reviews.llvm.org/D126286