[cfe-dev] [PATCH] Clang Static Analyzer support for temporary destructors

[Jordan Rose]

Wow, thanks for working on this, Alex. Unfortunately I think there may be a few more problems than simply turning things back on. In particular, from test/Analysis/temporaries.cpp:

     if (compute(i == 5 &&
                 (i == 4 || i == 4 ||
                  compute(i == 5 && (i == 4 || check(NoReturnDtor()))))) ||
         i != 4) {
-      clang_analyzer_eval(true); // no warning, unreachable code
+      clang_analyzer_eval(true); // expected-warning{{TRUE}} (possible when i=6)
     }

i cannot equal 6 at this point in the code; testConsistencyNested has a line earlier that says "if (i != 5) return". So we're not getting the correct behavior here—either the destructor isn't ending up in the right place in the CFG, or something is invalidating the value of 'i' that shouldn't be. I would guess it's the former, since this series of tests were designed to check Pavel's reworking of the CFG.

This part confuses me:

+    while (const ArrayType *ArrayType = Ctx.getAsArrayType(ObjectType)) {
+      ObjectType = ArrayType->getElementType();
+      VisitCXXDestructor(ObjectType, Dest, S, IsBaseDtor, Pred, Dst);
+    }

For a multidimensional array of, say, Foo, this visits "array", "array[0]", "array[0][0]", etc, down to the single element case. In addition, visiting "array[0]" will also trigger a destruction of "array[0][0]", etc., since this loop happens as the very first thing in VisitCXXDestructor.

I would just leave the FIXME as is here, and not worry about this.

   // FIXME: We need to run the same destructor on every element of the array.
   // This workaround will just run the first destructor (which will still
   // invalidate the entire array).

And then we have your new test case:

+  //TODO: figure out why this case causes an unexpected "Undefined or garbage value returned to caller" warning
+  bool testNamedCustomDestructor() {
+    if (CheckCustomDestructor c = CheckCustomDestructor())
+      return true;
+    return false;
+  }

First of all, nicely discovered. I'm not immediately sure what's wrong here, but let's take a look at the CFG:

 [B3]
   1: CheckCustomDestructor() (CXXConstructExpr, struct CheckCustomDestructor)
   2: [B3.1] (BindTemporary)
   3: [B3.2] (ImplicitCastExpr, NoOp, const struct CheckCustomDestructor)
   4: [B3.3]
   5: [B3.4] (CXXConstructExpr, struct CheckCustomDestructor)
   6: ~CheckCustomDestructor() (Temporary object destructor)
   7: CheckCustomDestructor c = CheckCustomDestructor();

B3.1 is the actual creation of the temporary. B3.5 is the copy constructor required by the C++ standard to copy the temporary into 'c'. B3.6 is the destruction of the temporary, and B3.7 is the actual VarDecl for 'c'. (The block then goes on to call 'operator bool' on 'c' and then do the if-branch.)

The current handling of constructors for VarDecls is a bit hacky. If you look in ExprEngine::VisitCXXConstructExpr, you'll notice it tries to look ahead to the next CFG element to see if it's constructing a particular variable. If so, it sets the target region to that variable. The trouble is, there's now a destructor between B3.5 and B3.7, so my guess is that the analyzer has decided it's not constructing a variable. I am okay with cheating right now by skipping over destructor CFG elements in VisitCXXConstructExpr, but I haven't thought about if there's a better way to tell that a particular CXXConstructExpr goes with a particular VarDecl.

As far as this part goes...

-  case CFGElement::TemporaryDtor:
+  case CFGElement::TemporaryDtor: {
+    const CFGTemporaryDtor &Dtor = Source.castAs<CFGTemporaryDtor>();
+    return PathDiagnosticLocation(Dtor.getBindTemporaryExpr(), SM, CallerCtx);
+  }

...that seems like a good obvious change, and I'm happy to commit that (or for you to commit that) without turning anything else on. :-)

I know this is a lot to throw at you at once, but please continue to e-mail me with questions and progress. I'm very happy that someone is able to put time into this.

Jordan


On Mar 2, 2014, at 0:41 , Alex McCarthy <alexmc at google.com> wrote:

> Hi all,
> 
> I'm running clang's static analyzer on a C++ codebase at Google. I saw a roughly a 50% false positive rate which stemmed from the analyzer not recognizing temporary object destructors: this issue is discussed in some length in another thread, which mentions a similar error rate on the Chromium codebase: http://comments.gmane.org/gmane.comp.compilers.clang.devel/33901
> 
> Starting from Pavel's work which was reverted in http://llvm-reviews.chandlerc.com/rL186925 , I've put together a new patch (see attachment) that attempts to fix temporary object destructor handling.
> 
> This new patch fixes all of the new regression tests added after Pavel's change was reverted, notably including http://llvm-reviews.chandlerc.com/rL187133 . I've also fixed some other crashes, including a crash when processing an array of temporary objects use in a C++11 initializer_list, covered by a new regression test in cfe/test/Analysis/dtor-cxx11.cpp . And most importantly, running clang with this patch eliminates the 50% false positive rate I saw previously (from ~800 warnings to ~400 across the ~1700 file codebase).
> 
> 
> Now for the bad news:
> 
> This patch introduces a new regression which wasn't covered by existing tests: named temporaries declared and used within if statements are considered dead while they're still being used, which results in "Undefined or garbage value returned to caller" errors. I've added regression tests to test/Analysis/dtor.cpp to cover this case, which currently fail. I've also updated test/Analysis/temp-obj-dtors-cfg-output.cpp with relevant CFG dumps to try to debug the problem. This new false positive is much nosier than the ones this patch fixes: the only advantage to the current patch as-is is that the garbage return value warnings are emitted in a small collection of header files, making them much easier to ignore en masse.
> 
> I don't have any compiler experience, so I'm moving slowly in the clang codebase and could use some help understanding where to look next. I've mostly been handling each crash or error as I find it, but I don't have a high level understanding of the impact or context of my change. In particular, I don't know how to read the CFG dumps I've generated, so I'm not sure where things are going wrong. Ted, Jordan, and Anna: Manuel Klimek mentioned that you've looked into this issue at length. Do have any advice on what I'm doing wrong, or could you suggest other approaches I might be able to try? Anything you can think of that can speed up my search for a fix would be greatly appreciated.
> 
> If we can get this patch working, it should address the following issues:
> http://llvm.org/bugs/show_bug.cgi?id=15599 
> http://llvm.org/bugs/show_bug.cgi?id=16664
> http://llvm.org/bugs/show_bug.cgi?id=18159 (not sure, this bug is referenced by a newly fixed test in test/Analysis/temporaries.cpp)
> 
> Thanks for your help,
> -Alex McCarthy
> <temporary-destructors.patch>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140304/b2b78e7d/attachment.html>