[RISCV] Support assembling @plt symbol operands

[lewis-revill]

Summary

This patch allows symbols appended with @plt to parse and assemble with the R_RISCV_CALL_PLT relocation.

[rogfer01]

Would it make sense to add an operand that allows a bare symbol or bare symbol + @plt and use it only in CALL and TAIL instead of allowing identifier@plt everywhere?

Perhaps that'd be too restrictive and prevent legitimate references to @plt references in other instructions?

I did something like this in my downstream in RISCVInstrInfo.td

def BareSymbolOrPlt : AsmOperandClass {
  let Name = "BareSymbolOrPlt";
  let RenderMethod = "addImmOperands";
  let DiagnosticType = "InvalidBareSymbolOrPlt";
  let ParserMethod = "parseBareSymbolOrPlt";
}

// A bare symbol optionally decorated with @plt.
def bare_symbol_or_plt : Operand<XLenVT> {
  let ParserMatchClass = BareSymbolOrPlt;
  let MCOperandPredicate = [{
     return MCOp.isBareSymbolRef();
  }];
}

This will require new functions in RISCVAsmParser.cpp and additional diagnostic handling but nothing too onerous.

And then in CALL (similarly thing for TAIL) in RISCVInstrInfo.td

let isCall = 1, Defs = [X1], isCodeGenOnly = 0, Size = 8 in
def PseudoCALL : Pseudo<(outs), (ins bare_symbol_or_plt:$func),
                        [(Call tglobaladdr:$func)]> {
  let AsmString = "call\t$func";
}

If this is feasible, a nice feature is that we can give a slightly better diagnostic.

Thoughts?

[lewis-revill]

I agree that it would be good to prevent other instructions using '@plt' (this was mentioned in the original MC+codegen patch). The difficulty is that getParser().parseIdentifier consumes the '@' as a valid character. This would mean that the new parseBareSymbolOrPLT would be trivial to do from what I have implemented here but a check would have to be added in parseBareSymbol for whether the identifier ends in '@plt'. The other possibility is to keep the logic in 'parseBareSymbol' and if it is an identifier with '@plt' then it should check Operands[0] for 'call' or 'tail', which doesn't seem very nice to me.

[rogfer01]

Ah of course. I hadn't thought of that case. I guess we don't expect symbols to end in @plt as part of their name, do we?

[lewis-revill]

Ideally no. I guess it would be good to error when parsing rather than trying to emit a R_RISCV_CALL_PLT relocation for a non-call... I'm leaning towards checking Operands[0] at the moment but it depends what others think.

[lewis-revill]

Rebased with updated dependency and added checking for tail or call instruction mnemonic in Operands[0] when parsing '@plt' symbols, with appropriate tests added. This is just a proof of concept for this implementation.

[jrtc27]

In D55335#1320688, @lewis-revill wrote:

Ideally no. I guess it would be good to error when parsing rather than trying to emit a R_RISCV_CALL_PLT relocation for a non-call... I'm leaning towards checking Operands[0] at the moment but it depends what others think.

I personally don't particularly like having parseBareSymbol change behaviour based on the instruction name and would prefer to have a separate parser. You can then make parseBareSymbol reject anything with an @ in it, and parseCallTarget (or whatever you want to name it) reject anything with an @ in it after stripping off the optional @plt.

Other than the minor points given here, it seems good, hopefully we can get an amended version merged soon!

lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp

115 ↗

