Page MenuHomePhabricator

[analyzer][MallocChecker][NFC] Change the use of IdentifierInfo* to CallDescription
ClosedPublic

Authored by Szelethus on Sep 27 2019, 3:10 PM.

Details

Summary

Exactly what it says on the tin! I decided not to merge this with the patch that changes all these to a CallDescriptionMap object, so the patch is that much more trivial.

Diff Detail

Event Timeline

Szelethus created this revision.Sep 27 2019, 3:10 PM
NoQ accepted this revision.Sep 27 2019, 7:01 PM

Fantastic, thanks!!

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
259

I don't fully understand how does overload resolution work in this case, maybe rename the original function?

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
997

The FD->getKind() != Decl::Function part is super mega redundant here.

This revision is now accepted and ready to land.Sep 27 2019, 7:01 PM
kimgr added a subscriber: kimgr.Sep 28 2019, 1:20 AM
kimgr added inline comments.
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
997

Sorry for jumping in from nowhere. AFAIK, this is the only way to detect free vs member functions. It looks like this wants to discard member functions. Are you sure it's redundant?

Szelethus marked 2 inline comments as done.Sep 28 2019, 2:20 AM
Szelethus added inline comments.
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
259

Well, isCalled may now have an arbitrary number of arguments. The single argument version is this function, and the n-argument ones call this n times: isCalled(A, B, C) -> isCalled(A) || isCalled(B) || isCalled(B). I guess I could rename the other one to isAnyCalled, but I think its fine.

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
997

Please do! Though I have a suspicion that if this isn't redundant, such a check should be done in CallDescription.

Szelethus updated this revision to Diff 222791.Oct 2 2019, 3:14 AM

Rebase, unforget to support 3-arg malloc, for which apparently were no tests at all.

Szelethus marked 5 inline comments as done.Feb 25 2020, 6:28 AM
Szelethus added inline comments.
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
259

But that would be super ugly imo :/ I think the current implementation is intuitive enough, but I'm happy to change it if you disagree.

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
997

I agree that it should be removed, since we explicitly support functions annotated to return a dynamically allocated memory region, even if it is a member function.

This revision was automatically updated to reflect the committed changes.
NoQ added inline comments.Feb 25 2020, 7:26 AM
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
259

Ok, so, maybe in such cases we should add a flag to our CallDescriptionMap and then write code like

if (CDM.lookup(Call)->hasFlag) {
  ...
}

?

(or, maybe, make a separate map if these calls are never present in the rest of the code... do we want CallDescriptionSet for this purpose?)

Szelethus marked an inline comment as done.Feb 25 2020, 10:13 AM
Szelethus added inline comments.
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
259

Well, the followup patch where the map is deployed uses nothing fancy. Lets wait until we actually need such a thing.

What do you mean under "calls that are never present in the code"?