[analyzer] ReturnVisitor: Bypass everything to see inlined calls

[Charusso]

Summary

When we traversed backwards on ExplodedNodes to see where we processed the
given statement we break too early. With the current approach we do not
miss the CallExitEnd ProgramPoint which stands for an inlined call.

[Szelethus]

clang/test/Analysis/new-ctor-null-throw.cpp

1–3

Hmm, how come you removed -analyzer-config c++-allocator-inlining=true?

[NoQ]

Aha, that's something! And nice to see we've already had this bug covered with tests. Because, of course, i added these tests a year ago without even thinking about what the correct behavior should look like :/

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

835–842

This iteration may now take us straight to the root of the graph. I don't think it's supposed to be that slow; i think it's supposed to only skip within maybe a full-expression at most.

Which statements in the AST are getting peeled off here that weren't before? Which statements are supposed to get peeled off?

Might it be that we should simply add one more case to peelOffOuterExpr() or something like that?

clang/test/Analysis/new-ctor-null-throw.cpp

1–3

It's on by default these days.

29

Let's add a // no-warning here, probably with a comment that this the warning is intentionally suppressed.

clang/test/Analysis/new-ctor-null.cpp

29

Let's add a // no-warning here as well.

[Charusso]

• More explicit approach.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

835–842

We have the following sequence in the loop:

"stmt_kind": "DeclStmt", "stmt_point_kind": "PostStmt"
"stmt_kind": "DeclStmt", "stmt_point_kind": "PostStore"
"stmt_kind": "DeclStmt",  "stmt_point_kind": "PreStmtPurgeDeadSymbols"
"pretty": "S *s = new S [10];"

"stmt_kind": "CXXNewExpr", "stmt_point_kind": "PostStmt"
"stmt_kind": "CXXNewExpr", "stmt_point_kind": "PreStmtPurgeDeadSymbols"
"pretty": "new S [10]"

"stmt_kind": "CXXConstructExpr", "stmt_point_kind": "PostStmt"
"stmt_kind": "CXXConstructExpr", "stmt_point_kind": "PreStmt"
"stmt_kind": "CXXConstructExpr", "stmt_point_kind": "PreStmtPurgeDeadSymbols"
"pretty": null

"kind": "CallExitEnd" - found the ReturnStmt, all good.

This ReturnVisitor is totally ProgramPoint-based and I wanted to be super generic. Do you know about more problematic constructing-object?

[Charusso]

• Better wording.

[Charusso]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

835–842

Btw: this cool dump based on D62946

[Charusso]

If I run scan-build on LLVM this is the only non-bypassed case since the first diff:

non-bypassed.html569 KBDownload

I think it is working well.

[NoQ]

Aha, nice, that's much cleaner! Indeed, we have to skip the construct-expression, which is going to be on our way to the program point of new-expression which corresponds to calling the allocator, despite being nested within the new-expression. An annoying corner-case.

I'd like to have a test in which the constructor is inlined. I suspect that we'll have to skip the whole inlined call in this case, and it'll be a lot of various program points. We'll most likely have to consult location contexts to skip everything within the constructor's stack frame.

In the test cases that you've updated the constructor isn't inlined because it's an array-new operator and we don't bother inlining 10 default constructors (@Szelethus: a nice application of data flow analysis might be to find out if the default constructor always initializes the structure in the same way, say all nulls, so we could evaluate one constructor and default-bind it to the whole array in this case; @baloghadamsoftware: this in turn reminds me that i still need to do something about D22374).

[Charusso]

• Added CallExpr as being purged away
• More test

[Charusso]

In D62926#1533560, @NoQ wrote:

I'd like to have a test in which the constructor is inlined.

What do you mean by that test?

[NoQ]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

837–840

Comparing statements is usually insufficient because the same statement may appear multiple times due to recursion. When recursion occurs, you may reach the same statement in a different location context. You should think in terms of (statement, location context) pairs to avoid these problems. Your aim here is to find the CallExitEnd node that corresponds to returning from an inlined operator new to the current location context. You should stop searching when you find an unrelated statement in the current location context or when you exit the current location context entirely.

[Charusso]

• The most generic approach.

[Charusso]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

837–840

I have made a little test when we have a 25-second long Static Analyzer run with predefined names and checker. The loop ran 500 times in summary and we have some serious performance impacts at other places.

We exit the current context to see inlined calls, so that could not work sadly. If you remove that nonsense second condition we run at the same time, so if we have not got any problem since 7 years ago I think it is good to go.

[NoQ]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

837–840

You should break the loop as soon as we go past the new-expression that we've started with in the stack frame that we've started with. That is, you should at most go through the constructor within the new-expression, and then break. You shouldn't explore the whole graph to the root every time the operator-new call isn't inlined.

This might still be slow in some rare cases, but it should be much faster.

[Charusso]

• I have used LocationContext::isParentOf()which is not worked well, so I thought we are out of our context.
• With that I made some misleading assumptions about our code.

[Charusso]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

837–840

Sorry, I was dumb and I have used isParentOf() before, I think it is the most simple approach now truly.

[NoQ]

All right, it seems that i'm completely misunderstanding this problem and we've been talking past each other this whole time.

The problem is not that we need to skip the CXXConstructExpr. The problem is that we need to skip CXXNewExpr (!!). CFG elements for an operator-new call go like this:

1. CFGAllocatorCall
2. CXXConstructExpr
3. CXXNewExpr

We're staring at element 3. The original loop says "hey, that's our statement! we've found it!". However, even though this is the statement that's taken as the call site, the respective ExplodedNode does not correspond to the end of the call. Instead, it corresponds to a moment of time after construction has finished and the new-expression is fully evaluated. So we need to ignore element 3. Then, element 2 is ignored automatically because the statements don't match. Then, we need to learn to recognize that element 1 is the right element, which is going to be either a CallExitEnd (if the allocator call was inlined) or dunno-but-definitely-not-PostStmt if the allocator was evaluated conservatively.

Bonus: the constructor is never inlined in our scenario, so it's easy to skip element 2, as it's going to be just PreStmt/PostStmt and no inlined call within it. Why? Because how the hell do you inline a constructor of a null pointer. The Standard guarantees that when the allocator returns a null pointer, the constructor is gracefully omitted. We currently have a FIXME in prepareForObjectConstruction() to implement this behavior:

179         // TODO: Detect when the allocator returns a null pointer.
180         // Constructor shall not be called in this case.

The current incorrect behavior is to instead evaluate the constructor conservatively, with a fake temporary region instead of the null pointer (this is generally the conservative approach to evaluating the constructor when object-under-construction is not known).

So my request to come up with a test case in which the constructor is going to be inlined was completely pointless. But in the general case we aren't really sure that it's going to be a null pointer (we may want to track an arbitrary value), so we shouldn't rely on that.

clang/test/Analysis/new-ctor-null.cpp

36

This is essentially the same FIXME: the constructor should not have been evaluated, so in particular we should not invalidate globals.

[Charusso]

In D62926#1539191, @NoQ wrote:

All right, it seems that i'm completely misunderstanding this problem and we've been talking past each other this whole time.

The problem is not that we need to skip the CXXConstructExpr. The problem is that we need to skip CXXNewExpr (!!). CFG elements for an operator-new call go like this:

I really wanted to create a generic approach without any overhead. I have added the proper test case what is going on: we enter into another contexts and that should be modeled properly. I was not sure about how to do so, but after a while it has been born. It contains all of your ideas from all the comments and now we only bypassing CXXNewExprs.

[NoQ]

Yup, this makes sense now! I'll do some nit-picking for a little longer.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

839

For now this hack is clearly only for CXXNewExpr. And it happens because our AST is weird and there's no separate sub-expression dedicated entirely to invoking the allocator. Therefore let's give this variable a better name, maybe BypassCXXNewExprEvaluation or something like that.

845–849

Branches here are unnecessary because there's no performance win here. Let's just

CurrentSFC = Node->getStackFrame();

851–855

This code would currently bypass the call site for the conservatively evaluated allocator. It doesn't seem to be immediately important but let's add a FIXME.

[Charusso]

• Fixed comments.

[NoQ]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

845

Mmm, wait a sec. This way the loop condition is trivially true. The second check on line 862 is also trivially true. I think you want to make it a separate variable.

[Charusso]

• Whoops.

[Charusso]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

845

I was never cool in algorithms, thanks!

[NoQ]

Great, thanks!! Let's commit this.

[hintonda]

This test fails to compile on Windows 64 bit builds. Please see http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/77

cfe/trunk/test/Analysis/inlining/placement-new-fp-suppression.cpp

16 ↗

