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

[David Blaikie via llvm-commits]

On Tue, Sep 7, 2021 at 3:48 PM Fangrui Song <i at maskray.me> wrote:

> On Tue, Sep 7, 2021 at 11:56 AM David Blaikie <dblaikie at gmail.com> wrote:
> >
> > 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.
>
> Agree, but perhaps with either of the two forms:
>
> (a) enable -Wnon-virtual-dtor only for Clang
> (b) enable -Wnon-virtual-dtor for GCC as well if
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102168 example is good
>

Sure, I don't mind if its phrased compiler-agnostically with a check that
that particular behavior doesn't show up. I think we have a bunch of other
warning enable/disable functionality based on small test cases like that
too, so there should be some prior-art to work from for that.


> Are you willing to push such a commit? :)
>

If you could, that'd be great, thanks! Otherwise I'll see what I can figure
out.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210907/3f1fe2cc/attachment.html>