[llvm][SystemZ] Fix parsing of .cfi_undefined with percent-less registers.
#107032
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-backend-systemz @llvm/pr-subscribers-mc Author: Alex Rønne Petersen (alexrp) ChangesThis is just e3d658b applied to SystemZ. An example of this being used in the wild: https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/s390/s390-64/start.S;h=59eeb7e998227bdf32029cd074f0876c450404ea;hb=HEAD#l63 Full diff: https://github.com/llvm/llvm-project/pull/107032.diff 3 Files Affected:
diff --git a/llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp b/llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp
index f2c04215d12dda..2ad87b51b57401 100644
--- a/llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp
+++ b/llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp
@@ -757,25 +757,28 @@ void SystemZOperand::print(raw_ostream &OS) const {
// Parse one register of the form %<prefix><number>.
bool SystemZAsmParser::parseRegister(Register &Reg, bool RestoreOnFailure) {
- Reg.StartLoc = Parser.getTok().getLoc();
-
- // Eat the % prefix.
- if (Parser.getTok().isNot(AsmToken::Percent))
- return Error(Parser.getTok().getLoc(), "register expected");
const AsmToken &PercentTok = Parser.getTok();
- Parser.Lex();
+ bool HasPercent = PercentTok.is(AsmToken::Percent);
+
+ Reg.StartLoc = PercentTok.getLoc();
+
+ // If we encounter a %, ignore it. This code handles registers with and
+ // without the prefix, unprefixed registers can occur in cfi directives.
+ if (HasPercent) {
+ Parser.Lex(); // Eat percent token.
+ }
// Expect a register name.
if (Parser.getTok().isNot(AsmToken::Identifier)) {
- if (RestoreOnFailure)
+ if (RestoreOnFailure && HasPercent)
getLexer().UnLex(PercentTok);
- return Error(Reg.StartLoc, "invalid register");
+ return Error(Reg.StartLoc, HasPercent ? "invalid register" : "register expected");
}
// Check that there's a prefix.
StringRef Name = Parser.getTok().getString();
if (Name.size() < 2) {
- if (RestoreOnFailure)
+ if (RestoreOnFailure && HasPercent)
getLexer().UnLex(PercentTok);
return Error(Reg.StartLoc, "invalid register");
}
@@ -783,7 +786,7 @@ bool SystemZAsmParser::parseRegister(Register &Reg, bool RestoreOnFailure) {
// Treat the rest of the register name as a register number.
if (Name.substr(1).getAsInteger(10, Reg.Num)) {
- if (RestoreOnFailure)
+ if (RestoreOnFailure && HasPercent)
getLexer().UnLex(PercentTok);
return Error(Reg.StartLoc, "invalid register");
}
@@ -800,7 +803,7 @@ bool SystemZAsmParser::parseRegister(Register &Reg, bool RestoreOnFailure) {
else if (Prefix == 'c' && Reg.Num < 16)
Reg.Group = RegCR;
else {
- if (RestoreOnFailure)
+ if (RestoreOnFailure && HasPercent)
getLexer().UnLex(PercentTok);
return Error(Reg.StartLoc, "invalid register");
}
diff --git a/llvm/test/MC/SystemZ/regs-bad.s b/llvm/test/MC/SystemZ/regs-bad.s
index 320cba0fc856c1..6392ff2863002e 100644
--- a/llvm/test/MC/SystemZ/regs-bad.s
+++ b/llvm/test/MC/SystemZ/regs-bad.s
@@ -262,8 +262,6 @@
# Test general register parsing, with no predetermined class in mind.
#
-#CHECK: error: register expected
-#CHECK: .cfi_offset r0,0
#CHECK: error: invalid register
#CHECK: .cfi_offset %,0
#CHECK: error: invalid register
@@ -289,7 +287,6 @@
#CHECK: error: invalid register
#CHECK: .cfi_offset %arid,0
- .cfi_offset r0,0
.cfi_offset %,0
.cfi_offset %r,0
.cfi_offset %f,0
diff --git a/llvm/test/MC/SystemZ/regs-good.s b/llvm/test/MC/SystemZ/regs-good.s
index b4c1edd1b591e7..49bf8a48ca4c2c 100644
--- a/llvm/test/MC/SystemZ/regs-good.s
+++ b/llvm/test/MC/SystemZ/regs-good.s
@@ -176,6 +176,70 @@
st 0, 4095(1,15)
st 0, 4095(15,1)
+#CHECK: .cfi_offset %r0, 0
+#CHECK: .cfi_offset %r1, 8
+#CHECK: .cfi_offset %r2, 16
+#CHECK: .cfi_offset %r3, 24
+#CHECK: .cfi_offset %r4, 32
+#CHECK: .cfi_offset %r5, 40
+#CHECK: .cfi_offset %r6, 48
+#CHECK: .cfi_offset %r7, 56
+#CHECK: .cfi_offset %r8, 64
+#CHECK: .cfi_offset %r9, 72
+#CHECK: .cfi_offset %r10, 80
+#CHECK: .cfi_offset %r11, 88
+#CHECK: .cfi_offset %r12, 96
+#CHECK: .cfi_offset %r13, 104
+#CHECK: .cfi_offset %r14, 112
+#CHECK: .cfi_offset %r15, 120
+#CHECK: .cfi_offset %f0, 128
+#CHECK: .cfi_offset %f1, 136
+#CHECK: .cfi_offset %f2, 144
+#CHECK: .cfi_offset %f3, 152
+#CHECK: .cfi_offset %f4, 160
+#CHECK: .cfi_offset %f5, 168
+#CHECK: .cfi_offset %f6, 176
+#CHECK: .cfi_offset %f7, 184
+#CHECK: .cfi_offset %f8, 192
+#CHECK: .cfi_offset %f9, 200
+#CHECK: .cfi_offset %f10, 208
+#CHECK: .cfi_offset %f11, 216
+#CHECK: .cfi_offset %f12, 224
+#CHECK: .cfi_offset %f13, 232
+#CHECK: .cfi_offset %f14, 240
+#CHECK: .cfi_offset %f15, 248
+#CHECK: .cfi_offset %a0, 256
+#CHECK: .cfi_offset %a1, 260
+#CHECK: .cfi_offset %a2, 264
+#CHECK: .cfi_offset %a3, 268
+#CHECK: .cfi_offset %a4, 272
+#CHECK: .cfi_offset %a5, 276
+#CHECK: .cfi_offset %a6, 280
+#CHECK: .cfi_offset %a7, 284
+#CHECK: .cfi_offset %a8, 288
+#CHECK: .cfi_offset %r9, 292
+#CHECK: .cfi_offset %a10, 296
+#CHECK: .cfi_offset %a11, 300
+#CHECK: .cfi_offset %a12, 304
+#CHECK: .cfi_offset %a13, 308
+#CHECK: .cfi_offset %a14, 312
+#CHECK: .cfi_offset %a15, 316
+#CHECK: .cfi_offset %c0, 318
+#CHECK: .cfi_offset %c1, 326
+#CHECK: .cfi_offset %c2, 334
+#CHECK: .cfi_offset %c3, 342
+#CHECK: .cfi_offset %c4, 350
+#CHECK: .cfi_offset %c5, 358
+#CHECK: .cfi_offset %c6, 366
+#CHECK: .cfi_offset %c7, 374
+#CHECK: .cfi_offset %c8, 382
+#CHECK: .cfi_offset %c9, 390
+#CHECK: .cfi_offset %c10, 398
+#CHECK: .cfi_offset %c11, 406
+#CHECK: .cfi_offset %c12, 414
+#CHECK: .cfi_offset %c13, 422
+#CHECK: .cfi_offset %c14, 430
+#CHECK: .cfi_offset %c15, 438
#CHECK: .cfi_offset %r0, 0
#CHECK: .cfi_offset %r1, 8
#CHECK: .cfi_offset %r2, 16
@@ -306,4 +370,68 @@
.cfi_offset %c13,422
.cfi_offset %c14,430
.cfi_offset %c15,438
+ .cfi_offset r0,0
+ .cfi_offset r1,8
+ .cfi_offset r2,16
+ .cfi_offset r3,24
+ .cfi_offset r4,32
+ .cfi_offset r5,40
+ .cfi_offset r6,48
+ .cfi_offset r7,56
+ .cfi_offset r8,64
+ .cfi_offset r9,72
+ .cfi_offset r10,80
+ .cfi_offset r11,88
+ .cfi_offset r12,96
+ .cfi_offset r13,104
+ .cfi_offset r14,112
+ .cfi_offset r15,120
+ .cfi_offset f0,128
+ .cfi_offset f1,136
+ .cfi_offset f2,144
+ .cfi_offset f3,152
+ .cfi_offset f4,160
+ .cfi_offset f5,168
+ .cfi_offset f6,176
+ .cfi_offset f7,184
+ .cfi_offset f8,192
+ .cfi_offset f9,200
+ .cfi_offset f10,208
+ .cfi_offset f11,216
+ .cfi_offset f12,224
+ .cfi_offset f13,232
+ .cfi_offset f14,240
+ .cfi_offset f15,248
+ .cfi_offset a0,256
+ .cfi_offset a1,260
+ .cfi_offset a2,264
+ .cfi_offset a3,268
+ .cfi_offset a4,272
+ .cfi_offset a5,276
+ .cfi_offset a6,280
+ .cfi_offset a7,284
+ .cfi_offset a8,288
+ .cfi_offset r9,292
+ .cfi_offset a10,296
+ .cfi_offset a11,300
+ .cfi_offset a12,304
+ .cfi_offset a13,308
+ .cfi_offset a14,312
+ .cfi_offset a15,316
+ .cfi_offset c0,318
+ .cfi_offset c1,326
+ .cfi_offset c2,334
+ .cfi_offset c3,342
+ .cfi_offset c4,350
+ .cfi_offset c5,358
+ .cfi_offset c6,366
+ .cfi_offset c7,374
+ .cfi_offset c8,382
+ .cfi_offset c9,390
+ .cfi_offset c10,398
+ .cfi_offset c11,406
+ .cfi_offset c12,414
+ .cfi_offset c13,422
+ .cfi_offset c14,430
+ .cfi_offset c15,438
.cfi_endproc
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
alexrp
force-pushed
the
llvm-s390x-cfi-percent
branch
from
2759925
to
4ec3c17
Compare
uweigand
requested changes
Sep 3, 2024
There was a problem hiding this comment.
This doesn't look right. I agree that .cfi directives should support percent-less registers to match GAS behavior, but this patch would accept them in a lot of other places.
Note that this changes the SystemZAsmParser::parseRegister(Register &Reg, bool RestoreOnFailure) routine, which is called internally from various places - in some of those, accepting percent-less register names would definitely be wrong (e.g. when parsing an index register in an address).
I think all callers need to be reviewed, and explicitly check for percent where needed (or add another parameter to this routine to specific whether or not percent needs to be mandatory).
…ters. This is just e3d658b applied to SystemZ.
alexrp
force-pushed
the
llvm-s390x-cfi-percent
branch
from
4ec3c17
to
263df4f
Compare
|
Ok, this iteration of the patch rejects percent-less registers as instruction operands and in addresses as before. It only allows omitting the percent token in the overridden |
VitaNuo
pushed a commit
to VitaNuo/llvm-project
that referenced
this pull request
Sep 12, 2024
…isters. (llvm#107032) This is just e3d658b applied to SystemZ. An example of this being used in the wild: https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/s390/s390-64/start.S;h=59eeb7e998227bdf32029cd074f0876c450404ea;hb=HEAD#l63
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is just e3d658b applied to SystemZ.
An example of this being used in the wild: https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/s390/s390-64/start.S;h=59eeb7e998227bdf32029cd074f0876c450404ea;hb=HEAD#l63