[PATCH] D127549: RISCV: handle 64-bit PCREL data relocations

Mon Jun 13 16:24:22 PDT 2022

MaskRay added inline comments.

================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:235
+    if (A.isInSection())
+      return !IsDebugOrEHFrameSection(A.getSection());
+    if (B.isInSection())
----------------
compnerd wrote:
> MaskRay wrote:
> > isInSection() checks defined non-absolute symbols. I think all the conditions here are to make MCDwarf.cpp use constants/32_PCREL if applicable.
> > 
> > Can `S.getKind().isMetadata()` unnecessarily match non-MCDwarf created sections?
> > 
> > Note that we have other functions (e.g. `isTemporary`) which are usable but I haven't thought hard for the best condition here.
> I don't think that `isTemporary` works.  `isMetadata` shouldn't include anything that is marked as progbits or is part of the text or data segments.
If the condition is match MCDwarf.cpp produced sections, testing section prefix `.debug` will be more specific.

isMetadata() catches a lot of non-debug non-SHF_ALLOC sections.

================
Comment at: llvm/test/MC/RISCV/riscv64-64b-pcrel.s:9
+# CHECK:  .relasy {
+# CHECK-NEXT:    0x0 R_RISCV_ADD64 x 0x0
+# CHECK-NEXT:    0x0 R_RISCV_SUB64 y 0x0
----------------
compnerd wrote:
> MaskRay wrote:
> > ADD/SUB are for labels. The definition isn't clear in the psABI, but in spirit they should be https://sourceware.org/binutils/docs/as/Symbol-Names.html local labels. At the very least they need to be STB_LOCAL.
> > 
> > Supporting STB_GLOBAL is fine as GNU as supports such subtraction for STB_GLOBAL for various ports but they can use a comment.
> Sorry, I don't necessarily understand the exact action to take here.
Remove `.global` to test only the supported usage.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127549