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

Stefan Gränitz via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed May 25 04:26:03 PDT 2022


sgraenitz added a comment.

Kudos, this is a really good first patch. Thanks for working on this sunho! I think this is almost ready to land as is. Just pointing out a few minor details I encountered while reading through.



================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/ELF_aarch64.h:25
+enum ELFAarch64RelocationKind : Edge::Kind {
+  R_AARCH64_CALL26 = Edge::FirstRelocation,
+};
----------------
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.


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/aarch64.h:202
+}
+
 } // namespace aarch64
----------------
All these fixups are now automatically available in the AArch64 backend as well? That's amazing! Let's talk about the testing story for it in our weekly today.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_aarch64.cpp:110
+    }
+    default:
+      llvm_unreachable("Special relocation kind should not appear in "
----------------
I think Lang shortly mentioned it in Discord already: The `default` block here is counter-productive and causes a compiler warning, because LLVM builds with `-Wcovered-switch-default`. If you remove the default and more enum values are added in the future, the compiler can warn us in case we forget to handle them in this switch. That's a very handy mechanism.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_aarch64.cpp:175
+  default:
+    llvm_unreachable("unhandled elf relocation kind name");
+  }
----------------
We could now use `getGenericEdgeKindName()` instead of the `llvm_unreachable()` here


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

https://reviews.llvm.org/D126286



More information about the llvm-commits mailing list