[llvm] r254592 - Fix class SCEVPredicate has virtual functions and accessible non-virtual destructor.

[David Blaikie via llvm-commits]

On Thu, Dec 3, 2015 at 9:32 AM, Andy Gibbs <andyg1001 at hotmail.co.uk> wrote:

> On Thursday, December 03, 2015 6:06 PM, David Blaikie wrote:
>
> On Thu, Dec 3, 2015 at 9:00 AM, Andy Gibbs wrote:
>
>> On Thursday, December 03, 2015 5:29 PM, David Blaikie wrote:
>> On Thu, Dec 3, 2015 at 12:20 AM, Andy Gibbs wrote:
>>
>>> Author: andyg
>>>> Date: Thu Dec  3 02:20:20 2015
>>>> New Revision: 254592
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=254592&view=rev
>>>> Log:
>>>> Fix class SCEVPredicate has virtual functions and accessible
>>>> non-virtual destructor.
>>>>
>>>>
>>> Wait - why did the dtor need to be made virtual at all? These objects
>>> are never
>>> destroyed polymorphically, so far as I can tell.
>>>
>>> Could we just disable the GCC warning that was firing here instead? (if I
>>> understand correctly and this was a GCC warning)
>>>
>>
>> Fine, but IMHO the warning is a valid one.  The code may not currently
>> destroy these
>> objects polymorphically, but if that changes in future (for whatever
>> reason) then
>> its a potential memory leak.  Its one of those "golden rule" moments,
>> isn't it? :o)
>>
>
> That's why the dtor is protected (& the derived classes with public
> non-virtual dtors are final). This pattern is used quite a bit in LLVM &
> the compiler diagnostics support it.
>
> Both Clang and GCC don't warn in that case - but GCC does warn if that
> class then has any friends.
>
> I don't see that having a friend or two should change the tradeoff here -
> you've exposed your internal details to those trusted pieces of code that,
> much liket he class itself, are restricted to not doing the bad thing.
>
> Not quite sure what golden rule(s) you're referring to.
>
>
> Please don't get me wrong here, I accept all your technical arguments even
> if I lean to a different stand-point, and if the decision is made to roll
> back this change, that's ok.
>
> However, GCC *does* warn here.  My personal feeling is that I would rather
> see the GCC warnings in this case and not disable them simply because if
> they are disabled, then they are disabled throughout the project and
> somewhere where it would correctly pick up a real issue may be lost.  That
> said, I compile with -Werror mostly, so for me its not great to see the
> warnings either :o)
>

We still have Clang that (arguably) correctly implements the warning
without the friend exception that GCC implements.

My basic bar would be: Either we consider GCC's behavior correct and we fix
Clang's warning to do the same (& keep your code change). Or we consider
GCC's behavior to be incorrect and we undo this change and disable GCC's
warning.


>
> So, I come back to my initial stance which is it isn't really "adding
> features"
>

Well, it adds the feature of polymorphic destruction for this
type/hierarchy.


> and there is (unless you can correct me here) no tangible overhead,
>

dynamic dispatch running the dtor is something, though it's unlikely to be
significant for this class


> just additional safety.  No bad thing, perhaps?  But, I'm in your hands...
> let me know your decision!
>

My usual reason to push back against changes like this is the precedent - I
don't want us to keep making sub-optimal changes to placate a warning.
Death by a thousand cuts & all that (though there's probably only tens of
cases of non-virtual dtors in polymorphic hierarchies across LLVM and
Clang, to be fair)

I'd rather discuss these things & come to an understanding/agreement, but
if a request is simpler, I think this change should be reverted and, if
you'd like to keep the GCC build warning-clean, a patch to disable this
warning would be acceptable. Clang's warnings will continue to catch the
other cases & self-hosting buildbots will flag them soon enough.

- David


>
> Thanks
> Andy
>
>
>> Rather than disable the warning, which could open up potential problems
>> elsewhere
>> not even related to this class,
>
>
> Except that it's highly restricted because the dtor is protected and the
> derived classes are final.
>
>
>> I think it is better to fix the issue, even if it is
>> technically/arguably a non-issue in this case.
>>
>> Its not like there is a huge overhead (code size, execution time, ...) to
>> the fix.
>> The classes already have vtables due to the other virtual methods, for
>> example.
>>
>
> But we shouldn't be adding features just because of a compiler warning -
> we should add features if/when we need them.
>
> - Dave
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151203/d5de8cdf/attachment.html>