Skip to content

clang crash assigning to a global named register variable #109778 #113105

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.

Sign up for GitHub

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

Closed
wants to merge 7 commits into from
+15 −12
Closed

clang crash assigning to a global named register variable #109778 #113105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ecbf43e
clang crash assigning to a global named register variable #109778
akshaykumars614 Oct 20, 2024
b8cd6f2
fixes #109778
akshaykumars614 Oct 31, 2024
24822dc
fixes 109778
akshaykumars614 Oct 31, 2024
6499176
corrected the format. fixes 109778
akshaykumars614 Oct 31, 2024
8446c63
fixes #109778
akshaykumars614 Nov 9, 2024
8e59839
fixes #109778
akshaykumars614 Nov 9, 2024
99d202e
fixes #109778
akshaykumars614 Nov 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 8 additions & 11 deletions llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11519,17 +11519,14 @@ SDValue AArch64TargetLowering::LowerSPONENTRY(SDValue Op,
Register AArch64TargetLowering::
getRegisterByName(const char* RegName, LLT VT, const MachineFunction &MF) const {
Register Reg = MatchRegisterName(RegName);
if (AArch64::X1 <= Reg && Reg <= AArch64::X28) {
const AArch64RegisterInfo *MRI = Subtarget->getRegisterInfo();
unsigned DwarfRegNum = MRI->getDwarfRegNum(Reg, false);
if (!Subtarget->isXRegisterReserved(DwarfRegNum) &&
!MRI->isReservedReg(MF, Reg))
Reg = 0;
}
if (Reg)
return Reg;
report_fatal_error(Twine("Invalid register name \""
+ StringRef(RegName) + "\"."));
if (Reg == AArch64::NoRegister)
report_fatal_error(
Twine("Invalid register name \"" + StringRef(RegName) + "\"."));
BitVector ReservedRegs = Subtarget->getRegisterInfo()->getReservedRegs(MF);
if (!ReservedRegs.test(Reg) && !Subtarget->isRegisterReservedByUser(Reg))
Copy link
Collaborator

@DavidSpickett DavidSpickett Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how other targets do it but AArch64 does not separate user reserved vs. reserved for ABI reasons (x18 is usually reserved for the OS to use).

I think you can still improve the error message but it'll need to check ReserveXRegister.

The tell tale sign this doesn't work is that nothing in AArch64 writes to UserReservedRegister but in RISCV and M68K they do (find in files or grep in llvm/, even if you don't understand what they're doing, it'll tell you something interacts with it at least).

I don't think it's worth moving AArch64 to the same style just for a better error though.

I think the tests pass because for AArch64 -ffixed-xN does set a bit in ReservedRegs.test and so the fact that isRegisterReservedByUser will always be False doesn't change anything.

I think if this becomes:
if (!ReservedRegs.test(Reg))

That should work. Then remove the new bitset you added.

Another way to do this is to keep the original code but instead of:

if (!Subtarget->isXRegisterReserved(DwarfRegNum) &&
        !MRI->isReservedReg(MF, Reg))
      Reg = 0;

report_fatal_error here with the improved message.

report_fatal_error(Twine("Trying to obtain non-reserved register \"" +
StringRef(RegName) + "\"."));
return Reg;
}

SDValue AArch64TargetLowering::LowerADDROFRETURNADDR(SDValue Op,
Expand Down
6 changes: 6 additions & 0 deletions llvm/lib/Target/AArch64/AArch64Subtarget.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ class AArch64Subtarget final : public AArch64GenSubtargetInfo {
unsigned MinimumJumpTableEntries = 4;
unsigned MaxJumpTableSize = 0;

std::bitset<AArch64::NUM_TARGET_REGS> UserReservedRegister;

// ReserveXRegister[i] - X#i is not available as a general purpose register.
BitVector ReserveXRegister;

Expand Down Expand Up @@ -208,6 +210,10 @@ class AArch64Subtarget final : public AArch64GenSubtargetInfo {
return MinVectorRegisterBitWidth;
}

bool isRegisterReservedByUser(Register i) const {
assert(i < AArch64::NUM_TARGET_REGS && "Register out of range");
return UserReservedRegister[i];
}
bool isXRegisterReserved(size_t i) const { return ReserveXRegister[i]; }
bool isXRegisterReservedForRA(size_t i) const { return ReserveXRegisterForRA[i]; }
unsigned getNumXRegisterReserved() const {
Expand Down
2 changes: 1 addition & 1 deletion llvm/test/CodeGen/AArch64/arm64-named-reg-alloc.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
define i32 @get_stack() nounwind {
entry:
; FIXME: Include an allocatable-specific error message
; CHECK: Invalid register name "x5".
; CHECK: Trying to obtain non-reserved register "x5".
%sp = call i32 @llvm.read_register.i32(metadata !0)
ret i32 %sp
}
Expand Down
Loading