Page MenuHomePhabricator

[analyzer][NFC] Introduce CXXDeallocatorCall, deploy it in MallocChecker
ClosedPublic

Authored by Szelethus on Mar 1 2020, 4:27 PM.

Details

Summary

This is my first ever patch touching ExprEngine, sorry if the code causes lasting damage to the reader.

One of the pain points in simplifying MallocCheckers interface by gradually changing to CallEvent is that a variety of C++ allocation and deallocation functionalities are modeled through preStmt<...> where CallEvent is unavailable, and a single one of these callbacks can prevent a mass parameter change.

This patch introduces a new CallEvent, CXXDeallocatorCall, which happens after preStmt<CXXDeleteExpr>, and can completely replace that callback as demonstrated. Note how you can retrieve this call through preCall, yet there is no postCall to pair it with -- the reason behind this is that neither does preStmt<CXXDeleteExpr> have a postStmt<CXXDeleteExpr> pair. But I have no clue why that is :^)

Diff Detail

Event Timeline

Szelethus created this revision.Mar 1 2020, 4:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2020, 4:27 PM

Cool! May it worth to mention the corresponding mail from the mailing list in the Summary: http://lists.llvm.org/pipermail/cfe-dev/2020-February/064754.html

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

St -> State

958

return getOriginExpr()->getNumArgs()

Charusso accepted this revision.Mar 3 2020, 3:54 AM
This revision is now accepted and ready to land.Mar 3 2020, 3:54 AM

This might be a silly question but is CXXDeleteExpr the only way to invoke a deallocator? What would happen if the user would call it by hand? Should we expect ExprEngine to trigger a callback in that case?

NoQ added a comment.Mar 3 2020, 7:53 PM

I love where this is going.

the reason behind this is that neither does preStmt<CXXDeleteExpr> have a postStmt<CXXDeleteExpr> pair. But I have no clue why that is :^)

I'm pretty sure it's forgotten :/

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

After this you probably received compiler warnings saying "case isn't covered in switch". You'll need to clean them up.

Another thing to do would be to update CallEventManager's getCall() and getCaller() methods that'd allow the users to construct the new CallEvent easily without thinking about what specific kind of call event it is.

958

This wouldn't compile. CXXDeleteExpr is not a CallExpr.

It sounds like there are currently five different operator deletes:

And, unlike operator new, there's no option to provide custom "placement" arguments.

So i think the logic in this patch is correct but we should do some custom logic for all 5 cases in the getArgExpr method, where argument expressions for the extra arguments will have to be conjured out of thin air (or we might as well return null).

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
853

Yes, we should absolutely have PostCall here as well, even if nothing happens in between.

And once it's there, the next major step would be to invoke ExprEngine::evalCall() instead if the operator was overloaded, so that we could inline the deallocator (or evaluate it conservatively, which might also be of value as it would invalidate the necessary globals).

steakhal added inline comments.Mar 4 2020, 3:38 AM
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
958

It sounds like there are currently five different operator deletes:

I think it is even worse since C++17 and C++20 introduced a couple of others like:

  • overloads with std::align_val_t parameter (C++17)
  • overloads with std::destroying_delete_t parameter (C++20) which I haven't heard of yet :D

You can check it in the draft: http://eel.is/c++draft/new.delete
And of course at cppreference as well: https://en.cppreference.com/w/cpp/memory/new/operator_delete

Szelethus updated this revision to Diff 254780.Apr 3 2020, 6:43 AM
Szelethus marked 2 inline comments as done.
  • Add PostStmt<CXXDeleteExpr>
  • Add PostCall for CXXDeallocatorCall
  • Add a unittest, which is kind of pointless as-is, but I realized only later that most of the delete operators aren't recognized as CXXDeleteExpr
  • Add a bunch of TODOs about incorrect CallEvent types.
Szelethus requested review of this revision.Apr 3 2020, 6:43 AM

Since this change had a few blocking issues, I'm placing it back to review for greater visibility.

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

No, I did not, infuriatingly. I did however get errors after trying to make a toString function for CallEventKind, apparently both CE_CXXDeallocator and CE_Block has the value of 7. When I moved the enum, everything was fine, and I did get the warnings you mentioned.

