[llvm] 4bb5f44 - [CMake] Remove unneeded -Wnon-virtual-dtor availability check

[David Blaikie via llvm-commits]

On Wed, Sep 1, 2021 at 12:30 AM Martin Storsjö via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

>
>
> > On Sep 1, 2021, at 10:21, Fangrui Song <i at maskray.me> wrote:
> >
> > On Tue, Aug 31, 2021 at 11:58 PM Martin Storsjö <martin at martin.st>
> wrote:
> >>
> >> This causes lots new build warnings when building with GCC (noted with
> GCC 7 and 9), like these:
> >>
> >> ../include/llvm/Analysis/ScalarEvolution.h:200:7: warning: ‘class
> llvm::SCEVPredicate’ has virtual functions and accessible non-virtual
> destructor [-Wnon-virtual-dtor]
> >> class SCEVPredicate : public FoldingSetNode {
> >>       ^~~~~~~~~~~~~
> >> ../include/llvm/Analysis/ScalarEvolution.h:267:7: warning: base class
> ‘class llvm::SCEVPredicate’ has accessible non-virtual destructor
> [-Wnon-virtual-dtor]
> >> class SCEVEqualPredicate final : public SCEVPredicate {
> >>       ^~~~~~~~~~~~~~~~~~
> >> ../include/llvm/Analysis/ScalarEvolution.h:304:7: warning: base class
> ‘class llvm::SCEVPredicate’ has accessible non-virtual destructor
> [-Wnon-virtual-dtor]
> >> class SCEVWrapPredicate final : public SCEVPredicate {
> >>       ^~~~~~~~~~~~~~~~~
> >> ../include/llvm/Analysis/ScalarEvolution.h:399:7: warning: base class
> ‘class llvm::SCEVPredicate’ has accessible non-virtual destructor
> [-Wnon-virtual-dtor]
> >> class SCEVUnionPredicate final : public SCEVPredicate {
> >>       ^~~~~~~~~~~~~~~~~~
> >>
> >> I haven’t dug in to see whether this is the exact issue that the
> removed comment referred to or if it’s something else (or if the code
> should be fixed?).
> >>
> >> // Martin
> >
> > g++ 5.1, 7.5, and 9.5 -Wnon-virtual-dtor -std=c++14 do not warn on the
> > tested code:
> >
> > class base {public: virtual void anchor();protected: ~base();};
> > class derived final : public base { public: ~derived();};
> > int main() { return 0; }
> >
> > Can you check -Wnon-virtual-dtor was previously disabled for GCC?
> >
> > I haven't looked at include/llvm/Analysis/ScalarEvolution.h
>
> Previously we only added -Wnon-virtual-dtor within the condition `if (NOT
> CMAKE_COMPILER_IS_GNUCXX AND NOT WIN32)`, so it was never added for GCC
> before afaik.
>
> Ok, so then it sounds like the warning is potentially legit for those
> classes then?
>

Based on a difference in behavior with the GCC warning (it warns on final
classes/those with protected destructors (both Clang and GCC generally
suppress the warning in this case) if the type has a friend - under the
assumption that the friend might violate the constraints of the type and
try to destroy it polymorphically - I think this is a bug/misfeature/not a
constraint I'd want to enforce in LLVM's codebase)

Given the friend "bug"/implementation choice in GCC's version of this
warning - I think we should rollback the fixes (since they aren't needed -
they pessimize code unnecessarily) and disable this warning under gcc.

Looks like all these patches might've been unnecessary/workarounds for the
GCC behavior:
https://reviews.llvm.org/rG4852c770fe87
https://github.com/llvm/llvm-project/commit/6df09d6ccbc0cb72d3278cafb592e9bc0e6b84a1
https://github.com/llvm/llvm-project/commit/4852c770fe8703145dd2a35395985646ce57a454
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210907/62aafe70/attachment.html>