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

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd added inline comments.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:235
+    if (A.isInSection())
+      return !IsDebugOrEHFrameSection(A.getSection());
+    if (B.isInSection())
----------------
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.


================
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
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127549