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

[Sriraman Tallam via Phabricator via llvm-commits]

tmsriram added inline comments.


================
Comment at: lld/ELF/Arch/X86_64.cpp:154
+        IS.relocations[I].expr != R_NONE)
+      break;
+  }
----------------
ruiu wrote:
> nit: `Return I` here and add an `llvm_unreachable` after the loop, so that we can catch an impossible condition at runtime.
Don't follow.  I == rellocations.size() is used to check that no such relocation was found. It is not an impossible condition.


================
Comment at: lld/ELF/Arch/X86_64.cpp:191-192
+
+  // If this jmp is a fall thru, the target offset is the beginning of the
+  // next section.
+  uint64_t NextSectionOffset = NextIS->getOutputSection()->addr +
----------------
ruiu wrote:
> If you define new set of relocation types for bb sections, you can make it guaranteed that the relocations always point to the beginning of a section (because if there's a jump instruction jumping to a middle of a bb section, it is not really a BB).
Agreed, a new relocation type will simplify this.  I added a comment here.  Could we keep this temporarily until we can get a new relocation added?


================
Comment at: lld/ELF/Arch/X86_64.cpp:163
+
+static unsigned getJumpRelocationWithOffset(const InputSection &IS,
+                                            uint64_t Offset) {
----------------
MaskRay wrote:
> getJumpRelocationWithOffset is unused.
> 
> See Relocations.cpp:scanRelocs. A better way is to sort relocations beforehand.
Deleted.


================
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);
+}
----------------
MaskRay wrote:
> Neither _PC8 nor _PC32 is tested.
How do I test this?  LLVM uses PLT32 relocs for this.  Potentially, PC32 or PC8 can be used for this.


================
Comment at: lld/ELF/Arch/X86_64.cpp:258
+  const uint8_t *SecContents = IS.data().data();
+  if (!isDirectJmpInsnOpcode(SecContents + R.offset - 1))
+    return false;
----------------
MaskRay wrote:
> 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.
We check above that the size of the InputSection is atleast that of a direct jmp.  We also check the relocation 4 bytes from the end.  So, this out-of-bounds is guaranteed to not occur.


================
Comment at: lld/ELF/Arch/X86_64.cpp:260
+    return false;
+
+  if (isFallThruRelocation(IS, File, NextIS, R)) {
----------------
MaskRay wrote:
> 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?
If there are instructions between the jumps, the optimization is simply not valid.  The optimization is only valid for consecutive jumps like:

jne <next_BB>
jmp <far_BB>

which can be converted to

je <far_BB>
jmp <next_BB>


================
Comment at: lld/ELF/Arch/X86_64.cpp:277
+  unsigned RbIndex =
+      getRelocationWithOffset(IS, (IS.getSize() - SizeOfDirectJmpInsn - 4));
+  if (RbIndex == IS.relocations.size())
----------------
MaskRay wrote:
> ditto. An invalid r_offset can cause the read to go out-of-bounds.
Same argument here, cannot go out of bounds as we explicitly check the size of the Input Section in the above lines.


================
Comment at: lld/ELF/InputSection.cpp:869
 
+    if (expr == R_SIZE) {
+      target->relocateNoSym(bufLoc, type,
----------------
MaskRay wrote:
> How is `R_SIZE` used?
In cases where the range is represented in DWARF as start and length, we defer the length calculation to the link stage, by emitting an appropriate SIZE relocation instead of hardcoding the length directly in the object file by the compiler.



================
Comment at: lld/ELF/InputSection.cpp:946
   for (const Relocation &rel : relocations) {
+    if (rel.expr == R_NONE)
+      continue;
----------------
MaskRay wrote:
> After patching `expr` to R_NONE, propeller should delete these relocations. InputSection.cpp should not know R_NONE has to be skipped.
I assumed it would be helpful in general to avoid R_NONE relocations here. I dont mind deleting the R_NONE relocations with Propeller.


================
Comment at: lld/ELF/LTO.cpp:88
+      c.Options.BBSections = BasicBlockSection::None;
+    else {
+      llvm::propeller::getBBSectionsList(config->ltoBBSections,
----------------
MaskRay wrote:
> Check `config->ltoBBSections == "list"`
"list" is not an option but is specified as a file name with a list. The file contains a list of functions for which basic block sections must be generated.


================
Comment at: lld/ELF/OutputSections.cpp:346
+      // Check if this IS needs a special filler.
+      if (isec->SpecialFiller)
+        fill(start, end - start, *(isec->SpecialFiller));
----------------
ruiu wrote:
> This SpecialFiller is always NOP instructions, so how about adding just a boolean flag (e.g. `isec->fallthrough`) to an input section and remove this `SpecialFiller` vector?
Done.


================
Comment at: lld/ELF/Target.h:96
+
+  virtual unsigned growJmpInsn(InputSection &IS, InputFile *File,
+                               uint32_t MaxAlignment) const
----------------
MaskRay wrote:
> If growJmpInsn is X86 specific. Can it be moved to Arch/X86_64.cpp?
growJmpInsn has been removed from this patch and will be presented as a separate patch.


================
Comment at: lld/ELF/Writer.cpp:1647
+static void fixSymbolsAfterShrinking() {
+  for (InputFile *File : objectFiles) {
+    parallelForEach(File->getSymbols(), [&](Symbol *Sym) {
----------------
PkmX wrote:
> I don't think you need to loop over all input files, just files with sections that have been shrunk.
How would we determine that? We need to keep track of this somewhere.


================
Comment at: lld/ELF/Writer.cpp:1663
+
+      if (Def->value > NewSize) {
+        LLVM_DEBUG(llvm::dbgs() << "Moving symbol " << Sym->getName() <<
----------------
PkmX wrote:
> MaskRay wrote:
> > This is a dangerous operation. Are you arbitrarily changing st_value/st_size? In what conditions can this be triggered?
> I think having symbols in the part of section that is shrunk should just be an error, or at least it should clamp them to the end of a section.
This is called from optimizeBBJumps() and can only be triggered when the size of the section is shrunk from deleting jump instructions.  


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

https://reviews.llvm.org/D68065