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

[Jordan Rose]

On Feb 14, 2014, at 9:35 , Manuel Klimek <klimek at google.com> wrote:

> On Fri, Feb 14, 2014 at 6:09 PM, Jordan Rose <jordan_rose at apple.com> wrote:
> 
> On Feb 14, 2014, at 8:19 , David Blaikie <dblaikie at gmail.com> wrote:
> 
>> 
>> 
>> 
>> 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)
>>  
> 
> It isn't just Objective-C code. Pavel's patch revealed a hole in liveness tracking that meant impossible paths were getting reported, but only in very specific circumstances involving nested logical expressions. This can happen in any language mode. None of us have put in the time to do more than triage this and talk about possible solutions, which were in my original e-mail to Daniel. (The liveness issue is described in PR18159, and the original ||/&& discussion is PR11645. It's entirely possible to come up with answers that don't fix the liveness issue, too.)
> 
> If temporary destructor false positives are bad, how about a path that explicitly says "if (!x)" and then "if (x)"? That's just not a trade worth making.
> 
> So if I understand you correctly, you're saying that a path that says "if (!x)" and then "if (x)" will be handled incorrectly with Pavel's patch?=

That's correct, for particular values of "x".

> My question about the option of putting it in under a flag / getting a better idea about the type of regressions it causes comes from the observation that when we included Pavel's patch the false positives went drastically down (at least on our code bases).

You can still turn on temporary destructors with the cfg-temporary-dtors config option. I'm just not sure what will happen when there are logical expressions containing types with cleanups.

I am very much against putting something subtly wrong in ways we don't fully understand back into the analyzer to get around a known issue with well-defined limitations, even if that known issue makes the analyzer unusable on a particular codebase of interest. I would be thrilled if someone found the time to (re-)solve the problem of cleanups within expressions with flow control, but to be fully up-front about this I don't think Ted, Anna, or I will be able to dedicate time to this for the next few months.

On the other hand, if you can find someone like Daniel who is able to spend a good chunk of time working on this, I'll try to be more available to answer questions and provide feedback, because you're right: not having this feature means the analyzer can't reliably be used on a whole class of codebases.

Jordan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140218/aec99ca4/attachment.html>