[MC][CodeGen] Define R_RISCV_PLT32 and lower dso_local_equivalent to it

[leonardchan]

Summary

This introduces R_RISCV_PLT32, PC-relative data relocation that takes the 32-bit relative offset to a function or its PLT entry from its relocation location.

This is needed to support relative vtables on RISCV.

Github PR: https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/363

The lld handling of this reloc is D143115.

[mcgrathr]

LGTM pending acceptance of the spec change.

llvm/test/MC/RISCV/elf-reloc-plt32.s

5

This label is unused.

[jrtc27]

Title should say [MC] not [llvm]. Do we not also need CodeGen support for this to be useful outside of hand-written assembly?

llvm/test/MC/RISCV/elf-reloc-plt32.s

1–2

[MaskRay]

llvm/test/MC/RISCV/elf-reloc-plt32.s

1–2

This is a difference in taste. In MC and lld tests, backslash at the previous line is pretty common.

[jrtc27]

llvm/test/MC/RISCV/elf-reloc-plt32.s

1–2

In a tree I have lying around from August there are only 4 instances of \| \\$ in llvm/test/MC/RISCV but 1088 of : *\|

[leonardchan]

In D143226#4101389, @jrtc27 wrote:

Title should say [MC] not [llvm]. Do we not also need CodeGen support for this to be useful outside of hand-written assembly?

CodeGen already picks this up via the PLTRelativeVariantKind = MCSymbolRefExpr::VK_PLT;. A dso_local_equivalent @func gets lowered to func@PLT.

[jrtc27]

In D143226#4103521, @leonardchan wrote:

In D143226#4101389, @jrtc27 wrote:

Title should say [MC] not [llvm]. Do we not also need CodeGen support for this to be useful outside of hand-written assembly?

CodeGen already picks this up via the PLTRelativeVariantKind = MCSymbolRefExpr::VK_PLT;. A dso_local_equivalent @func gets lowered to func@PLT.

Then this should be [MC][CodeGen] and have an IR test for it

[leonardchan]

In D143226#4103526, @jrtc27 wrote:

In D143226#4103521, @leonardchan wrote:

In D143226#4101389, @jrtc27 wrote:

Title should say [MC] not [llvm]. Do we not also need CodeGen support for this to be useful outside of hand-written assembly?

CodeGen already picks this up via the PLTRelativeVariantKind = MCSymbolRefExpr::VK_PLT;. A dso_local_equivalent @func gets lowered to func@PLT.

Then this should be [MC][CodeGen] and have an IR test for it

Done

[jrtc27]

llvm/test/CodeGen/RISCV/dso_local_equivalent.ll

1 ↗

