This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Support assembling @plt symbol operands
ClosedPublic

Authored by lewis-revill on Dec 5 2018, 12:08 PM.

Details

Summary

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

Diff Detail

Event Timeline

lewis-revill created this revision.Dec 5 2018, 12:08 PM

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?

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.

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?

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.

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 requested changes to this revision.Dec 7 2018, 6:30 AM

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

This revision now requires changes to proceed.Dec 7 2018, 6:30 AM
lewis-revill added inline comments.Dec 7 2018, 1:48 PM
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..

Address minor issues.

lewis-revill marked 2 inline comments as done.Dec 7 2018, 2:06 PM

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

Rebase following update to D55560.

lewis-revill retitled this revision from [RISCV, WIP] Support assembling @plt symbol operands to [RISCV] Support assembling @plt symbol operands.

Rebased.

Rebased with updated dependency

Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2019, 2:07 AM
asb accepted this revision.Apr 2 2019, 5:32 AM

Looks good to me, thanks.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 2 2019, 5:46 AM
This revision was automatically updated to reflect the committed changes.