Skip to content

[analyzer] Fix getElementRegion to retain address space information #151249

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 1 commit into from
Closed

[analyzer] Fix getElementRegion to retain address space information #151249

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 10 additions & 0 deletions clang/include/clang/AST/Type.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1188,6 +1188,7 @@ class QualType {
void removeLocalConst();
void removeLocalVolatile();
void removeLocalRestrict();
QualType removeNonAddressSpaceQualifiers();

void removeLocalFastQualifiers() { Value.setInt(0); }
void removeLocalFastQualifiers(unsigned Mask) {
Expand Down Expand Up @@ -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;
}
Comment on lines +8278 to +8285
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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).


/// Check if this type has any address space qualifier.
inline bool QualType::hasAddressSpace() const {
return getQualifiers().hasAddressSpace();
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/StaticAnalyzer/Core/MemRegion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
18 changes: 18 additions & 0 deletions clang/test/Analysis/element-region-addr-space.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
}
Comment on lines +9 to +18
Copy link
Contributor

Choose a reason for hiding this comment

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

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.