[analyzer] Improve `CallDescription` to handle c++ method.
Needs ReviewPublic

Authored by MTC on Mon, Jun 11, 7:39 AM.

Details

Summary

CallDecription can only handle function for the time being. If we want to match c++ method, we can only use method name to match and can't improve the matching accuracy through the qualifiers.

Diff Detail

Repository
rC Clang
MTC created this revision.Mon, Jun 11, 7:39 AM
MTC added a comment.EditedMon, Jun 11, 7:47 AM

The implementation is not complicated, the difficulty is that there is no good way to get the qualified name without template arguments. For std::basic_string::c_str(), its qualified name may be std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::c_str, it is almost impossible for users to provide such a name. So one possible implementation is to use std, basic_string and c_str to match in the std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::c_str sequentially.

MTC updated this revision to Diff 150758.Mon, Jun 11, 7:56 AM

Remove useless header files for testing.

Having C++ support would be awesome!
Thanks for working on this!

While I do agree matching is not trivial with qualified names, this problem is already solved with AST matchers.

I wonder if using matchers or reusing the HasNameMatcher class would make this easier. That code path is already well tested and optimized.

MTC added a comment.Mon, Jun 11, 7:13 PM

Having C++ support would be awesome!
Thanks for working on this!

While I do agree matching is not trivial with qualified names, this problem is already solved with AST matchers.

I wonder if using matchers or reusing the HasNameMatcher class would make this easier. That code path is already well tested and optimized.

Thank you for pointing out the direction, xazax.hun!

I will looking into the matchers and try to give a better solution.

MTC updated this revision to Diff 151166.Wed, Jun 13, 7:48 AM
  • Use hasName matcher to match the qualified name.
  • Use the full name, like std::basic_string<char>::c_str instead of c_str to match the c_str method in DanglingInternalBufferChecker.cpp.
xazax.hun added inline comments.Wed, Jun 13, 9:30 AM
lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
32

I am not sure if this is the right solution in case of this check. We should track c_str calls regardless of what the template parameter is, so supporting any instantiation of basic_string is desired. This might not be the case, however, for other checks.

If we think it is more likely we do not care about the template arguments, maybe a separate API could be used, where we pass the qualified name of the class separately without the template arguments.
Alternatively, we could use matches name so the users could use regexps.

At this point I also wonder what isCalled API gives us compared to matchers? Maybe it is more convenient to use than calling a match. Also, isCalled API has an IdentifierInfo cached which could be used for relatively efficient checks.

@NoQ what do you think?

65

If we check for fully qualified names, this check might be redundant.

lib/StaticAnalyzer/Core/CallEvent.cpp
273

Doesn't match also traverse the subtree of the AST? We only need to check if that particular node matches the qualified name. I wonder if we could, during the traversal, find another node that is a match unintentionally.

NoQ added inline comments.Wed, Jun 13, 3:18 PM
lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
32

I agree that it's better to match the chain of classes and namespaces (in as much detail as the user cares to provide) and discard template parameters.

For example, i wish that a CallDescription that's defined as {"std", "basic_string", "c_str"} would be able to match both std::string::c_str() and std::__1::string::c_str().

MTC added inline comments.Wed, Jun 13, 7:21 PM
lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
32

Yea, checker writers may can't provide all the template arguments, and it's not convenient to use!

I will try to give a better solution!

65

Right, thanks!

lib/StaticAnalyzer/Core/CallEvent.cpp
273

Yea, this maybe a problem, I will test whether this is going to happen and give a fine-grained match. Thanks!