[cfe-dev] [PATCH] Clang Static Analyzer support for temporary destructors
Alexander Kornienko
alexfh at google.com
Sat Mar 22 17:33:32 PDT 2014
I'm waiting impatiently for this patch to land ;)
Any progress here? Jordan, AFAIU the ball is on your side?
On Thu, Mar 6, 2014 at 8:47 PM, Alex McCarthy <alexmc at google.com> wrote:
> Thanks for taking a look, Jordan, I really appreciate the extra eyes.
> Please find an updated patch attached.
>
> I misread the test in temporaries.cpp, thanks. I've restored the test so
> it's testing the right thing: it still fails and I'm currently stumped.
> I've produced some simpler test cases that are much easier to analyze, but
> I still don't understand how this incorrect behavior is triggered. I've
> also added more CFG dumps to temp-obj-dtors-cfg-output.cpp.test to see if
> anything stands out. I've also added a lot of debug output which clearly
> has to be removed before submission. I'd really appreciate any more advice
> you have here, and I'll keep (slowly) debugging in the mean time.
>
> The array processing while loop I added to ExprEngineCxx.cpp's
> ::VisitCXXDestructor was an attempt to fix the crash that I found when
> processing initializer_lists of temporary objects. I added a new test to
> dtor-xx11.cpp to cover this case:
>
> namespace Cxx11BraceInit {
> struct Foo {
> ~Foo() {}
> };
>
> void testInitializerList() {
> for (Foo foo : {Foo(), Foo()}) {}
> }
> }
>
> Without the array special casing, clang crashes while parsing this:
> clang:
> third_party/llvm/llvm/tools/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:322:
> void clang::ento::ExprEngine::VisitCXXDestructor(clang::QualType, const
> clang::ento::MemRegion *, const clang::Stmt *, bool,
> clang::ento::ExplodedNode *, clang::ento::ExplodedNodeSet &): Assertion
> `RecordDecl && "Only CXXRecordDecls destructors can be processed"' failed.
>
> I've updated the special casing to be a bit simpler, but I think it (or
> something like it) is necessary to avoid crashing on initializer_lists.
> What do you think?
>
> Your recommendation of skipping destructors in VisitCXXConstructExpr
> worked perfectly: I've updated getRegionForConstructedObject in
> ExprEngineCXX.cpp to recurse when it finds a destructor, and that fixed the
> spurious garbage return value warnings, which now means that this patch
> eliminates all large scale false positives on this project's codebase.
> Thanks for your help!
>
> If there are any parts of this change that you'd like to split out and
> individually commit (like the dtor handling in PathDiagnostic.cpp) that
> sounds fine to me. I'm happy with whatever patching combination you'd
> prefer if you help me through the splitting and committing process.
>
> Thanks again for your help with this!
>
> -Alex
>
>
> On Tue, Mar 4, 2014 at 7:45 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>
>> 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/20140323/aacdd21a/attachment.html>
More information about the cfe-dev
mailing list