-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[RISCV] Replace @plt/@gotpcrel in data directives with %pltpcrel %gotpcrel #132569
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
00dc73e
fd883ab
4ae2528
57958b3
0b2468d
3f025af
21624dd
eb49642
d97c1a2
e820a82
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -97,4 +97,4 @@ branch: | |
# PC-NOT: .plt: | ||
# PLT: .plt: | ||
|
||
.word target@plt - . | ||
.word %plt(target - .) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1194,18 +1194,34 @@ const MCExpr *TargetLoweringObjectFileELF::lowerRelativeReference( | |
MCSymbolRefExpr::create(TM.getSymbol(RHS), getContext()), getContext()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} | ||
|
||
// Reference the function or its PLT entry (`Equiv`), optionally with a | ||
// subtrahend (`RHS`). | ||
const MCExpr *TargetLoweringObjectFileELF::lowerDSOLocalEquivalent( | ||
const DSOLocalEquivalent *Equiv, const TargetMachine &TM) const { | ||
const DSOLocalEquivalent *Equiv, const MCSymbolRefExpr *RHS, | ||
const TargetMachine &TM) const { | ||
assert(supportDSOLocalEquivalentLowering()); | ||
|
||
// If GV is dso_local, reference the function itself. Return GV or GV-RHS. | ||
const auto *GV = Equiv->getGlobalValue(); | ||
|
||
// A PLT entry is not needed for dso_local globals. | ||
const MCExpr *Res = MCSymbolRefExpr::create(TM.getSymbol(GV), getContext()); | ||
if (RHS) | ||
Res = MCBinaryExpr::createSub(Res, RHS, getContext()); | ||
if (GV->isDSOLocal() || GV->isImplicitDSOLocal()) | ||
return MCSymbolRefExpr::create(TM.getSymbol(GV), getContext()); | ||
|
||
return MCSymbolRefExpr::create(TM.getSymbol(GV), PLTRelativeSpecifier, | ||
getContext()); | ||
return Res; | ||
|
||
// Otherwise, reference the PLT. Return a relocatable expression with the PLT | ||
// specifier, %plt(GV) or %plt(GV-RHS). | ||
Res = createTargetMCExpr(Res, PLTRelativeSpecifier); | ||
if (Res) | ||
return Res; | ||
|
||
// If the target only supports the legacy syntax @plt, return GV@plt or | ||
// GV@plt - RHS. | ||
Res = MCSymbolRefExpr::create(TM.getSymbol(GV), PLTRelativeSpecifier, | ||
getContext()); | ||
if (RHS) | ||
Res = MCBinaryExpr::createSub(Res, RHS, getContext()); | ||
return Res; | ||
} | ||
|
||
MCSection *TargetLoweringObjectFileELF::getSectionForCommandLines() const { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -199,7 +199,7 @@ class RISCVAsmParser : public MCTargetAsmParser { | |
ParseStatus parseRegister(OperandVector &Operands, bool AllowParens = false); | ||
ParseStatus parseMemOpBaseReg(OperandVector &Operands); | ||
ParseStatus parseZeroOffsetMemOp(OperandVector &Operands); | ||
ParseStatus parseOperandWithModifier(OperandVector &Operands); | ||
ParseStatus parseOperandWithSpecifier(OperandVector &Operands); | ||
ParseStatus parseBareSymbol(OperandVector &Operands); | ||
ParseStatus parseCallSymbol(OperandVector &Operands); | ||
ParseStatus parsePseudoJumpSymbol(OperandVector &Operands); | ||
|
@@ -225,6 +225,8 @@ class RISCVAsmParser : public MCTargetAsmParser { | |
} | ||
|
||
bool parseOperand(OperandVector &Operands, StringRef Mnemonic); | ||
bool parseExprWithSpecifier(const MCExpr *&Res, SMLoc &E); | ||
bool parseDataExpr(const MCExpr *&Res) override; | ||
|
||
bool parseDirectiveOption(); | ||
bool parseDirectiveAttribute(); | ||
|
@@ -1746,7 +1748,7 @@ bool RISCVAsmParser::matchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode, | |
case Match_InvalidSImm12: | ||
return generateImmOutOfRangeError( | ||
Operands, ErrorInfo, -(1 << 11), (1 << 11) - 1, | ||
"operand must be a symbol with %lo/%pcrel_lo/%tprel_lo modifier or an " | ||
"operand must be a symbol with %lo/%pcrel_lo/%tprel_lo specifier or an " | ||
"integer in the range"); | ||
case Match_InvalidSImm12Lsb0: | ||
return generateImmOutOfRangeError( | ||
|
@@ -1765,17 +1767,19 @@ bool RISCVAsmParser::matchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode, | |
Operands, ErrorInfo, -(1 << 15), (1 << 15) - 1, | ||
"immediate must be non-zero in the range"); | ||
case Match_InvalidUImm20LUI: | ||
return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 20) - 1, | ||
"operand must be a symbol with " | ||
"%hi/%tprel_hi modifier or an integer in " | ||
"the range"); | ||
return generateImmOutOfRangeError( | ||
Operands, ErrorInfo, 0, (1 << 20) - 1, | ||
"operand must be a symbol with " | ||
"%hi/%tprel_hi specifier or an integer in " | ||
"the range"); | ||
case Match_InvalidUImm20: | ||
return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 20) - 1); | ||
case Match_InvalidUImm20AUIPC: | ||
return generateImmOutOfRangeError( | ||
Operands, ErrorInfo, 0, (1 << 20) - 1, | ||
"operand must be a symbol with a " | ||
"%pcrel_hi/%got_pcrel_hi/%tls_ie_pcrel_hi/%tls_gd_pcrel_hi modifier or " | ||
"%pcrel_hi/%got_pcrel_hi/%tls_ie_pcrel_hi/%tls_gd_pcrel_hi specifier " | ||
"or " | ||
"an integer in the range"); | ||
case Match_InvalidSImm21Lsb0JAL: | ||
return generateImmOutOfRangeError( | ||
|
@@ -2220,38 +2224,51 @@ ParseStatus RISCVAsmParser::parseImmediate(OperandVector &Operands) { | |
return ParseStatus::Failure; | ||
break; | ||
case AsmToken::Percent: | ||
return parseOperandWithModifier(Operands); | ||
return parseOperandWithSpecifier(Operands); | ||
} | ||
|
||
Operands.push_back(RISCVOperand::createImm(Res, S, E, isRV64())); | ||
return ParseStatus::Success; | ||
} | ||
|
||
ParseStatus RISCVAsmParser::parseOperandWithModifier(OperandVector &Operands) { | ||
ParseStatus RISCVAsmParser::parseOperandWithSpecifier(OperandVector &Operands) { | ||
SMLoc S = getLoc(); | ||
SMLoc E; | ||
|
||
if (parseToken(AsmToken::Percent, "expected '%' for operand modifier")) | ||
return ParseStatus::Failure; | ||
if (!parseOptionalToken(AsmToken::Percent)) | ||
return Error(getLoc(), "expected '%' relocation specifier"); | ||
lenary marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const MCExpr *Expr = nullptr; | ||
bool Failed = parseExprWithSpecifier(Expr, E); | ||
if (!Failed) | ||
Operands.push_back(RISCVOperand::createImm(Expr, S, E, isRV64())); | ||
return Failed; | ||
} | ||
|
||
bool RISCVAsmParser::parseExprWithSpecifier(const MCExpr *&Res, SMLoc &E) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be great to move these all to ParseStatus, but this PR isn't the right moment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I understand it, |
||
if (getLexer().getKind() != AsmToken::Identifier) | ||
return Error(getLoc(), "expected valid identifier for operand modifier"); | ||
return Error(getLoc(), "expected '%' relocation specifier"); | ||
StringRef Identifier = getParser().getTok().getIdentifier(); | ||
auto Spec = RISCVMCExpr::getSpecifierForName(Identifier); | ||
if (!Spec) | ||
return Error(getLoc(), "unrecognized operand modifier"); | ||
return Error(getLoc(), "invalid relocation specifier"); | ||
|
||
getParser().Lex(); // Eat the identifier | ||
if (parseToken(AsmToken::LParen, "expected '('")) | ||
return ParseStatus::Failure; | ||
return true; | ||
|
||
const MCExpr *SubExpr; | ||
if (getParser().parseParenExpression(SubExpr, E)) | ||
return ParseStatus::Failure; | ||
return true; | ||
|
||
const MCExpr *ModExpr = RISCVMCExpr::create(SubExpr, *Spec, getContext()); | ||
Operands.push_back(RISCVOperand::createImm(ModExpr, S, E, isRV64())); | ||
return ParseStatus::Success; | ||
Res = RISCVMCExpr::create(SubExpr, *Spec, getContext()); | ||
return false; | ||
} | ||
|
||
bool RISCVAsmParser::parseDataExpr(const MCExpr *&Res) { | ||
SMLoc E; | ||
if (parseOptionalToken(AsmToken::Percent)) | ||
return parseExprWithSpecifier(Res, E); | ||
return getParser().parseExpression(Res); | ||
} | ||
|
||
ParseStatus RISCVAsmParser::parseBareSymbol(OperandVector &Operands) { | ||
|
@@ -3732,7 +3749,7 @@ bool RISCVAsmParser::checkPseudoAddTPRel(MCInst &Inst, | |
if (Inst.getOperand(2).getReg() != RISCV::X4) { | ||
SMLoc ErrorLoc = ((RISCVOperand &)*Operands[3]).getStartLoc(); | ||
return Error(ErrorLoc, "the second input operand must be tp/x4 when using " | ||
"%tprel_add modifier"); | ||
"%tprel_add specifier"); | ||
} | ||
|
||
return false; | ||
|
@@ -3745,7 +3762,7 @@ bool RISCVAsmParser::checkPseudoTLSDESCCall(MCInst &Inst, | |
if (Inst.getOperand(0).getReg() != RISCV::X5) { | ||
SMLoc ErrorLoc = ((RISCVOperand &)*Operands[3]).getStartLoc(); | ||
return Error(ErrorLoc, "the output operand must be t0/x5 when using " | ||
"%tlsdesc_call modifier"); | ||
"%tlsdesc_call specifier"); | ||
} | ||
|
||
return false; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would
%plt(foo) - .
not be the saner syntax? PLT of an offset is a bit nonsensical...Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The core principle is that relocation specifiers must operate on the full expression, not just a part of it. (semantically closer to gas https://maskray.me/blog/2025-03-16-relocation-generation-in-assemblers#gnu-assembler-internals)
This would avoid a lot of complexity/ambiguity in parsing and expression folding.
.word %plt(foo - .)
might look unusual. It describes a PC-relative relocatable expression.We evaluate it to a relocatable expression (
relocation_specifier(sym_a - sym_b + offset)
), and allows the referenced "addend" (sym_a) to be a PLT..word %plt(foo)
describes an absolute reference to foo's PLT, which we don't support.Branch instructions have an implicit PC-relative operand, so
call foo@plt
(legacy syntax) could be viewed ascall %plt(foo)
(conceptually, we shouldn't add the syntax), which is internallyconverted to PC-relative as
%plt(foo - .)
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I know, but it's pretty weird and confusing syntax. It's not really written that way because it makes sense, it's just written that way because it aligns with how implementations think about it.
Perhaps %pltpcrel, to mirror %gotpcrel, would be the right thing to do here that sidesteps the issue?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's challenging to use an inherent PC-relative specifier (e.g.
%pltpcrel
; which I actually thought about).... This is becausein the code block around
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:3477
(AsmPrinter::lowerConstant
), the function doesn't actually know its current address. The function call needs a subtrahend. (llvm/test/CodeGen/X86/x86-64-plt-relative-reloc.ll
contains a better test that demonstrates this: the subtrahendvtable
might be a few bytes before the current address (that feature might be obsoleted by the dsolocal equivalent feature))(If we adopt
%pltpcrel
and if we ever decide to support.word %plt(foo)
, we would introduce another specifier. While with%plt(foo - .)
, we won't need a new specifier.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have %(got_)pcrel_hi and now %gotpcrel, what's so different about this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I don't like
%plt(foo - .)
, because the thing being put into the instruction is really the difference between foo's PLT entry's address, and the current address (rather than a PLT entry for the difference between foo and the current address). I would much prefer%pltpcrel(foo)
if%plt(foo)-.
is not a direction you are happy with, given the existing%gotpcrel(foo)
means the difference between the current address and foo's GOT entry's address.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The IR doesn't model the current location (DOT). Instead, It computes
SymA-SymB+offset
.SymB
might not be the current location, but it and the current location must be in the same section.Let's use
llvm/test/CodeGen/X86/x86-64-plt-relative-reloc.ll
(legacy; but slightly better thandso_local_equivalent.ll
):If we need to emit
%pltpcrel
, we will have to changeAsmPrinter::lowerConstant
to know the base GlobalVariable, which might be possible...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well my overarching point would be that user-facing syntax should not be beholden to arbitrary historic implementation choices. If it's truly impossible to make it work then that's one thing, but I doubt that to be the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented
%pltpcrel
, while a bit complex, I am happy with the result. (As inRISCVMCExpr::evaluateAsRelocatableImpl
, we don't need an exception forVK_PLT
, if we go with%plt
. This might be difficult to explain without playing with it.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've not looked at the implementation in detail, but thank you for taking the time to do so, I know from experience it can be quite painful getting the MC code to do things that weren't previously expected of it