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

Sat Apr 5 18:16:15 PDT 2014

It looks like the simple case test in r205661 was commented out in favor of
the more complex test. Here's a patch to uncomment the simple test. We can
also delete the simple case if you'd prefer.

-Alex

On Fri, Apr 4, 2014 at 7:14 PM, Alex McCarthy <alexmc at google.com> wrote:

> 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/20140405/62cca3cc/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Re-enable-simple-case-test-that-was-introduced-in-r2.patch
Type: application/octet-stream
Size: 1106 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140405/62cca3cc/attachment.obj>