(On Diff #177019)

Isn't this just copying Expr if it's a RISCVMCExpr (assuming Ctx ends up being the same context)? If so, we can just re-use Expr for that case. But thinking about it further, can we not make sure that PseudoCALL/PseudoTAIL always get a RISCVMCExpr, either with a VK_RISCV_CALL or a VK_RISCV_CALL_PLT, so everywhere is always explicit about which it wants rather than having an implicit fallback?

lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp

44 ↗

(On Diff #177019)

Personally I'd move this to before the closing ')'. I know the two are mutually exclusive, but to me looking at the code it just feels slightly wrong that the @plt suffix isn't obviously attached directly to the symbol.

test/MC/RISCV/tail-call.s

54 ↗

(On Diff #177019)

Please include the trailing newline

[lewis-revill]

lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp

115 ↗

(On Diff #177019)

I agree, it would be a better way to do it. This confirms your opinion about adding a new operand for call/tail so we can then add VK_RISCV_CALL/VK_RISCV_CALL_PLT when parsing it. I think what I'd need to do first is to create a patch implementing that, IE adding a CallSymbol operand, changing parsing/codegen to attach VK_RISCV_CALL to the RISCVMCExpr, which expandFunctionCall should just emit as is. After I've done that I could make the change in this patch & the codegen patch to incorporate that..

[lewis-revill]

Address minor issues.

[lewis-revill]

Rebased removing unnecessary dependencies and to use the cleaner CallSymbol patch.

[lewis-revill]

Rebase following update to D55560.

[lewis-revill]

Rebased.

[lewis-revill]

Rebased with updated dependency

[asb]

Looks good to me, thanks.

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

public:
 
   bool isCallSymbol() const {
     int64_t Imm;
     RISCVMCExpr::VariantKind VK;
     // Must be of 'immediate' type but not a constant.
     if (!isImm() || evaluateConstantImm(getImm(), Imm, VK))
       return false;
     return RISCVAsmParser::classifySymbolRef(getImm(), VK, Imm) &&
-           VK == RISCVMCExpr::VK_RISCV_CALL;
+           (VK == RISCVMCExpr::VK_RISCV_CALL ||
+            VK == RISCVMCExpr::VK_RISCV_CALL_PLT);
   }
 
   bool isCSRSystemRegister() const { return isSystemRegister(); }
 
   /// Return true if the operand is a valid for the fence instruction e.g.
   /// ('iorw').
   bool isFenceArg() const {
     if (!isImm())
if (getLexer().getKind() != AsmToken::Identifier)
     return MatchOperand_NoMatch;
 
   StringRef Identifier;
   AsmToken Tok = getLexer().getTok();
 
   if (getParser().parseIdentifier(Identifier))
     return MatchOperand_ParseFail;
 
+  if (Identifier.consume_back("@plt")) {
+    Error(getLoc(), "'@plt' operand not valid for instruction");
+    return MatchOperand_ParseFail;
+  }
+
   MCSymbol *Sym = getContext().getOrCreateSymbol(Identifier);
 
   if (Sym->isVariable()) {
     const MCExpr *V = Sym->getVariableValue(/*SetUsed=*/false);
     if (!isa<MCSymbolRefExpr>(V)) {
       getLexer().UnLex(Tok); // Put back if it's not a bare symbol.
       return MatchOperand_NoMatch;
     }
OperandMatchResultTy RISCVAsmParser::parseCallSymbol(OperandVector &Operands) {
 
   if (getLexer().getKind() != AsmToken::Identifier)
     return MatchOperand_NoMatch;
 
   StringRef Identifier;
   if (getParser().parseIdentifier(Identifier))
     return MatchOperand_ParseFail;
 
+  RISCVMCExpr::VariantKind Kind = RISCVMCExpr::VK_RISCV_CALL;
+  if (Identifier.consume_back("@plt"))
+    Kind = RISCVMCExpr::VK_RISCV_CALL_PLT;
+
   MCSymbol *Sym = getContext().getOrCreateSymbol(Identifier);
   Res = MCSymbolRefExpr::create(Sym, MCSymbolRefExpr::VK_None, getContext());
-  Res = RISCVMCExpr::create(Res, RISCVMCExpr::VK_RISCV_CALL, getContext());
+  Res = RISCVMCExpr::create(Res, Kind, getContext());
   Operands.push_back(RISCVOperand::createImm(Res, S, E, isRV64()));
   return MatchOperand_Success;
 }
 
 OperandMatchResultTy RISCVAsmParser::parseJALOffset(OperandVector &Operands) {
   // Parsing jal operands is fiddly due to the `jal foo` and `jal ra, foo`
   // both being acceptable forms. When parsing `jal ra, foo` this function
   // will be called for the `ra` register operand in an attempt to match the

llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h

const static MCFixupKindInfo Infos[] = {
       { "fixup_riscv_pcrel_lo12_i", 20,     12,  MCFixupKindInfo::FKF_IsPCRel },
       { "fixup_riscv_pcrel_lo12_s",  0,     32,  MCFixupKindInfo::FKF_IsPCRel },
       { "fixup_riscv_got_hi20",     12,     20,  MCFixupKindInfo::FKF_IsPCRel },
       { "fixup_riscv_jal",          12,     20,  MCFixupKindInfo::FKF_IsPCRel },
       { "fixup_riscv_branch",        0,     32,  MCFixupKindInfo::FKF_IsPCRel },
       { "fixup_riscv_rvc_jump",      2,     11,  MCFixupKindInfo::FKF_IsPCRel },
       { "fixup_riscv_rvc_branch",    0,     16,  MCFixupKindInfo::FKF_IsPCRel },
       { "fixup_riscv_call",          0,     64,  MCFixupKindInfo::FKF_IsPCRel },
+      { "fixup_riscv_call_plt",      0,     64,  MCFixupKindInfo::FKF_IsPCRel },
       { "fixup_riscv_relax",         0,      0,  0 },
       { "fixup_riscv_align",         0,      0,  0 }
     };
     static_assert((array_lengthof(Infos)) == RISCV::NumTargetFixupKinds,
                   "Not all fixup kinds added to Infos array");
 
     if (Kind < FirstTargetFixupKind)
       return MCAsmBackend::getFixupKindInfo(Kind);

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

case RISCV::fixup_riscv_branch: {
     unsigned Lo4 = (Value >> 1) & 0xf;
     // Inst{31} = Sbit;
     // Inst{30-25} = Mid6;
     // Inst{11-8} = Lo4;
     // Inst{7} = Hi1;
     Value = (Sbit << 31) | (Mid6 << 25) | (Lo4 << 8) | (Hi1 << 7);
     return Value;
   }
-  case RISCV::fixup_riscv_call: {
+  case RISCV::fixup_riscv_call:
+  case RISCV::fixup_riscv_call_plt: {
     // Jalr will add UpperImm with the sign-extended 12-bit LowerImm,
     // we need to add 0x800ULL before extract upper bits to reflect the
     // effect of the sign extension.
     uint64_t UpperImm = (Value + 0x800ULL) & 0xfffff000ULL;
     uint64_t LowerImm = Value & 0xfffULL;
     return UpperImm | ((LowerImm << 20) << 32);
   }
   case RISCV::fixup_riscv_rvc_jump: {

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

unsigned RISCVELFObjectWriter::getRelocType(MCContext &Ctx,
   case RISCV::fixup_riscv_branch:
     return ELF::R_RISCV_BRANCH;
   case RISCV::fixup_riscv_rvc_jump:
     return ELF::R_RISCV_RVC_JUMP;
   case RISCV::fixup_riscv_rvc_branch:
     return ELF::R_RISCV_RVC_BRANCH;
   case RISCV::fixup_riscv_call:
     return ELF::R_RISCV_CALL;
+  case RISCV::fixup_riscv_call_plt:
+    return ELF::R_RISCV_CALL_PLT;
   case RISCV::fixup_riscv_relax:
     return ELF::R_RISCV_RELAX;
   case RISCV::fixup_riscv_align:
     return ELF::R_RISCV_ALIGN;
   }
 }
 
 std::unique_ptr<MCObjectTargetWriter>
 llvm::createRISCVELFObjectWriter(uint8_t OSABI, bool Is64Bit) {
   return llvm::make_unique<RISCVELFObjectWriter>(OSABI, Is64Bit);
 }

llvm/lib/Target/RISCV/MCTargetDesc/RISCVFixupKinds.h

enum Fixups {
   // compressed jump instruction
   fixup_riscv_rvc_jump,
   // fixup_riscv_rvc_branch - 8-bit fixup for symbol references in the
   // compressed branch instruction
   fixup_riscv_rvc_branch,
   // fixup_riscv_call - A fixup representing a call attached to the auipc
   // instruction in a pair composed of adjacent auipc+jalr instructions.
   fixup_riscv_call,
+  // fixup_riscv_call_plt - A fixup representing a procedure linkage table call
+  // attached to the auipc instruction in a pair composed of adjacent auipc+jalr
+  // instructions.
+  fixup_riscv_call_plt,
   // fixup_riscv_relax - Used to generate an R_RISCV_RELAX relocation type,
   // which indicates the linker may relax the instruction pair.
   fixup_riscv_relax,
   // fixup_riscv_align - Used to generate an R_RISCV_ALIGN relocation type,
   // which indicates the linker should fixup the alignment after linker
   // relaxation.
   fixup_riscv_align,
 
   // fixup_riscv_invalid - used as a sentinel and a marker, must be last fixup
   fixup_riscv_invalid,
   NumTargetFixupKinds = fixup_riscv_invalid - FirstTargetFixupKind
 };
 } // end namespace RISCV
 } // end namespace llvm
 
 #endif

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

case RISCVMCExpr::VK_RISCV_PCREL_HI:
       break;
     case RISCVMCExpr::VK_RISCV_GOT_HI:
       FixupKind = RISCV::fixup_riscv_got_hi20;
       break;
     case RISCVMCExpr::VK_RISCV_CALL:
       FixupKind = RISCV::fixup_riscv_call;
       RelaxCandidate = true;
       break;
+    case RISCVMCExpr::VK_RISCV_CALL_PLT:
+      FixupKind = RISCV::fixup_riscv_call_plt;
+      RelaxCandidate = true;
+      break;
     }
   } else if (Kind == MCExpr::SymbolRef &&
              cast<MCSymbolRefExpr>(Expr)->getKind() == MCSymbolRefExpr::VK_None) {
     if (Desc.getOpcode() == RISCV::JAL) {
       FixupKind = RISCV::fixup_riscv_jal;
     } else if (MIFrm == RISCVII::InstFormatB) {
       FixupKind = RISCV::fixup_riscv_branch;
     } else if (MIFrm == RISCVII::InstFormatCJ) {

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.h

public:
   enum VariantKind {
     VK_RISCV_None,
     VK_RISCV_LO,
     VK_RISCV_HI,
     VK_RISCV_PCREL_LO,
     VK_RISCV_PCREL_HI,
     VK_RISCV_GOT_HI,
     VK_RISCV_CALL,
+    VK_RISCV_CALL_PLT,
     VK_RISCV_Invalid
   };
 
 private:
   const MCExpr *Expr;
   const VariantKind Kind;
 
   int64_t evaluateAsInt64(int64_t Value) const;

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

 #define DEBUG_TYPE "riscvmcexpr"
 
 const RISCVMCExpr *RISCVMCExpr::create(const MCExpr *Expr, VariantKind Kind,
                                        MCContext &Ctx) {
   return new (Ctx) RISCVMCExpr(Expr, Kind);
 }
 
 void RISCVMCExpr::printImpl(raw_ostream &OS, const MCAsmInfo *MAI) const {
-  bool HasVariant =
-      ((getKind() != VK_RISCV_None) && (getKind() != VK_RISCV_CALL));
+  VariantKind Kind = getKind();
+  bool HasVariant = ((Kind != VK_RISCV_None) && (Kind != VK_RISCV_CALL) &&
+                     (Kind != VK_RISCV_CALL_PLT));
+
   if (HasVariant)
     OS << '%' << getVariantKindName(getKind()) << '(';
   Expr->print(OS, MAI);
+  if (Kind == VK_RISCV_CALL_PLT)
+    OS << "@plt";
   if (HasVariant)
     OS << ')';
 }
 
 const MCFixup *RISCVMCExpr::getPCRelHiFixup() const {
   MCValue AUIPCLoc;
   if (!getSubExpr()->evaluateAsRelocatable(AUIPCLoc, nullptr, nullptr))
     return nullptr;
case VK_RISCV_GOT_HI:
     return "got_pcrel_hi";
   }
 }
 
 bool RISCVMCExpr::evaluateAsConstant(int64_t &Res) const {
   MCValue Value;
 
   if (Kind == VK_RISCV_PCREL_HI || Kind == VK_RISCV_PCREL_LO ||
-      Kind == VK_RISCV_GOT_HI || Kind == VK_RISCV_CALL)
+      Kind == VK_RISCV_GOT_HI || Kind == VK_RISCV_CALL ||
+      Kind == VK_RISCV_CALL_PLT)
     return false;
 
   if (!getSubExpr()->evaluateAsRelocatable(Value, nullptr, nullptr))
     return false;
 
   if (!Value.isAbsolute())
     return false;
 

llvm/test/MC/RISCV/function-call.s

 # INSTR: jalr  ra
 # FIXUP: fixup A - offset: 0, value: ra, kind: fixup_riscv_call
 
 call mstatus
 # RELOC: R_RISCV_CALL mstatus 0x0
 # INSTR: auipc ra, 0
 # INSTR: jalr  ra
 # FIXUP: fixup A - offset: 0, value: mstatus, kind: fixup_riscv_call
+
+# Ensure that calls to procedure linkage table symbols work.
+
+call foo@plt
+# RELOC: R_RISCV_CALL_PLT foo 0x0
+# INSTR: auipc ra, 0
+# INSTR: jalr  ra
+# FIXUP: fixup A - offset: 0, value: foo@plt, kind: fixup_riscv_call_plt

llvm/test/MC/RISCV/lla-invalid.s

 # RUN: not llvm-mc -triple=riscv32 < %s 2>&1 | FileCheck %s
 # RUN: not llvm-mc -triple=riscv64 < %s 2>&1 | FileCheck %s
 
 # Non bare symbols must be rejected
 lla a2, %lo(a_symbol) # CHECK: :[[@LINE]]:9: error: operand must be a bare symbol name
 lla a2, %hi(a_symbol) # CHECK: :[[@LINE]]:9: error: operand must be a bare symbol name
+lla a2, foo@plt # CHECK: :[[@LINE]]:17: error: '@plt' operand not valid for instruction

llvm/test/MC/RISCV/tail-call.s

 # INSTR: jr  t1
 # FIXUP: fixup A - offset: 0, value: f1, kind:
 
 tail ra
 # RELOC: R_RISCV_CALL ra 0x0
 # INSTR: auipc t1, 0
 # INSTR: jr  t1
 # FIXUP: fixup A - offset: 0, value: ra, kind:
+
+tail foo@plt
+# RELOC: R_RISCV_CALL_PLT foo 0x0
+# INSTR: auipc t1, 0
+# INSTR: jr  t1
+# FIXUP: fixup A - offset: 0, value: foo@plt, kind: