[compiler-rt] r177132 - Fix a virtual destructor warning.

[Alexey Samsonov]

On Tue, Mar 19, 2013 at 8:12 PM, David Blaikie <dblaikie at gmail.com> wrote:

> On Tue, Mar 19, 2013 at 3:11 AM, Alexey Samsonov <samsonov at google.com>
> wrote:
> >
> > On Fri, Mar 15, 2013 at 11:07 AM, Eric Christopher <echristo at gmail.com>
> > wrote:
> >>
> >>
> >>
> >>
> >> On Thu, Mar 14, 2013 at 11:21 PM, Alexey Samsonov <samsonov at google.com>
> >> wrote:
> >>>
> >>>
> >>> On Fri, Mar 15, 2013 at 4:20 AM, Eric Christopher <echristo at gmail.com>
> >>> wrote:
> >>>>
> >>>> Author: echristo
> >>>> Date: Thu Mar 14 19:20:17 2013
> >>>> New Revision: 177132
> >>>>
> >>>> URL: http://llvm.org/viewvc/llvm-project?rev=177132&view=rev
> >>>> Log:
> >>>> Fix a virtual destructor warning.
> >>>>
> >>>> Patch by Manuel Klimek!
> >>>
> >>>
> >>> Thanks! Should we make this warning fire in CMake build +
> >>> LLVM_ENABLE_WERROR?
> >>>
> >>
> >>
> >> Mmm. Probably.
> >
> >
> > Done in r177385.
>
> Sorry I didn't reply earlier - but just to check, do we want this?
>
> Clang has a warning about virtual destruction as well that might be
> sufficiently accurate without having to provide virtual dtors for
> types that are used, but not owned, virtually. Hmm, seems that's off
> by default too.
>
> So, alternate proposal:
>
> disable GCC's -Wnon-virtual-dtor
> enable Clang's -Wdelete-non-virtual-dtor
>
> It catches things like:
>
> struct foo {
>   virtual void bar();
> };
>
> struct derived: foo { };
>
> int main() {
>   foo *f = new derived;
>   delete f;
> }
>
> (without incorrectly warning on: void func(foo& f) { f.bar(); } int
> main() { derived d; func(d); })
>
> Which could still be a false positive if such a pointer doesn't
> actually point to various derived objects at runtime, but is probably
> still a better signal than -Wnon-virtual-dtor.
>

FTR, -Wnon-virtual-dtor didn't show any warnings on LLVM/Clang code, and
3 (seemingly not true positive) warnings on lld code (which I silenced).

Just to make sure we're on the same page - do you want
to add "-Wno-non-virtual-dtor -Wdelete-non-virtual-dtor" to compile flags?

This sounds fine to me, and it may indeed help with false positives, but
my motivation for the change was patch by Manuel - it seems that some people
actually compile LLVM codebase with -Wnon-virtual-dtor and expect it
to compile w/o warnings.


>
> >
> >>
> >>
> >> -eric
> >>
> >>>>
> >>>>
> >>>> Modified:
> >>>>
> compiler-rt/trunk/lib/sanitizer_common/sanitizer_thread_registry.cc
> >>>>     compiler-rt/trunk/lib/sanitizer_common/sanitizer_thread_registry.h
> >>>>
> >>>> Modified:
> >>>> compiler-rt/trunk/lib/sanitizer_common/sanitizer_thread_registry.cc
> >>>> URL:
> >>>>
> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_thread_registry.cc?rev=177132&r1=177131&r2=177132&view=diff
> >>>>
> >>>>
> ==============================================================================
> >>>> ---
> compiler-rt/trunk/lib/sanitizer_common/sanitizer_thread_registry.cc
> >>>> (original)
> >>>> +++
> compiler-rt/trunk/lib/sanitizer_common/sanitizer_thread_registry.cc
> >>>> Thu Mar 14 19:20:17 2013
> >>>> @@ -22,6 +22,8 @@ ThreadContextBase::ThreadContextBase(u32
> >>>>    name[0] = '\0';
> >>>>  }
> >>>>
> >>>> +ThreadContextBase::~ThreadContextBase() {}
> >>>> +
> >>>>  void ThreadContextBase::SetName(const char *new_name) {
> >>>>    name[0] = '\0';
> >>>>    if (new_name) {
> >>>>
> >>>> Modified:
> >>>> compiler-rt/trunk/lib/sanitizer_common/sanitizer_thread_registry.h
> >>>> URL:
> >>>>
> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_thread_registry.h?rev=177132&r1=177131&r2=177132&view=diff
> >>>>
> >>>>
> ==============================================================================
> >>>> --- compiler-rt/trunk/lib/sanitizer_common/sanitizer_thread_registry.h
> >>>> (original)
> >>>> +++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_thread_registry.h
> >>>> Thu Mar 14 19:20:17 2013
> >>>> @@ -34,6 +34,7 @@ enum ThreadStatus {
> >>>>  class ThreadContextBase {
> >>>>   public:
> >>>>    explicit ThreadContextBase(u32 tid);
> >>>> +  virtual ~ThreadContextBase();
> >>>>
> >>>>    const u32 tid;  // Thread ID. Main thread should have tid = 0.
> >>>>    u64 unique_id;  // Unique thread ID.
> >>>>
> >>>>
> >>>> _______________________________________________
> >>>> llvm-commits mailing list
> >>>> llvm-commits at cs.uiuc.edu
> >>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >>>
> >>>
> >>>
> >>>
> >>> --
> >>> Alexey Samsonov, MSK
> >>
> >>
> >
> >
> >
> > --
> > Alexey Samsonov, MSK
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
>



-- 
Alexey Samsonov, MSK
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130319/7811f1e2/attachment.html>