[PATCH] D131813: [BOLT][NFC] Move addRelocation{X86,AArch64} into MCPlusBuilder

Fri Jan 27 14:39:48 PST 2023

rafauler added inline comments.

================
Comment at: bolt/include/bolt/Core/MCPlusBuilder.h:399
+  /// Return true if the relocation type is supported
+  virtual bool isSupportedRelocation(uint64_t RelType) const {
+    llvm_unreachable("not implemented");
----------------
Amir wrote:
> rafauler wrote:
> > I'm a bit confused with the name, for example, what does it mean to have a supported relocation? Why is ELF::R_AARCH64_CALL26 unsupported? I think either the comments here need to be expanded a bit or the function get a better name, because this "support" is very specific to the needs of BinaryFunction.
> What name and comment would you suggest? These are relocations we store for function splitting and reordering.
I don't have good names that give the necessary context. I guess something like "shouldRecordCodeReloc/shouldStoreReloc" or "shouldRetainCodeReloc". The context might go in a comment maybe... the context is that these relocations are essentially used by our disassembly step to better understand code (by using these code relocs). For ARM, they help us decode instruction operands unambiguously, but sometimes we might discard them because we already have the necessary information in the instruction itself (for example, we don't need to record CALL relocs in ARM because we can fully decode the target from the call operand). For X86, they might be used in scanExternalRefs when we want to skip a function but still patch references inside it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131813