This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Allow matching non-CallExprs using CallDescriptions
ClosedPublic

Authored by steakhal on Oct 11 2021, 3:40 AM.

Details

Summary

Fallback to stringification and string comparison if we cannot compare
the IdentifierInfos, which is the case for C++ overloaded operators,
constructors, destructors, etc.

Examples:

{ "std", "basic_string", "basic_string", 2} // match the 2 param std::string constructor
{ "std", "basic_string", "~basic_string" }  // match the std::string destructor
{ "aaa", "bbb", "operator int" } // matches the struct bbb conversion operator to int

Diff Detail

Unit TestsFailed

Event Timeline

steakhal created this revision.Oct 11 2021, 3:40 AM
steakhal requested review of this revision.Oct 11 2021, 3:40 AM
whisperity added inline comments.Oct 11 2021, 5:26 AM
clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
67

Oh yeah, if only. But also, sadly, D98477 is stalled... ☹

69

Is there a chance people accidentally passing const X (perhaps involving decltype somewhere) to T cause a problem in this logic? is_same does not remove qualifiers, right?

77

"Unsupported expression kind in CallDescriptor" for message? When an UNREACHABLE is hit, the user is only given the message (just like in the case of assert()) and not a context useful enough to deduce what only these is supposed to mean...

89

(Ditto, as before.)

Thanks for the review @whisperity.
If you don't have strong objections I wouldn't change the code.

clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
69

It simply explicitly express the limitations of the unit-test itself.
It's not supposed to be user-facing to the developers.
That being said, I don't want to spew metaprogramming without much reason.

The best would be to somehow reuse the actual dispatching logic embedded into the analyzer expr engine.
However, that is tightly coupled with the exploded graph-building logic, so it won't happen any time soon.

77

By the end of the day, it doesn't really matter what the message is, it's for developers only.
They would have to check the code to make a conscious decision about *what* exactly is missing :D

I don't mind improving the message, but it won't help by any means.

balazske added inline comments.
clang/lib/StaticAnalyzer/Core/CallEvent.cpp
332

Nice way of refactoring the tests!

However, could have a test that checks if the logic "sees through" type aliases? E.g. can we match the ctor of std::string even if we have using X = std::string; X x("banana");

clang/lib/StaticAnalyzer/Core/CallEvent.cpp
332
333

Is it possible to have an empty QualifiedName at this point? Should we assert that it is not empty?

Nice way of refactoring the tests!

However, could have a test that checks if the logic "sees through" type aliases? E.g. can we match the ctor of std::string even if we have using X = std::string; X x("banana");

I agree but did not want to pollute the blame of this commit. I want to have tests covering *only* the changed parts.
I'm going to create a follow-up patch covering this case.

clang/lib/StaticAnalyzer/Core/CallEvent.cpp
333

Technically it's a class invariant. We must have at least one element in that vector, representing the name of the function/operator/etc. of the entity that we want to match against. In that sense, we should add an assertion in the constructor of the class enforcing this property.

I wanted to focus only on the isCalled() function in this patch, but I can introduce that assertion into the CallDescription class without much problem I suppose. I'm gonna address this.

steakhal updated this revision to Diff 379671.Oct 14 2021, 5:10 AM
steakhal edited the summary of this revision. (Show Details)
steakhal added reviewers: balazske, ASDenysPetrov.

I reorganized the changes across the patches. The first NFC patch introduced the unittests to cover a lot more cases and gain confidence that the actual refactor in the second NFC patch does not break anything.
Finally, this patch simply switches the behavior in that sense, to have the expected behavior.

martong accepted this revision.Oct 14 2021, 7:09 AM

LGTM!

This revision is now accepted and ready to land.Oct 14 2021, 7:09 AM
steakhal updated this revision to Diff 379779.Oct 14 2021, 11:05 AM
steakhal marked 7 inline comments as done.
Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2021, 5:58 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript