[cfe-dev] [analyzer] Thoughts on Temporary Destructors

[David Blaikie]

On Fri, Feb 14, 2014 at 1:31 AM, Manuel Klimek <klimek at google.com> wrote:

> Ted, Jordan, Anna: ping.
>
> This is really important for us - we love the analyzer, but without this
> patch, we basically cannot use it (apart from maintaining our own private
> fork, which we would rather not do, if at all possible). Every time
> somebody follows a code path that leads to an incorrect handling of
> temporary destructors, we loose a bunch of engineering time - might we be
> able to get this patch re-applied under a flag?
>

I haven't at all looked at the actual patch or the false positives, but a
few questions come to mind:

My understanding is that this helps fix C++ quality, but regresses some
ObjC cases, yes?

If so, why don't we only do this when compiling C++ (and, maybe, ObjC++ - I
assume it helps more than it hurts there) - in that way we wouldn't regress
ObjC quality and wouldn't need a flag?

What are the specific ObjC regressions? Are they a matter of luck in the
current design, or intentional but the design that works for them is
inflexible to this enhancement? (this is a difficult question to explain -
but I'm kind of drawing an analogy from the LLVM optimization pipeline -
sometimes improving one optimization exposes flaws in another, through no
fault of the improvement. We still care about the overall regression and
work diligently to fix or revert, etc)


>
> /Manuel
>
>
>
> On Tue, Feb 11, 2014 at 10:57 AM, Manuel Klimek <klimek at google.com> wrote:
>
>> So, given the 50% false positive rate for C++ code I'd be curious what
>> regressions were there in C/Obj-C code that tip the scale towards reverting
>> the patch rather than living with the regressions.
>>
>> Thanks!
>> /Manuel
>>
>>
>> On Mon, Feb 10, 2014 at 11:38 AM, Daniel Connelly <dconnelly at google.com>wrote:
>>
>>> (cc: klimek at google.com, djasper at google.com)
>>>
>>> As a bit of a follow-up, we are still very interested in getting this
>>> worked out, as it's a significant barrier to the Analyzer's usefulness. All
>>> of our C++ projects make extensive use of custom assertions. For example,
>>>
>>> CHECK(condition) << "something happened with the hydrospanners";
>>>
>>> which logs a message and kills the program if the condition isn't met.
>>> None of these custom assertions are pruning code paths due to the missing
>>> conditional expression support. For example, when building Chromium, the
>>> difference between running without and with the reverted patch is 1982
>>> reported errors vs. 3210 reported errors. This number is a bit
>>> conservative, since we also have this problem in dependent libraries that
>>> don't (but could) use analyzer_noreturn on their assertions; the real cost
>>> is that about 50% of all errors reported by the analyzer are false
>>> positives of this form.
>>>
>>>
>>> On Fri, Jan 31, 2014 at 11:32 AM, Daniel Connelly <dconnelly at google.com>wrote:
>>>
>>>> After digging into this for a couple of weeks across the 2013/2014
>>>> boundary, I now doubt that my 20% time will be enough to properly implement
>>>> this support. Anybody who's interested should feel free to work on this
>>>> instead, as I'll not be moving forward with it. Jordan, thanks for the
>>>> pointers and the summary of the problem!
>>>>
>>>>
>>>> On Wed, Dec 18, 2013 at 1:55 AM, Jordan Rose <jordan_rose at apple.com>wrote:
>>>>
>>>>>
>>>>> On Dec 16, 2013, at 6:57 , Daniel Connelly <dconnelly at google.com>
>>>>> wrote:
>>>>>
>>>>> On Fri, Dec 13, 2013 at 7:13 PM, Jordan Rose <jordan_rose at apple.com>wrote:
>>>>>
>>>>>>
>>>>>> In discussing the first problem, Ted and Anna realized that the
>>>>>> liveness information for nested && and || expressions was still incorrect:
>>>>>> the outermost expression may potentially depend on all of the inner
>>>>>> expressions, but that means that those sub-expressions are *always* considered
>>>>>> live (since expressions are marked dead, in a reverse analysis, at the
>>>>>> point of evaluation, and there are paths that don't evaluate a particular
>>>>>> sub-expression in a complex logical expression). This wasn't an issue
>>>>>> before because && and || didn't actually use those expressions to compute
>>>>>> their value, but it's still incorrect.
>>>>>>
>>>>>
>>>>> Is this http://llvm.org/bugs/show_bug.cgi?id=18159 ?
>>>>>
>>>>>
>>>>> Yes.
>>>>>
>>>>> Thanks for the overview. This is all pretty new to me, so I'm going to
>>>>> dig into the problem for a couple of days and hopefully come up with some
>>>>> intelligent questions.
>>>>>
>>>>>
>>>>> Sorry for the blast of information. If you want an easier place to
>>>>> start playing with the analyzer, you could try hacking on productizing
>>>>> SimpleStreamChecker. (And if you haven't watched our talk from last year's
>>>>> LLVM dev meeting, I definitely suggest taking the time to do so. It's aimed
>>>>> at checker writers, but still describes the general structure of the
>>>>> analyzer. It's linked near the top of
>>>>> http://clang-analyzer.llvm.org/checker_dev_manual.html)
>>>>>
>>>>> Jordan
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Daniel Connelly | Software Engineer - Chrome | dconnelly at google.com |
>>>>
>>>> Google Germany GmbH
>>>> Dienerstr. 12
>>>> 80331 München
>>>>
>>>> Registergericht und -nummer: Hamburg, HRB 86891
>>>> Sitz der Gesellschaft: Hamburg
>>>> Geschäftsführer: Graham Law, Christine Elizabeth Flores
>>>>
>>>>
>>>
>>>
>>> --
>>> Daniel Connelly | Software Engineer - Chrome | dconnelly at google.com |
>>>
>>> Google Germany GmbH
>>> Dienerstr. 12
>>> 80331 München
>>>
>>> Registergericht und -nummer: Hamburg, HRB 86891
>>> Sitz der Gesellschaft: Hamburg
>>> Geschäftsführer: Graham Law, Christine Elizabeth Flores
>>>
>>>
>>
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140214/73e6c461/attachment.html>