[analyzer] Fix getElementRegion to retain address space information

[vabridgers]

This change modifies getElementRegion to retain address space information when removing all QualType qualifiers. Always removing address space qualifiers can expose cases where QualTypes for different pointer types may be different depending on how the QualTypes are obtained.

clang: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:785:
void assertEqualBitWidths(clang::ento::ProgramStateRef, clang::ento::Loc,
clang::ento::Loc): Assertion `RhsBitwidth == LhsBitwidth &&
"RhsLoc and LhsLoc bitwidth must be same!"' failed.

#0 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (clang-21)
...
#7 (anonymous namespace)::SimpleSValBuilder::evalBinOpLL(llvm::IntrusiveRefCntPtr<clang::ento::ProgramState const>,
clang::BinaryOperatorKind, clang::ento::Loc, clang::ento::Loc, clang::QualType) SimpleSValBuilder.cpp:0:0
#8 clang::ento::SValBuilder::evalBinOp(llvm::IntrusiveRefCntPtr<clang::ento::ProgramState const>,
clang::BinaryOperatorKind, clang::ento::SVal, clang::ento::SVal, clang::QualType) (clang-21)
#9 clang::ento::ExprEngine::VisitBinaryOperator(clang::BinaryOperator const*, clang::ento::ExplodedNode*,
clang::ento::ExplodedNodeSet&) (clang-21)

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: None (vabridgers)

Changes

This change modifies getElementRegion to retain address space information when removing all QualType qualifiers. Always removing address space qualifiers can expose cases where QualTypes for different pointer types may be different depending on how the QualTypes are obtained.

clang: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:785:
void assertEqualBitWidths(clang::ento::ProgramStateRef, clang::ento::Loc,
clang::ento::Loc): Assertion `RhsBitwidth == LhsBitwidth &&
"RhsLoc and LhsLoc bitwidth must be same!"' failed.

#0 <addr> llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (clang-21)
...
#7 <addr> (anonymous namespace)::SimpleSValBuilder::evalBinOpLL(llvm::IntrusiveRefCntPtr<clang::ento::ProgramState const>,
clang::BinaryOperatorKind, clang::ento::Loc, clang::ento::Loc, clang::QualType) SimpleSValBuilder.cpp:0:0
#8 <addr> clang::ento::SValBuilder::evalBinOp(llvm::IntrusiveRefCntPtr<clang::ento::ProgramState const>,
clang::BinaryOperatorKind, clang::ento::SVal, clang::ento::SVal, clang::QualType) (clang-21)
#9 <addr> clang::ento::ExprEngine::VisitBinaryOperator(clang::BinaryOperator const*, clang::ento::ExplodedNode*,
clang::ento::ExplodedNodeSet&) (clang-21)

---

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

3 Files Affected:

• (modified) clang/include/clang/AST/Type.h (+10)
• (modified) clang/lib/StaticAnalyzer/Core/MemRegion.cpp (+1-1)
• (added) clang/test/Analysis/element-region-addr-space.c (+18)

diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index 98810fbea726e..a56fbbe1fac56 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -1188,6 +1188,7 @@ class QualType {
   void removeLocalConst();
   void removeLocalVolatile();
   void removeLocalRestrict();
+  QualType removeNonAddressSpaceQualifiers();
 
   void removeLocalFastQualifiers() { Value.setInt(0); }
   void removeLocalFastQualifiers(unsigned Mask) {
@@ -8274,6 +8275,15 @@ inline void QualType::removeLocalVolatile() {
   removeLocalFastQualifiers(Qualifiers::Volatile);
 }
 
+inline QualType QualType::removeNonAddressSpaceQualifiers() {
+  if (getQualifiers().hasTargetSpecificAddressSpace()) {
+    removeLocalFastQualifiers();
+  } else {
+    return getCanonicalType().getUnqualifiedType();
+  }
+  return *this;
+}
+
 /// Check if this type has any address space qualifier.
 inline bool QualType::hasAddressSpace() const {
   return getQualifiers().hasAddressSpace();
diff --git a/clang/lib/StaticAnalyzer/Core/MemRegion.cpp b/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
index 3e68373028b10..895f2ab43dc35 100644
--- a/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
+++ b/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -1217,7 +1217,7 @@ const ElementRegion *
 MemRegionManager::getElementRegion(QualType elementType, NonLoc Idx,
                                    const SubRegion *superRegion,
                                    const ASTContext &Ctx) {
-  QualType T = Ctx.getCanonicalType(elementType).getUnqualifiedType();
+  QualType T = elementType.removeNonAddressSpaceQualifiers();
 
   llvm::FoldingSetNodeID ID;
   ElementRegion::ProfileRegion(ID, T, Idx, superRegion);
diff --git a/clang/test/Analysis/element-region-addr-space.c b/clang/test/Analysis/element-region-addr-space.c
new file mode 100644
index 0000000000000..f8a1185fd1ecf
--- /dev/null
+++ b/clang/test/Analysis/element-region-addr-space.c
@@ -0,0 +1,18 @@
+// RUN: %clang_analyze_cc1 -triple amdgcn-unknown-unknown \
+// RUN: -analyzer-checker=core,unix.Malloc,debug.ExprInspection -verify \
+// RUN: -Wno-incompatible-pointer-types -Wno-unused-comparison %s
+
+// expected-no-diagnostics
+//
+// By default, pointers are 64-bits.
+#define ADDRESS_SPACE_32BITS __attribute__((address_space(3)))
+ADDRESS_SPACE_32BITS void *b();
+ADDRESS_SPACE_32BITS int *c();
+typedef struct {
+  ADDRESS_SPACE_32BITS int *e;
+} f;
+void g() {
+  ADDRESS_SPACE_32BITS void *h = b();
+  ADDRESS_SPACE_32BITS f *j = c();
+  j->e == h;
+}

[steakhal]

I'm not sure how broadly this function is useful for anyone else.
If only the static analyzer uses this, then probably we should just have this as a static helper there instead.

Suggested change

	inline QualType QualType::removeNonAddressSpaceQualifiers() {
	if (getQualifiers().hasTargetSpecificAddressSpace()) {
	removeLocalFastQualifiers();
	} else {
	return getCanonicalType().getUnqualifiedType();
	}
	return *this;
	}
	inline QualType QualType::removeNonAddressSpaceQualifiers() {
	if (getQualifiers().hasTargetSpecificAddressSpace()) {
	removeLocalFastQualifiers();
	return *this;
	}
	return getCanonicalType().getUnqualifiedType();
	}

BTW I'm not really familiar with the QualType methods, but I was not expecting mutations if we anyway return a value. What I'd expect when calling this function that this would not mutate (and have side effect) the object itself but just return the right value instead.
This semantic is usually achieved by taking a copy of self, apply the mutation on the copy and return the mutated copy.

Alternatively, you could decide to have a void return type to solve this inconsistency, and clearly admit that calling this has side effects.

[NagyDonat]

I don't think that this method would be used outside of the static analyzer, so as @steakhal also suggested I think this should be a static function in MemRegion.cpp (and not a method of QualType which is used everywhere in clang).

[NagyDonat]

I experimented with this test and it turns out that the crux is that you compare an ADDRESS_SPACE_32BITS int * (the field e) and an ADDRESS_SPACE_32BITS void * (the pointer h). (This forces an implicit cast, which is represented by the ElementRegion that drops the address space information.)

I verified that (without your fix) the assertion is also triggered by

Suggested change

	ADDRESS_SPACE_32BITS void *b();
	ADDRESS_SPACE_32BITS int *c();
	typedef struct {
	ADDRESS_SPACE_32BITS int *e;
	} f;
	void g() {
	ADDRESS_SPACE_32BITS void *h = b();
	ADDRESS_SPACE_32BITS f *j = c();
	j->e == h;
	}
	int test(ADDRESS_SPACE_32BITS int *p, ADDRESS_SPACE_32BITS void *q) {
	return p == q;
	}

so I think it would be better to use this instead of your (also correct, but more complex) example.

[NagyDonat]

I dug into investigating the details of this patch and ended up reimplementing its logic in PR #151370 which is a bit more streamlined but basically "does the same thing" as this one.

[vabridgers]

I'm abandoning this review in favor of @NagyDonat 's solution here - #151370
Thanks @NagyDonat !

I will be abandoning this PR in favor of Donat's changes. Thanks for the comments.