[PATCH] D68065: Propeller: LLD Support for Basic Block Sections

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Arch/X86_64.cpp:163
+
+static unsigned getJumpRelocationWithOffset(const InputSection &IS,
+                                            uint64_t Offset) {
----------------
getJumpRelocationWithOffset is unused.

See Relocations.cpp:scanRelocs. A better way is to sort relocations beforehand.


================
Comment at: lld/ELF/Arch/X86_64.cpp:175
+  return (R.type == R_X86_64_PLT32 || R.type == R_X86_64_PC32 ||
+          R.type == R_X86_64_PC8);
+}
----------------
Neither _PC8 nor _PC32 is tested.


================
Comment at: lld/ELF/Arch/X86_64.cpp:258
+  const uint8_t *SecContents = IS.data().data();
+  if (!isDirectJmpInsnOpcode(SecContents + R.offset - 1))
+    return false;
----------------
Variable cases need to be fixed.

If `R` is an invalid R_X86_64_PLT32 that happens to be r_offset=0. R.offset-1 will be an out-of-bounds read.


================
Comment at: lld/ELF/Arch/X86_64.cpp:260
+    return false;
+
+  if (isFallThruRelocation(IS, File, NextIS, R)) {
----------------
I am a bit skeptical about this scheme.

The code sequence relies heavily on the order of the last two instructions of a basic block.
Does it disable optimizations placing additional instructions between jcc and jmp?


================
Comment at: lld/ELF/Arch/X86_64.cpp:277
+  unsigned RbIndex =
+      getRelocationWithOffset(IS, (IS.getSize() - SizeOfDirectJmpInsn - 4));
+  if (RbIndex == IS.relocations.size())
----------------
ditto. An invalid r_offset can cause the read to go out-of-bounds.


================
Comment at: lld/ELF/Arch/X86_64.cpp:297
+  IS.addJumpRelocation({JInvert, (Rb.offset - 1), 4});
+  // Move R's values to Rb
+  Rb.expr = R.expr;
----------------
`rb = {... , ..., ...}`

Set all fields at once.


================
Comment at: lld/ELF/Arch/X86_64.cpp:667
+    break;
+  default:
+    llvm_unreachable("Unknown Jump Relocation");
----------------
change default to J_UNKNOWN. All cases of the switch should be covered, instead of relying on `default:`.


================
Comment at: lld/ELF/InputSection.cpp:869
 
+    if (expr == R_SIZE) {
+      target->relocateNoSym(bufLoc, type,
----------------
How is `R_SIZE` used?


================
Comment at: lld/ELF/InputSection.cpp:946
   for (const Relocation &rel : relocations) {
+    if (rel.expr == R_NONE)
+      continue;
----------------
After patching `expr` to R_NONE, propeller should delete these relocations. InputSection.cpp should not know R_NONE has to be skipped.


================
Comment at: lld/ELF/LTO.cpp:88
+      c.Options.BBSections = BasicBlockSection::None;
+    else {
+      llvm::propeller::getBBSectionsList(config->ltoBBSections,
----------------
Check `config->ltoBBSections == "list"`


================
Comment at: lld/ELF/OutputSections.cpp:259
+    fatal("failed padding with special filler");
+  memcpy(Buf + I, SFiller.at(remaining - 1).data(), remaining);
+}
----------------
`Sfillter[remaining - 1]`

vector::at has an unneeded bound check.


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

https://reviews.llvm.org/D68065