Skip to content

[clang][analyzer] Fix InvalidatedIterator crash caused by overload operator member function with explicit this #132581

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

Merged
merged 3 commits into from
Mar 24, 2025
Merged

[clang][analyzer] Fix InvalidatedIterator crash caused by overload operator member function with explicit this #132581

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
67ebfb0
[clang][analyzer] Fix crash caused by overload operator member functi…
flovent Mar 23, 2025
a418a8a
move releated test to InvalidatedIteratorChecker's original testfile
flovent Mar 24, 2025
347c65d
add missing newline
flovent Mar 24, 2025
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
7 changes: 5 additions & 2 deletions clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,11 @@ void ContainerModeling::checkPostCall(const CallEvent &Call,
if (Func->isOverloadedOperator()) {
const auto Op = Func->getOverloadedOperator();
if (Op == OO_Equal) {
// Overloaded 'operator=' must be a non-static member function.
const auto *InstCall = cast<CXXInstanceCall>(&Call);
// Only handle the assignment operator with implicit this
Copy link
Collaborator

@Xazax-hun Xazax-hun Mar 24, 2025

Choose a reason for hiding this comment

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

Maybe a FIXME with a GitHub ticket to properly support explicit could be nice.

Copy link
Contributor

@steakhal steakhal Mar 24, 2025

Choose a reason for hiding this comment

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

Actually, it shouldn't be too hard to fix this. Only like 5 lines of code I imagine.
At that point, it may be easier to fix it instead of creating a ticket.

Copy link
Contributor Author

@flovent flovent Mar 24, 2025

Choose a reason for hiding this comment

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

I think I may not understand correctly, what should i fix here? explicit this will not appear in STL containers' member function.

Copy link
Contributor

@steakhal steakhal Mar 24, 2025

Choose a reason for hiding this comment

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

I think it's technically possible to have a conforming STL implementation using "deducing this" in their implementation.
I agree that it's bikshedding. I'd just say let's move on. We don't even need a GH ticket. I think there are far more pressing issues to track other than this tiny marginal edge case.

const auto *InstCall = dyn_cast<CXXInstanceCall>(&Call);
if (!InstCall)
return;

if (cast<CXXMethodDecl>(Func)->isMoveAssignmentOperator()) {
handleAssignment(C, InstCall->getCXXThisVal(), Call.getOriginExpr(),
Call.getArgSVal(0));
Expand Down
21 changes: 21 additions & 0 deletions clang/test/Analysis/issue-116372.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// RUN: %clang_analyze_cc1 -std=c++23 %s -verify -analyzer-checker=alpha.cplusplus.InvalidatedIterator -analyzer-config aggressive-binary-operation-simplification=true

// expected-no-diagnostics

class ExplicitThis {
int f = 0;
public:
ExplicitThis();
ExplicitThis(ExplicitThis& other);

ExplicitThis& operator=(this ExplicitThis& self, ExplicitThis const& other) { // no crash
self.f = other.f;
return self;
}

~ExplicitThis();
};

void func(ExplicitThis& obj1) {
obj1 = obj1;
}