(On Diff #494737)

Use update_llc_test_checks.py

4 ↗

(On Diff #494737)

If we're producing assembly that we test in an MC test we don't need this. Generally best to not do object file tests in CodeGen.

9 ↗

(On Diff #494737)

This isn't PC-relative

[leonardchan]

llvm/test/CodeGen/RISCV/dso_local_equivalent.ll

9 ↗

(On Diff #494737)

Updated with pc-relative codegen

[jrtc27]

llvm/test/CodeGen/RISCV/dso_local_equivalent.ll

1–11 ↗

(On Diff #494746)

Hm, seems update_llc_test_checks.py can't handle globals, just uses an empty dict :(

In that case I think you want this cleaned up version of your hand-written test.

Ideally we'd also have an RV32 test, but unifying them is a bit awkward due to the trunc needing to not exist. Technically you can do an oversized ptrtoint on RV32 (or undersized ptrtoint on RV64 and forego the trunc on both) but that's not what Clang (currently) generates. I do wonder why it doesn't just do an i32 ptrtoint unconditionally though...

llvm/test/MC/RISCV/elf-reloc-plt32.s

5

Just .data?

[MaskRay]

This introduces R_RISCV_PLT32, a pc-relative relocation that takes the offset to a function (or its plt entry) from the reloc's location.

My suggested wording on the PR uses proper cases: PC-relative and PLT, and relocation location.

llvm/test/CodeGen/RISCV/dso_local_equivalent.ll

6 ↗

(On Diff #494758)

You can add {{$}} to the end to prevent such a mistake or switch to FileCheck --match-full-lines

1 ↗

(On Diff #494737)

I think it is fine not using update_llc_test_checks.py. The test clearly doesn't cause more maintenance trouble.

[jrtc27]

llvm/test/CodeGen/RISCV/dso_local_equivalent.ll

6 ↗

(On Diff #494758)

Sorry, my bad, I meant to say CHECK-NEXT here but forgot to alter what I copied...

[MaskRay]

A subject like Define R_RISCV_PLT32 and lower dso_local_equivalent to it may be clearer than Introduce handling for R_RISCV_PLT32 relocation

[asb]

I wanted to raise something that came to mind looking at this patch, but is probably also relevant for the other recent Fuchsia related patches: typically (certainly for the llvm/test/CodeGen/RISCV and llvm/test/MC/RISCV) we try to add RV32 CHECK lines alongside RV64 lines wherever relevant. This is typically cheap to do, and of course provides just a little extra coverage, even in cases where the test isn't particularly targeted at an RV32 use case.

So the two thoughts are:

• Adding rv32 lines to tests like in this patch
• Whether the same should be done for other fuchsia related patches, and relatedly if -mtriple=riscv32-fuchsia is something that isn't likely to exist and we don't want to bother testing for it, would it be better to just disallow the triple?

[leonardchan]

In D143226#4109617, @asb wrote:

I wanted to raise something that came to mind looking at this patch, but is probably also relevant for the other recent Fuchsia related patches: typically (certainly for the llvm/test/CodeGen/RISCV and llvm/test/MC/RISCV) we try to add RV32 CHECK lines alongside RV64 lines wherever relevant. This is typically cheap to do, and of course provides just a little extra coverage, even in cases where the test isn't particularly targeted at an RV32 use case.

So the two thoughts are:

• Adding rv32 lines to tests like in this patch
• Whether the same should be done for other fuchsia related patches, and relatedly if -mtriple=riscv32-fuchsia is something that isn't likely to exist and we don't want to bother testing for it, would it be better to just disallow the triple?

I'm fine with adding RV32 cases in this patch and other patches related to this if it's trivial. For fuchsia-related changes though, I think the 32-bit tests might not be necessary since Fuchsia is exclusively 64-bit so we could disallow that in a separate patch maybe.

[asb]

In D143226#4111496, @leonardchan wrote:

For fuchsia-related changes though, I think the 32-bit tests might not be necessary since Fuchsia is exclusively 64-bit so we could disallow that in a separate patch maybe.

That would be great. There are enough valid/expected options to worry about that I'm in quite favour of erroring out early for things that we don't think make sense.

[leonardchan]

@jrtc27 any comments on this before landing?

[asb]

In D143226#4116086, @asb wrote:

In D143226#4111496, @leonardchan wrote:

For fuchsia-related changes though, I think the 32-bit tests might not be necessary since Fuchsia is exclusively 64-bit so we could disallow that in a separate patch maybe.

That would be great. There are enough valid/expected options to worry about that I'm in quite favour of erroring out early for things that we don't think make sense.

Just a friendly ping on this - follow-up patches to gate off rv32 fuchsia (and any other functionality introduced for fuchsia but isn't expected to work on rv32) would be appreciated.

[leonardchan]

Just a friendly ping on this - follow-up patches to gate off rv32 fuchsia (and any other functionality introduced for fuchsia but isn't expected to work on rv32) would be appreciated.

Sorry for the delay. D144998 adds this.

llvm/include/llvm/BinaryFormat/ELFRelocs/RISCV.def

 ELF_RELOC(R_RISCV_RELAX,             51)
 ELF_RELOC(R_RISCV_SUB6,              52)
 ELF_RELOC(R_RISCV_SET6,              53)
 ELF_RELOC(R_RISCV_SET8,              54)
 ELF_RELOC(R_RISCV_SET16,             55)
 ELF_RELOC(R_RISCV_SET32,             56)
 ELF_RELOC(R_RISCV_32_PCREL,          57)
 ELF_RELOC(R_RISCV_IRELATIVE,         58)
+ELF_RELOC(R_RISCV_PLT32,             59)

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

 //===-- RISCVELFObjectWriter.cpp - RISCV ELF Writer -----------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "MCTargetDesc/RISCVFixupKinds.h"
 #include "MCTargetDesc/RISCVMCExpr.h"
 #include "MCTargetDesc/RISCVMCTargetDesc.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCELFObjectWriter.h"
 #include "llvm/MC/MCFixup.h"
 #include "llvm/MC/MCObjectWriter.h"
+#include "llvm/MC/MCValue.h"
 #include "llvm/Support/ErrorHandling.h"
 
 using namespace llvm;
 
 namespace {
 class RISCVELFObjectWriter : public MCELFObjectTargetWriter {
 public:
   RISCVELFObjectWriter(uint8_t OSABI, bool Is64Bit);
if (Kind >= FirstLiteralRelocationKind)
     return Kind - FirstLiteralRelocationKind;
   if (IsPCRel) {
     switch (Kind) {
     default:
       Ctx.reportError(Fixup.getLoc(), "Unsupported relocation type");
       return ELF::R_RISCV_NONE;
     case FK_Data_4:
     case FK_PCRel_4:
-      return ELF::R_RISCV_32_PCREL;
+      return Target.getAccessVariant() == MCSymbolRefExpr::VK_PLT
+                 ? ELF::R_RISCV_PLT32
+                 : ELF::R_RISCV_32_PCREL;
     case RISCV::fixup_riscv_pcrel_hi20:
       return ELF::R_RISCV_PCREL_HI20;
     case RISCV::fixup_riscv_pcrel_lo12_i:
       return ELF::R_RISCV_PCREL_LO12_I;
     case RISCV::fixup_riscv_pcrel_lo12_s:
       return ELF::R_RISCV_PCREL_LO12_S;
     case RISCV::fixup_riscv_got_hi20:
       return ELF::R_RISCV_GOT_HI20;

llvm/lib/Target/RISCV/RISCVTargetObjectFile.cpp

 #include "llvm/MC/MCSectionELF.h"
 
 using namespace llvm;
 
 void RISCVELFTargetObjectFile::Initialize(MCContext &Ctx,
                                           const TargetMachine &TM) {
   TargetLoweringObjectFileELF::Initialize(Ctx, TM);
 
+  PLTRelativeVariantKind = MCSymbolRefExpr::VK_PLT;
+
   SmallDataSection = getContext().getELFSection(
       ".sdata", ELF::SHT_PROGBITS, ELF::SHF_WRITE | ELF::SHF_ALLOC);
   SmallBSSSection = getContext().getELFSection(".sbss", ELF::SHT_NOBITS,
                                                ELF::SHF_WRITE | ELF::SHF_ALLOC);
 }
 
 // A address must be loaded from a small section if its size is less than the
 // small section size threshold. Data in this section could be addressed by

llvm/test/MC/RISCV/elf-reloc-plt32.s

+# RUN: llvm-mc -triple=riscv64 -filetype=obj %s -o - | \
+# RUN:   llvm-readobj -r - | FileCheck %s
+
+  .section .data
+this:
+  .word extern_func@PLT - . + 4
+
+# CHECK:      Section ({{.*}}) .rela.data {
+# CHECK-NEXT:   0x0 R_RISCV_PLT32 extern_func 0x4
+# CHECK-NEXT: }