[PATCH] D145687: [BOLT] Add minimal RISC-V 64-bit support

[Rafael Auler via Phabricator via llvm-commits]

rafauler added inline comments.


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:2639
+    if (ContainingBF && RelSymbol != InputFile->symbol_end() &&
+        // TODO is this good enough? Could also pass for UND symbols.
+        cantFail(RelSymbol->getSection()) ==
----------------
Maybe convert this TODO into a regular comment saying this also passes for UND symbols, even though the intent is to capture ABS symbols that land outside any section.


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:2642
+            RelSymbol->getObject()->section_end()) {
+      auto *Symbol = BC->getOrCreateUndefinedGlobalSymbol(SymbolName);
+      ContainingBF->addRelocation(Rel.getOffset(), Symbol, Rel.getType(), 0,
----------------
I'm probably not getting the full picture here, so correct my thinking below.

So, if I understand correctly, you have an ABS __global_pointer$ symbol that points outside any section that BOLT has loaded (otherwise ReferencedSection wouldn't be null here). Therefore, BOLT won't change the location of this symbol (if it is pointing to some unknown region of memory that is not covered by any section known by BC->getSectionForAddress(SymbolAddress).

So why register this reloc, if we are not changing this symbol? We will arrive at link step and RuntimeDyld will resolve this unknown symbol to zero, no? How do we know the correct value of this symbol?

If we don't create the relocation here (with addRelocation), we will disassemble the original code that is already patched with the correct value for this symbol -- at least for X86. For RISC, this is harder to do if the real address is split in different instructions, and we don't have the luxury of ignoring the relocation. So my feeling is that, at the very least, this code is RISC-only, but you didn't protect this code from running in the X86 backend.


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

https://reviews.llvm.org/D145687