<div dir="ltr"><div dir="ltr">On Tue, Sep 7, 2021 at 3:48 PM Fangrui Song <<a href="mailto:i@maskray.me">i@maskray.me</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Tue, Sep 7, 2021 at 11:56 AM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
><br>
> On Wed, Sep 1, 2021 at 12:30 AM Martin Storsjö via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>><br>
>><br>
>> > On Sep 1, 2021, at 10:21, Fangrui Song <<a href="mailto:i@maskray.me" target="_blank">i@maskray.me</a>> wrote:<br>
>> ><br>
>> > On Tue, Aug 31, 2021 at 11:58 PM Martin Storsjö <<a href="mailto:martin@martin.st" target="_blank">martin@martin.st</a>> wrote:<br>
>> >><br>
>> >> This causes lots new build warnings when building with GCC (noted with GCC 7 and 9), like these:<br>
>> >><br>
>> >> ../include/llvm/Analysis/ScalarEvolution.h:200:7: warning: ‘class llvm::SCEVPredicate’ has virtual functions and accessible non-virtual destructor [-Wnon-virtual-dtor]<br>
>> >> class SCEVPredicate : public FoldingSetNode {<br>
>> >> ^~~~~~~~~~~~~<br>
>> >> ../include/llvm/Analysis/ScalarEvolution.h:267:7: warning: base class ‘class llvm::SCEVPredicate’ has accessible non-virtual destructor [-Wnon-virtual-dtor]<br>
>> >> class SCEVEqualPredicate final : public SCEVPredicate {<br>
>> >> ^~~~~~~~~~~~~~~~~~<br>
>> >> ../include/llvm/Analysis/ScalarEvolution.h:304:7: warning: base class ‘class llvm::SCEVPredicate’ has accessible non-virtual destructor [-Wnon-virtual-dtor]<br>
>> >> class SCEVWrapPredicate final : public SCEVPredicate {<br>
>> >> ^~~~~~~~~~~~~~~~~<br>
>> >> ../include/llvm/Analysis/ScalarEvolution.h:399:7: warning: base class ‘class llvm::SCEVPredicate’ has accessible non-virtual destructor [-Wnon-virtual-dtor]<br>
>> >> class SCEVUnionPredicate final : public SCEVPredicate {<br>
>> >> ^~~~~~~~~~~~~~~~~~<br>
>> >><br>
>> >> 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?).<br>
>> >><br>
>> >> // Martin<br>
>> ><br>
>> > g++ 5.1, 7.5, and 9.5 -Wnon-virtual-dtor -std=c++14 do not warn on the<br>
>> > tested code:<br>
>> ><br>
>> > class base {public: virtual void anchor();protected: ~base();};<br>
>> > class derived final : public base { public: ~derived();};<br>
>> > int main() { return 0; }<br>
>> ><br>
>> > Can you check -Wnon-virtual-dtor was previously disabled for GCC?<br>
>> ><br>
>> > I haven't looked at include/llvm/Analysis/ScalarEvolution.h<br>
>><br>
>> 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.<br>
>><br>
>> Ok, so then it sounds like the warning is potentially legit for those classes then?<br>
><br>
><br>
> 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)<br>
><br>
> 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.<br>
<br>
Agree, but perhaps with either of the two forms:<br>
<br>
(a) enable -Wnon-virtual-dtor only for Clang<br>
(b) enable -Wnon-virtual-dtor for GCC as well if<br>
<a href="https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102168" rel="noreferrer" target="_blank">https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102168</a> example is good<br></blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Are you willing to push such a commit? :)<br></blockquote><div><br>If you could, that'd be great, thanks! Otherwise I'll see what I can figure out. </div></div></div>