Skip to content

Conversation

apach301
Copy link
Contributor

@apach301 apach301 commented Sep 8, 2025

FIxes #157453

@llvmbot
Copy link
Member

llvmbot commented Sep 8, 2025

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-spirv

Author: Daniel Kuts (apach301)

Changes

FIxes #157453


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

1 Files Affected:

  • (modified) mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp (+2-2)
diff --git a/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp b/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp
index f99339852824c..7a6d4a212159b 100644
--- a/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp
+++ b/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp
@@ -727,9 +727,9 @@ void mlir::spirv::ConstantOp::getAsmResultNames(
       return setNameFn(getResult(), (intCst.getInt() ? "true" : "false"));
     }
 
-    if (intTy.isSignless()) {
+    if (intTy && intTy.isSignless()) {
       specialName << intCst.getInt();
-    } else if (intTy.isUnsigned()) {
+    } else if (intTy && intTy.isUnsigned()) {
       specialName << intCst.getUInt();
     } else {
       specialName << intCst.getSInt();

@kuhar kuhar changed the title [mlir] Check variable for null before dereferencing in Dialect/SPIRV/IR [mlir][spirv] Check variable for null before dereferencing Sep 8, 2025
Copy link
Member

@kuhar kuhar left a comment

Choose a reason for hiding this comment

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

I don't understand when this could happen. Could you add a lit test? If the value is an integer attr, the type must be either bool or int, no?

@apach301
Copy link
Contributor Author

apach301 commented Sep 8, 2025

I don't understand when this could happen. Could you add a lit test? If the value is an integer attr, the type must be either bool or int, no?

The problem is that intTy could be NULL after dynamic cast, there are checks in code for this (lines 726, 739), except these 2 cases (calling isSignless and isUnsigned). Can't add a test cause it was a static analyzer, I don't know how to trigger exact this code

@kuhar
Copy link
Member

kuhar commented Sep 8, 2025

I don't think it can inside the if statement that checks for IntegerAttr. If anything, we can probably remove the null check on line 726 (and maybe turn it into an assertion to be explicit).

Copy link
Member

@kuhar kuhar left a comment

Choose a reason for hiding this comment

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

LGTM if the CI passes

@kuhar kuhar requested a review from CoTinker September 10, 2025 13:34
@kuhar kuhar merged commit d070960 into llvm:main Sep 10, 2025
10 checks passed
@apach301 apach301 deleted the mlir-spirvops-null-dereference branch September 15, 2025 17:28
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.

Possible NULL dereference in mlir/Dialect/SPIRV/IR
4 participants