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

Fri Apr 4 19:14:33 PDT 2014

Great, thanks for the improvement!

I've started to dive into live variable analysis, but I think it'll take me
a while to dig into this one. I'll ping this thread once I have more
well-researched questions.

-Alex

On Fri, Apr 4, 2014 at 7:08 PM, Jordan Rose <jordan_rose at apple.com> wrote:

> Here's a trivial modification that shows two temporary destructors:
>
>   bool testNamedCustomDestructor2() {
>     if (CheckCustomDestructor c = (CheckCustomDestructor(),
> CheckCustomDestructor()))
>       return true;
>     return false;
>   }
>
> This also works with your patch, so I added it to dtor.cpp and committed
> everything in r205661. Thanks!
> Jordan
>
> On Apr 3, 2014, at 11:55 , Alex McCarthy <alexmc at google.com> wrote:
>
> I've attached a looping version of this patch. What do you think? I'm not
> a fan of the code duplication, but it's not too bad.
>
> The logic change in this patch actually doesn't affect the test that I
> uncommented: it fixes another bug that appears when analyzing dtor.cpp
> with cfg-temporary-dtors=true. If you apply just the test change in this
> patch and revert the lib change, you'll see this failure (noise omitted):
>
> ******************** TEST 'Clang :: Analysis/dtor.cpp' FAILED
> ********************
> error: 'warning' diagnostics seen but not expected:
>   File /Users/alexmc/Documents/dev/llvm/tools/clang/test/Analysis/dtor.cpp
> Line 394: Undefined or garbage value returned to caller
>
> Here's the line that causes the error:
>   struct CheckCustomDestructor {
>     bool bool_;
>     CheckCustomDestructor():bool_(true) {}
>     ~CheckCustomDestructor();
>     operator bool() const { return bool_; } // warning generated here
>   };
>
> The lib change is the fix you recommended in
> http://permalink.gmane.org/gmane.comp.compilers.clang.devel/35520 for
> this error. Now that the error's fixed, it's safe to
> use cfg-temporary-dtors=true to analyze the whole file. Does that make
> sense? Do you have any recommendations on how I should rework this patch?
>
> Thanks!
>
> -Alex
>
>
> On Tue, Apr 1, 2014 at 8:10 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>
>> I like using a loop instead of a recursion; that's all. I'm not sure I
>> had thought of the "multiple destructors in a row" case before I said my
>> previous advice. I'd be fine to commit this as is...
>>
>> ...except I don't see how it fixes the test case, and once I can see that
>> I'd like to have a more complicated test case here as well.
>>
>> Jordan
>>
>>
>> On Apr 1, 2014, at 11:05 , Alex McCarthy <alexmc at google.com> wrote:
>>
>> Maybe I misinterpreted your previous feedback: I also don't the "check
>> one ahead" approach in the patch.
>>
>> Here's another version that uses a recursive solution like the original
>> patch, which I think handles this much more elegantly. It's an early
>> recurse and return so there shouldn't be much wasted extra work. What do
>> you think?
>>
>> -Alex
>>
>>
>> On Tue, Apr 1, 2014 at 9:46 AM, Jordan Rose <jordan_rose at apple.com>wrote:
>>
>>> I've committed all the cleanup patches, but I'm not sure the destructor
>>> patch is correct. What if there were two temporaries within the assignment?
>>> Wouldn't we see CFGImplicitDtors for both of them? (Maybe a loop can solve
>>> this problem, although it's kind of unfortunate.)
>>>
>>> Jordan
>>>
>>>
>>> On Apr 1, 2014, at 6:50 , Alex McCarthy <alexmc at google.com> wrote:
>>>
>>> And two more patches: the first is the refactor to split out
>>> a getRegionForConstructedObject from ExprEngineCXX.cpp: this can be applied
>>> to head.
>>>
>>>  The second fixes spurious garbage value warnings when using temporary
>>> destructor analysis. It has to be applied after both the first refactoring
>>> attached to this mail
>>> (0001-Refactor-out-a-getRegionForConstructedObject-helper-.patch) and the
>>> previous patch to add new tests
>>> (0001-Prep-work-for-fixing-C-analysis-handling-of-temporar.patch).
>>>
>>> -Alex
>>>
>>>
>>> On Mon, Mar 31, 2014 at 8:29 PM, Alex McCarthy <alexmc at google.com>wrote:
>>>
>>>> And here's a second patch, to be applied after the first, that fixes
>>>> the crash while processing c++11 initializer lists. Thanks for explaining
>>>> the intent of makeZeroElementRegion: I'm not sure why the FIXME I removed
>>>> was there in the first place, but this seems like a much better fix than
>>>> the one I had before.
>>>>
>>>> -Alex
>>>>
>>>>
>>>> On Mon, Mar 31, 2014 at 8:03 PM, Alex McCarthy <alexmc at google.com>wrote:
>>>>
>>>>> Here's a new version of the simple patch. nullptr tests are gone, and
>>>>> I've uncommented tests that generate incorrect warnings. As discussed, I've
>>>>> left a few tests commented out because they trigger crashes or asserts: I
>>>>> think having the tests there but commented out serves as a cheap/poor form
>>>>> of documentation of the current state of these bugs in clang, and it should
>>>>> make upcoming patches that contain fixes smaller and easier to read. Let me
>>>>> know if you'd like me to remove those too.
>>>>>
>>>>> Since I don't have clang/llvm commit access, are you able to commit
>>>>> this on my behalf once this looks good to oyu?
>>>>>
>>>>> Thanks!
>>>>>
>>>>> -Alex
>>>>>
>>>>>
>>>>> On Mon, Mar 31, 2014 at 3:47 PM, Jordan Rose <jordan_rose at apple.com>wrote:
>>>>>
>>>>>>
>>>>>> On Mar 31, 2014, at 10:40, Alex McCarthy <alexmc at google.com> wrote:
>>>>>>
>>>>>> > Thanks for your advice, Jordan.
>>>>>> >
>>>>>> > I've split the NORETURN printing changes to CFG.cpp and the new
>>>>>> test additions into a separate patch (attached). Hopefully this should be a
>>>>>> safe no-op from a behavior perspective: debug output changes slightly, and
>>>>>> the new test cases more accurately reflects current clang behavior. Does
>>>>>> this look safe to commit?
>>>>>>
>>>>>> Rather than commenting out tests, I'd prefer you put them in with the
>>>>>> "wrong" expected-warnings, and then put a FIXME next to the warnings.
>>>>>> Commented-out tests become dead tests all too easily.
>>>>>>
>>>>>> I would drop the tests from nullptr.cpp. That file's for testing
>>>>>> nullptr and nullptr_t specifically, not null pointers in general, and the
>>>>>> new tests don't actually test any new behavior if we already believe that
>>>>>> nullptr properly converts to a null pointer value.
>>>>>>
>>>>>> Jordan
>>>>>>
>>>>>>
>>>>>
>>>>
>>> <0001-Refactor-out-a-getRegionForConstructedObject-helper-.patch>
>>> <0002-Fix-spurious-Undefined-or-garbage-value-returned-to-.patch>
>>>
>>>
>>>
>> <0002-Fix-spurious-Undefined-or-garbage-value-returned-to-.patch>
>>
>>
>>
> <0001-Loop-Fix-spurious-Undefined-or-garbage-value-returned-to-.patch>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140404/f18b5651/attachment.html>