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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay requested changes to this revision.
MaskRay added a comment.
This revision now requires changes to proceed.

40+ comments have not been addressed. I am not catching up recent discussions but I believe there is still concern about the overall approach. Whether code should be implemented in CodeGenPrepare.cpp, etc.

Mark as "Request Changes" in fear of an accidental commit.



================
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);
+}
----------------
tmsriram wrote:
> 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.
```
.byte 0xe8
.long foo - . - 4
```


================
Comment at: lld/ELF/Arch/X86_64.cpp:41
                 uint64_t val) const override;
+  void applyJumpInstrMod(uint8_t *Loc, JumpModType Type,
+                         unsigned size) const override;
----------------
Fix parameter case.


================
Comment at: lld/ELF/Arch/X86_64.cpp:63
 
+static std::vector<std::vector<uint8_t>> X86_NOP_INSTRUCTIONS = {
+    {0x90},
----------------
const




================
Comment at: lld/ELF/Arch/X86_64.cpp:159
+    if (is.relocations[i].offset == offset && is.relocations[i].expr != R_NONE)
+      break;
+  }
----------------
In an old comment, I suggested

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

It is not addressed.


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

https://reviews.llvm.org/D68065