(On Diff #204910)

I don't believe this is really ever portable, but definitely not on 64 bit Windows where a long is 32 bits.

Can't you just #include <stdint.h> instead?

[Szelethus]

cfe/trunk/test/Analysis/inlining/placement-new-fp-suppression.cpp

16 ↗

(On Diff #204910)

In the test, I'm pretty sure that he can't.

I grepped test/Analysis, and found that malloc.cpp does the following:

typedef unsigned __INTPTR_TYPE__ uintptr_t

Maybe that's worth a shot?

[Szelethus]

cfe/trunk/test/Analysis/inlining/placement-new-fp-suppression.cpp

16 ↗

(On Diff #204910)

I seem to have been proven wrong. :)

[Charusso]

In D62926#1545163, @hintonda wrote:

This test fails to compile on Windows 64 bit builds. Please see http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/77

Thanks you! I have not got a mail, now it passes on my side: http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/80/steps/stage%201%20check/logs/stdio
But some unrelated error occurs: http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/80/steps/stage%202%20build/logs/stdio

Sorry for all the inconveniences it caused.

[Charusso]

@NoQ, thanks for the review! Now everything is working by rL363515.

cfe/trunk/test/Analysis/inlining/placement-new-fp-suppression.cpp

16 ↗

(On Diff #204910)

I was a little-bit surprised as well. Thanks for the idea!

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

public:
   /// the statement is a call that was inlined, we add the visitor to the
   /// bug report, so it can print a note later.
   static void addVisitorIfNecessary(const ExplodedNode *Node, const Stmt *S,
                                     BugReport &BR,
                                     bool InEnableNullFPSuppression) {
     if (!CallEvent::isCallStmt(S))
       return;
 
     // First, find when we processed the statement.
     do {
       if (auto CEE = Node->getLocationAs<CallExitEnd>())
         if (CEE->getCalleeContext()->getCallSite() == S)
           break;
-      if (auto SP = Node->getLocationAs<StmtPoint>())
-        if (SP->getStmt() == S)
-          break;
 
       Node = Node->getFirstPred();
     } while (Node);
 
     // Next, step over any post-statement checks.
     while (Node && Node->getLocation().getAs<PostStmt>())
       Node = Node->getFirstPred();
     if (!Node)
       return;
 
     // Finally, see if we inlined the call.
     Optional<CallExitEnd> CEE = Node->getLocationAs<CallExitEnd>();
     if (!CEE)
       return;
 
     const StackFrameContext *CalleeContext = CEE->getCalleeContext();
     if (CalleeContext->getCallSite() != S)
       return;
 
     // Check the return value.
     ProgramStateRef State = Node->getState();
     SVal RetVal = Node->getSVal(S);
 
     // Handle cases where a reference is returned and then immediately used.

clang/test/Analysis/new-ctor-null-throw.cpp

-// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,debug.ExprInspection \
+// RUN:  -std=c++11 -verify %s
 
 void clang_analyzer_eval(bool);
 
 typedef __typeof__(sizeof(int)) size_t;
 
 
 // These are ill-formed. One cannot return nullptr from a throwing version of an
 // operator new.
 void *operator new(size_t size) {
   return nullptr;
+  // expected-warning@-1 {{'operator new' should not return a null pointer unless it is declared 'throw()' or 'noexcept'}}
 }
 void *operator new[](size_t size) {
   return nullptr;
+  // expected-warning@-1 {{'operator new[]' should not return a null pointer unless it is declared 'throw()' or 'noexcept'}}
 }
 
 struct S {
   int x;
   S() : x(1) {}
   ~S() {}
 };
 
 void testArrays() {
   S *s = new S[10]; // no-crash
-  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+  s[0].x = 2;
 }

clang/test/Analysis/new-ctor-null.cpp

-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,debug.ExprInspection \
+// RUN:  -std=c++11 -verify %s
 
 void clang_analyzer_eval(bool);
 void clang_analyzer_warnIfReached();
 
 typedef __typeof__(sizeof(int)) size_t;
 
 void *operator new(size_t size) throw() {
   return nullptr;
S() : x(1) {
     // returned by operator new().
     clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
   }
   ~S() {}
 };
 
 void testArrays() {
   S *s = new S[10]; // no-crash
-  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+  s[0].x = 2;
 }
 
 int global;
 void testInvalidationOnConstructionIntoNull() {
   global = 0;
   S *s = new S();
   // FIXME: Should be FALSE - we should not invalidate globals.
   clang_analyzer_eval(global); // expected-warning{{UNKNOWN}}
 }