958

Okay so here is the biggest issue: non-simple deletes don't appear in the AST as CXXDeleteExpr, but rather as a CXXOperatorCall. This is a big problem, what could be the reason for it?

Szelethus marked an inline comment as done.Apr 6 2020, 2:27 AM
Szelethus added inline comments.
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
59

I need to move this below CE_Function, as its not in the range of CE_BEG_FUNCTION_CALLS and CE_END_FUNCTION_CALLS.

Just a gentle ping :)

Overall looks good to me some questions and nits inline.

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

This is marked as done. If the conclusion is that you do not need to move it I think it would be worth to add a comment why :)

936

How important and hard it is to fix this? If you did not find the reason why those CXXDeleteExprs are missing you can do two things:

  1. Look at how such AST is consumed by CodeGen
  2. Ask around on the mailing list with a minimal example.
967

Is the index unused on purpose? If yes, wouldn't this trigger a warning on build bots? Adding a maybe unused annotation and/or a comment would be welcome.

clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp
95 ↗(On Diff #254780)

I think this change might be a better fit for a separate commit. I think you don't even need to have such a small change reviewed. You could just commit it as is. Just do not forget to describe that these cases are really hard to have a test for but this is the idiomatic way of doing this in checkers.

Szelethus marked 5 inline comments as done.May 8 2020, 4:22 AM
Szelethus added inline comments.
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
59

Marked done automatically, I'll address this before commiting :)

936

Fair point, but it surely is a thing to keep in mind because it could, or more probably will cause surprises. A TODO or a NOTE would be more appropriate. In any case, I'd prefer to address these in a followup patch.

967

Unused argument warnings are disabled by the build system, but a comment wouldn't hurt. CXXDeleteExprs can only have a single argument, but as earlier inlines suggests, we will need to mind custom deletes later on.

NoQ accepted this revision.May 9 2020, 7:09 PM

Sorry i'm not very responsive these days >.<

clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp
95 ↗(On Diff #254780)

I'm also curious why did this suddenly become necessary. This is obviously the right thing to do but why the change? It might indicate that we accidentally started incorrectly agglutinating nodes that were previously considered different. Like, we might have messed up our program points somewhere in this patch.

clang/test/Analysis/cxx-dynamic-memory-analysis-order.cpp
31–32 ↗(On Diff #254780)

That's fairly unobvious to me. Isn't the whole point of CXXAllocatorCall/CXXDeallocatorCall to give access to specific aspects of new/delete-expressions that aren't available in manual invocations with function syntax? On the other hand, i agree that it's nice to be able to cast the call event to CXXAllocatorCall/CXXDeallocatorCall and be done with allocators/deallocators.

This revision is now accepted and ready to land.May 9 2020, 7:09 PM
This revision was automatically updated to reflect the committed changes.
Szelethus marked 14 inline comments as done.
In D75430#2028456, @NoQ wrote:

Sorry i'm not very responsive these days >.<

No worries, thanks! ^-^

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

Actually, see my other inline. I do believe that this should be the one class to solve all deletes, or at the very least, standard-specified deletes.

clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp
95 ↗(On Diff #254780)

The creduced program that causes this checker to crash (without the early return):

struct a {};
struct b : a {};
void c() {
  a *d = new b;
  delete d;
}

Turns out, as I said in an another inline, I accidentally run PreStmt on delete expressions twice. In any case, I decided to add this early return and a test case in this patch, because I think strongly related, and as seen here, tests the added functionality.

clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
1652 ↗(On Diff #254780)

Oh wow. I accidentally ran checkers on PreStmt here as well. Thats why I needed the new early return to not crash.

clang/test/Analysis/cxx-dynamic-memory-analysis-order.cpp
31–32 ↗(On Diff #254780)

Yup, in my mind, this isn't CXXNewCall, but rather CXXAllocatorCall (same for delete), I would expect this to handle it all (maybe change the interfaces so that getOriginExpr returns with Expr, and add a getOriginAsCXXNewExpr method).

Szelethus marked an inline comment as done.May 19 2020, 3:46 AM