Skip to content
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

Add non-null check before accessing pointer #83459

Merged
merged 2 commits into from
Mar 7, 2024

Conversation

MartinWehking
Copy link
Contributor

Add a check if RC is not null to ensure that a consecutive access is safe.

A static analyzer flagged this issue since hasVectorRegisters potentially dereferences RC.

Add a check if RC is not null to ensure that a consecutive access
is safe.

A static analyzer flagged this issue since hasVectorRegisters
potentially dereferences RC.
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 29, 2024

@llvm/pr-subscribers-backend-amdgpu

Author: Martin Wehking (MartinWehking)

Changes

Add a check if RC is not null to ensure that a consecutive access is safe.

A static analyzer flagged this issue since hasVectorRegisters potentially dereferences RC.


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

1 Files Affected:

  • (modified) llvm/lib/Target/AMDGPU/SIFoldOperands.cpp (+1-1)
diff --git a/llvm/lib/Target/AMDGPU/SIFoldOperands.cpp b/llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
index 634b4aeb30a730..1cef4f1fa81e3a 100644
--- a/llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
+++ b/llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
@@ -1054,7 +1054,7 @@ void SIFoldOperands::foldOperand(
       // Don't fold if OpToFold doesn't hold an aligned register.
       const TargetRegisterClass *RC =
           TRI->getRegClassForReg(*MRI, OpToFold.getReg());
-      if (TRI->hasVectorRegisters(RC) && OpToFold.getSubReg()) {
+      if (RC && TRI->hasVectorRegisters(RC) && OpToFold.getSubReg()) {
         unsigned SubReg = OpToFold.getSubReg();
         if (const TargetRegisterClass *SubRC =
                 TRI->getSubRegisterClass(RC, SubReg))

@@ -1054,7 +1054,7 @@ void SIFoldOperands::foldOperand(
// Don't fold if OpToFold doesn't hold an aligned register.
const TargetRegisterClass *RC =
TRI->getRegClassForReg(*MRI, OpToFold.getReg());
if (TRI->hasVectorRegisters(RC) && OpToFold.getSubReg()) {
if (RC && TRI->hasVectorRegisters(RC) && OpToFold.getSubReg()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no context that can reach here where this can fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is getRegClassForReg always returning a non-null pointer for every possible context?
When looking at the body of the function:
return Reg.isVirtual() ? MRI.getRegClass(Reg) : getPhysRegBaseClass(Reg);
I spotted that getPhysRegBaseClass(Reg) might return a nullptr.

Copy link
Contributor

Choose a reason for hiding this comment

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

This would fail for unallocatable physical registers which shouldn't have been copy folding candidates to begin with

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, then we can close this PR again

Copy link
Contributor

Choose a reason for hiding this comment

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

Will adding an assert work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes an assert should also silence the issue:1af318e

@MartinWehking MartinWehking reopened this Mar 6, 2024
@arsenm arsenm merged commit 2484680 into llvm:main Mar 7, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants