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

Thu Apr 3 11:55:54 PDT 2014

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>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140403/76c83bb1/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Loop-Fix-spurious-Undefined-or-garbage-value-returned-to-.patch
Type: application/octet-stream
Size: 2414 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140403/76c83bb1/attachment.obj>