RISCV: handle 64-bit PCREL data relocations

[compnerd]

Summary

We would previously fail to handle 64-bit PC-relative relocations on
RISCV.  This was exposed by trying to build with
`-fprofile-instr-generate`.

The original changes restricted the relocation handling to the text
segment as the paired relocations are undesirable in at least the debug
and .eh_frame sections. We now make this explicit to handle the general
case for the data relocations as well.

It would be preferable to use R_RISCV_n_PCREL when available to avoid
an extra relocation.

Fixes: #55971

Fixes: #55971

[MaskRay]

llvm/test/MC/RISCV/instr-profile.s

1 ↗

(On Diff #436068)

The test name should not attach to the specific instr-profile.s. It can be a generic filename indicating 64-bit PC-relative support. Other instrumentations may use 64-bit PCrel too.

[compnerd]

I give up; the extra context seems to not paste in properly - the reduced context shows the diff properly. I can re-upload the extra context variant but it will render improperly.

[compnerd]

Change filename as per @MaskRay's suggestion.

[craig.topper]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

236

Should relaxation here be relocation?

[compnerd]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

236

In the case of a compact code model, we can relax the pair of relocations to a single relocation. You would still have a relocation, it just is a nominally smaller file, and fewer relocations for the linker to process.

[craig.topper]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

236

I was asking about “Avoid relaxation for symbolic differences…”. Is relaxation the right word there?

[compnerd]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

236

Right; I believe that is the term here - we are technically relaxing the sequence by replacing the two relocations with one.

[luismarques]

In D127549#3574953, @compnerd wrote:

I give up; the extra context seems to not paste in properly - the reduced context shows the diff properly. I can re-upload the extra context variant but it will render improperly.

What exactly was the problem? I applied and regenerated the patch and uploaded it to Phabricator and it seemed fine. I tried it both as a file upload and as a paste.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

236

I've never seen "relaxation" being used that way. While that seems a reasonable generalization of how the term is typically used in compiler land, I suspect most people are going to be confused by the comment.

[luismarques]

I know that this issue arose in the context of -fprofile-instr-generate but can you please clean up the test to focus on the bug? Something like this:

	.section	sx,"aw",@progbits
	.globl	x
x:

	.section	sy,"aw",@progbits
	.globl	y
y:
	.8byte	x-y

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

232–234

This point is a bit of a nit-pick but that optimization isn't limited to when we are generating code for the compact code model. Rather, to define the compact code mode it's necessary to define that new relocation. Once we have support for that relocation (in the object code generator and the linker) we should be able to use it, whatever code model we are targeting. (The same point applies to the patch summary).

236–242

Are we testing all of these conditions?

[compnerd]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

232–234

I agree, it isn't limited to the compact code-model; were the relocation available in general, that would be preferable. I can try to reword this to make that more explicit.

236–242

This isn't new - this is the existing code path. I would assume that they are are being tested.

[compnerd]

Apply changes from feedback from @luismarques

[luismarques]

LGTM.

(don't forget to update the commit message)

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

232

Nit: maybe clarify that it's been proposed as part of the draft specs for the compact and large code models but not yet adopted. Otherwise people might not know what it's meant by "when available".

llvm/test/MC/RISCV/riscv64-64b-pcrel.s

2

Nit: wrap line

[MaskRay]

This is not ready to be pushed as is. I need some time to prepare a message.

[MaskRay]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

235

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.

llvm/test/MC/RISCV/riscv64-64b-pcrel.s

10

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.

[compnerd]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

235

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.

llvm/test/MC/RISCV/riscv64-64b-pcrel.s

10

Sorry, I don't necessarily understand the exact action to take here.

[MaskRay]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

235

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.

llvm/test/MC/RISCV/riscv64-64b-pcrel.s

10

Remove .global to test only the supported usage.

[compnerd]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

235

I do agree that doing a prefix-match for .debug is likely to be more conservative in what sections are excluded, but I am trying to be more conservative about generating the pairwise relocation. Avoiding the pair-wise relocations in non-SHF_ALLOC doesn't seem like a bad thing to me.

[MaskRay]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

235

That's fine with me, but IsDebugOrEHFrameSection needs to be renamed to reflect the truth.

[compnerd]

Address feedback from @MaskRay .

[MaskRay]

llvm/test/MC/RISCV/riscv64-64b-pcrel.s

21

Add a test using a non-SHF_ALLOC section.

[craig.topper]

It appears that this commit has prevented test/DebugInfo/Generic/accel-table-hash-collisions.ll from completing when the llc invocation is using a riscv triple.

llvm-dwarfdebug prints an endless stream of

Data 49 [                                                                  
  Atom[0]: Error extracting the value                                      
]                                                                          
Data 50 [                                                                  
  Atom[0]: Error extracting the value                                      
]                                                                          
Data 51 [                                                                  
  Atom[0]: Error extracting the value                                      
]                                                                          
Data 52 [                                                                  
  Atom[0]: Error extracting the value                                      
]                                                                          
Data 53 [                                                                  
  Atom[0]: Error extracting the value                                      
]

with numbers that just keep increasing

Maybe because a bunch of R_RISCV_ADD32 and R_RISCV_SUB32 relocations ends up in the .rela.apple_names section?

[craig.topper]

In D127549#3620940, @craig.topper wrote:

It appears that this commit has prevented test/DebugInfo/Generic/accel-table-hash-collisions.ll from completing when the llc invocation is using a riscv triple.

llvm-dwarfdebug prints an endless stream of

Data 49 [                                                                  
  Atom[0]: Error extracting the value                                      
]                                                                          
Data 50 [                                                                  
  Atom[0]: Error extracting the value                                      
]                                                                          
Data 51 [                                                                  
  Atom[0]: Error extracting the value                                      
]                                                                          
Data 52 [                                                                  
  Atom[0]: Error extracting the value                                      
]                                                                          
Data 53 [                                                                  
  Atom[0]: Error extracting the value                                      
]

with numbers that just keep increasing

Maybe because a bunch of R_RISCV_ADD32 and R_RISCV_SUB32 relocations ends up in the .rela.apple_names section?

A similar failure happens with llvm/test/DebugInfo/Generic/cross-cu-inlining.ll but for .apple_types section

[MaskRay]

In D127549#3620985, @craig.topper wrote:

In D127549#3620940, @craig.topper wrote:

It appears that this commit has prevented test/DebugInfo/Generic/accel-table-hash-collisions.ll from completing when the llc invocation is using a riscv triple.

llvm-dwarfdebug prints an endless stream of

Data 49 [                                                                  
  Atom[0]: Error extracting the value                                      
]                                                                          
Data 50 [                                                                  
  Atom[0]: Error extracting the value                                      
]                                                                          
Data 51 [                                                                  
  Atom[0]: Error extracting the value                                      
]                                                                          
Data 52 [                                                                  
  Atom[0]: Error extracting the value                                      
]                                                                          
Data 53 [                                                                  
  Atom[0]: Error extracting the value                                      
]

with numbers that just keep increasing

Maybe because a bunch of R_RISCV_ADD32 and R_RISCV_SUB32 relocations ends up in the .rela.apple_names section?

A similar failure happens with llvm/test/DebugInfo/Generic/cross-cu-inlining.ll but for .apple_types section

I am testing adding a hack: S.getName() == ".apple_names" || S.getName() == ".apple_types";

I am checking why they are not metadata.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

static std::pair<unsigned, unsigned> getRelocPairForSize(unsigned Size) {
     case 8:
       return std::make_pair(RISCV::fixup_riscv_add_64,
                             RISCV::fixup_riscv_sub_64);
     }
   }
 
   static bool requiresFixups(MCContext &C, const MCExpr *Value,
                              const MCExpr *&LHS, const MCExpr *&RHS) {
+    auto IsMetadataOrEHFrameSection = [](const MCSection &S) -> bool {
+      return S.getKind().isMetadata() || S.getName() == ".eh_frame";
+    };
+
     const auto *MBE = dyn_cast<MCBinaryExpr>(Value);
     if (MBE == nullptr)
       return false;
 
     MCValue E;
     if (!Value->evaluateAsRelocatable(E, nullptr, nullptr))
       return false;
     if (E.getSymA() == nullptr || E.getSymB() == nullptr)
       return false;
 
     const auto &A = E.getSymA()->getSymbol();
     const auto &B = E.getSymB()->getSymbol();
 
     LHS =
         MCBinaryExpr::create(MCBinaryExpr::Add, MCSymbolRefExpr::create(&A, C),
                              MCConstantExpr::create(E.getConstant(), C), C);
     RHS = E.getSymB();
 
-    return (A.isInSection() ? A.getSection().hasInstructions()
-                            : !A.getName().empty()) ||
-           (B.isInSection() ? B.getSection().hasInstructions()
-                            : !B.getName().empty());
+    // TODO: when available, R_RISCV_n_PCREL should be preferred.
+
+    // Avoid pairwise relocations for symbolic difference in debug and .eh_frame
+    if (A.isInSection())
+      return !IsMetadataOrEHFrameSection(A.getSection());
+    if (B.isInSection())
+      return !IsMetadataOrEHFrameSection(B.getSection());
+    // as well as for absolute symbols.
+    return !A.getName().empty() || !B.getName().empty();
   }
 
   void reset() override {
     static_cast<RISCVTargetStreamer *>(getTargetStreamer())->reset();
     MCELFStreamer::reset();
   }
 
 public:
   RISCVELFStreamer(MCContext &C, std::unique_ptr<MCAsmBackend> MAB,
                    std::unique_ptr<MCObjectWriter> MOW,

llvm/test/MC/RISCV/riscv64-64b-pcrel.s

+# RUN: llvm-mc -triple riscv64-unknown-linux-gnu -filetype obj -o - %s \
+# RUN:   | llvm-readobj -r - | FileCheck %s
+
+# CHECK: Relocations [
+# CHECK:  .relasx {
+# CHECK-NEXT:    0x0 R_RISCV_ADD64 y 0x0
+# CHECK-NEXT:    0x0 R_RISCV_SUB64 x 0x0
+# CHECK:  }
+# CHECK:  .relasy {
+# CHECK-NEXT:    0x0 R_RISCV_ADD64 x 0x0
+# CHECK-NEXT:    0x0 R_RISCV_SUB64 y 0x0
+# CHECK:  }
+# CHECK:  .relasz {
+# CHECK-NEXT:    0x0 R_RISCV_ADD64 z 0x0
+# CHECK-NEXT:    0x0 R_RISCV_SUB64 a 0x0
+# CHECK:  }
+# CHECK:  .relasa {
+# CHECK-NEXT:    0x0 R_RISCV_ADD64 a 0x0
+# CHECK-NEXT:    0x0 R_RISCV_SUB64 z 0x0
+# CHECK:  }
+# CHECK: ]
+
+  .section  sx,"aw",@progbits
+x:
+  .quad y-x
+
+  .section  sy,"aw",@progbits
+y:
+  .quad x-y
+
+  .section  sz
+z:
+  .quad z-a
+
+  .section  sa
+a:
+  .quad a-z