[PATCH] D102712: BPF: Add more relocation kinds

[Yonghong Song via Phabricator via llvm-commits]

yonghong-song added inline comments.


================
Comment at: llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:954
   case ELF::R_BPF_NONE:
+  case ELF::R_BPF_64_ALLOC32:
     break;
----------------
MaskRay wrote:
> This needs an ExecutionEngine test. Otherwise, don't touch the file in this patch.
I will add a test for this.


================
Comment at: llvm/lib/Target/BPF/MCTargetDesc/BPFELFObjectWriter.cpp:45
     llvm_unreachable("invalid fixup kind!");
-  case FK_SecRel_8:
+  case FK_SecRel_4:
+    // LD_imm64 instruction.
----------------
MaskRay wrote:
> FK_SecRel_4 is used by PE-COFF to represent SECREL relocation types.
> 
> "The 32-bit offset of the target from the beginning of its section. This is used to support debugging information and static thread local storage."
> 
> Why does it map to a regular looking R_BPF_64_64?
In AMDGPU/MCTargetDesc/R600MCCodeEmitter.cpp, I see
```
Fixups.push_back(MCFixup::create(offset, MO.getExpr(), FK_SecRel_4, MI.getLoc()));
```
It looks like backend can choose create a MCFixup with FK_SecRel_4. Do we have restrictions that we have to use a different one? Use FK_SecRel_4 seems a good fit here as only 4 bytes are permitted to write anyway.


================
Comment at: llvm/lib/Target/BPF/MCTargetDesc/BPFELFObjectWriter.cpp:72
           if ((Flags & ELF::SHF_ALLOC) && (Flags & ELF::SHF_EXECINSTR))
-            return ELF::R_BPF_NONE;
+            return ELF::R_BPF_64_ALLOC32;
         } else {
----------------
MaskRay wrote:
> This doesn't make sense to me. The relocation type names shouldn't be aware of SHF_ALLOC/non-SHF_ALLOC.
I just wanted to choose a best-fit name. How about R_BPF_64_NODYLD_32 implying no runtime dynamic linking?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102712