clang crash assigning to a global named register variable #109778

[akshaykumars614]

fixed crash

I am not sure if the fix is correct. I tried to analyze the logic and found potential issue in the logic and corrected it. Because of this, two testcases;

arm64-named-reg-alloc.ll :
; RUN: not --crash llc < %s -mtriple=arm64-apple-darwin 2>&1 | FileCheck %s
; RUN: not --crash llc < %s -mtriple=arm64-linux-gnueabi 2>&1 | FileCheck %s

define i32 @get_stack() nounwind {
entry:
; FIXME: Include an allocatable-specific error message
; CHECK: Invalid register name "x5".
%sp = call i32 @llvm.read_register.i32(metadata !0)
ret i32 %sp
}

declare i32 @llvm.read_register.i32(metadata) nounwind

!0 = !{!"x5\00"}

aarch64-named-reg-x18.ll :
; RUN: llc -mtriple=aarch64-fuchsia -o - %s

define void @set_x18(i64 %x) {
entry:
; FIXME: Include an allocatable-specific error message
tail call void @llvm.write_register.i64(metadata !0, i64 %x)
ret void
}

declare void @llvm.write_register.i64(metadata, i64) nounwind

!0 = !{!"x18"}

I am new to project. Please help me here.

[llvmbot]

@llvm/pr-subscribers-backend-aarch64

Author: Akshay Kumar (akshaykumars614)

Changes

fixed crash

I am not sure if the fix is correct. I tried to analyze the logic and found potential issue in the logic and corrected it. Because of this, two testcases;

arm64-named-reg-alloc.ll :
; RUN: not --crash llc < %s -mtriple=arm64-apple-darwin 2>&1 | FileCheck %s
; RUN: not --crash llc < %s -mtriple=arm64-linux-gnueabi 2>&1 | FileCheck %s

define i32 @get_stack() nounwind {
entry:
; FIXME: Include an allocatable-specific error message
; CHECK: Invalid register name "x5".
%sp = call i32 @llvm.read_register.i32(metadata !0)
ret i32 %sp
}

declare i32 @llvm.read_register.i32(metadata) nounwind

!0 = !{!"x5\00"}

aarch64-named-reg-x18.ll :
; RUN: llc -mtriple=aarch64-fuchsia -o - %s

define void @set_x18(i64 %x) {
entry:
; FIXME: Include an allocatable-specific error message
tail call void @llvm.write_register.i64(metadata !0, i64 %x)
ret void
}

declare void @llvm.write_register.i64(metadata, i64) nounwind

!0 = !{!"x18"}

I am new to project. Please help me here.

---

Full diff: https://github.com/llvm/llvm-project/pull/113105.diff

1 Files Affected:

• (modified) llvm/lib/Target/AArch64/AArch64ISelLowering.cpp (+2-2)

diff --git a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
index 7448416c682abc..98703d8368813a 100644
--- a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
+++ b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
@@ -11522,8 +11522,8 @@ getRegisterByName(const char* RegName, LLT VT, const MachineFunction &MF) const
   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))
+    if (Subtarget->isXRegisterReserved(DwarfRegNum) ||
+        MRI->isReservedReg(MF, Reg))
       Reg = 0;
   }
   if (Reg)

[DavidSpickett]

Hi, I think I was the one who filed this issue in the first place. #109778.

We generally don't put the number of the issue being fixed in the title. Instead you can put Fixes #<issue number> in the PR description. https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

(and it is fine to do this even if you're not 100% sure that it does, because we can always edit the message later)

I am not sure if the fix is correct. I tried to analyze the logic and found potential issue in the logic and corrected it.

PR descriptions should explain the "why" of the change, at least as well as it is understood at the time of submission.

So for this change, what do you think the logic was doing, why was that incorrect and what did you change to fix that?

Not trying to trick you here, I don't know how the code works here either, first time reading it :) But without an attempt at an explanation, we cannot discern between a "fix", and simply avoiding the specific error that was reported. The latter may in fact make everything else worse if we do not think about it properly.

Because of this, two testcases;

Two testcases what? Do you mean you added those or that they failed because of the change, not sure. I assume you mean they failed, because I don't see any new tests added.

[DavidSpickett]

At least my own approach here would be to write a test case that matches the reported issue and find something that makes that new test pass while not breaking the other tests.

It's possible the existing tests are invalid, but I try to first make sure it is not my own understanding at fault.

[akshaykumars614]

Got it. I am digging deeper.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[akshaykumars614]

Sorry for the late submission. I was busy with the college examinations.
So, I analyzed further and found out the logic there was incorrect.
Subtarget->isXRegisterReserved(DwarfRegNum) checks if the register is reserved at subtarget level.
MRI->isReservedReg(MF, Reg) checks if the register is reserved at machine function level.

If either of the cases is true, we cannot use that register for general purpose use.

[DavidSpickett]

It was a bit mind bending for me too but I figured out what the code is doing you can see that in the comment above.

If you want to continue to do something for this issue perhaps you could port the nicer error reporting from llvm/lib/Target/RISCV/RISCVISelLowering.cpp into the AArch64 function.

If you want something to build on from there, try moving the check for whether the register is reserved into the target lowering base class so that everyone will get the more precise error message. Be aware that some targets may care about other things as well as the register being reserved.

You could look into plumbing to get the error all the way back to clang's frontend, but I suspect that there are structural issues that make that difficult so I wouldn't advise it.

[akshaykumars614]

Thank you for your feedback @DavidSpickett , I am understanding your approach. I would really like to do this issue. I will understand risc v error handling.

[akshaykumars614]

If you want to continue to do something for this issue perhaps you could port the nicer error reporting from llvm/lib/Target/RISCV/RISCVISelLowering.cpp into the AArch64 function.

I did this.

If you want something to build on from there, try moving the check for whether the register is reserved into the target lowering base class so that everyone will get the more precise error message. Be aware that some targets may care about other things as well as the register being reserved.

So you are saying instead of checking error for each individual targets, check in the Target Lowering base class itself?

[akshaykumars614]

@DavidSpickett , I would really appreciate your feedback.

[DavidSpickett]

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.

[DavidSpickett]

So you are saying instead of checking error for each individual targets, check in the Target Lowering base class itself?

Yes.

Though the more I look at this the more tricky this seems. With some targets separating ABI reserved registers and user reserved registers, but some don't. Improving the error message for one target is still an improvement though.