[chromium-dev] Re: PSA: Virtual dispatch doesn't work (as you might expect) in destructors!
On Fri, Oct 30, 2009 at 5:09 PM, David Levin wrote: > > > On Fri, Oct 30, 2009 at 3:59 PM, Antoine Labour wrote: >> >> >> On Fri, Oct 30, 2009 at 3:54 PM, Jeremy Orlow wrote: >>> >>> On Fri, Oct 30, 2009 at 3:46 PM, Scott Hess wrote: Just to be clear for those of us who are wobbly on C++, this is because during the constructor or destructor, your object is of the class in question, NOT of the class it will finally be, because in the constructor the subclass has not been constructed, yet, and in the destructor the subclass was already destructed. So calling to the subclass virtual implementation would be bad. Scott Meyers says: http://www.artima.com/cppsource/nevercall.html Is there any way we could modify an object to assert that it can't happen in development? Like scoped_vtable_killer declared in the constructor and destructor which makes calling a virtual method on that object fatal? >>> >>> Or is there any sort of built in compiler warning that we could turn on? >>> I did a bit of searching and was really surprised that I couldn't find any >>> mention of such a thing. >> >> The compiler could find if it's called directly from the destructor, but >> there's no way it'd find your case ! The virtual call happens on another >> thread. >> To Scott's question: you can blit NULL into the vtable field, if you know >> where it is (it's not too hard, but depends on the compiler). You'll know if >> you call it - you'll die. > > For the original issue this doesn't work b/c for virtual calls in the > constructor/destructor, the compiler may optimize them to be non-virtual. Good point. Given this, I'd suggest that the "poison vtable" approach would be better implemented as a compiler feature. When the flag is enabled, the compiler would specifically avoid these optimizations. Perhaps this would only work in debug builds. Erik > Also, it looks like this keeps biting > chromium: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33475 > >> >> Better yet, you could have a static table of functions that print a >> message before dying and blit that one, but that gets trickier. >> Antoine >> > > > > --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: PSA: Virtual dispatch doesn't work (as you might expect) in destructors!
On Fri, Oct 30, 2009 at 3:59 PM, Antoine Labour wrote: > > > On Fri, Oct 30, 2009 at 3:54 PM, Jeremy Orlow wrote: > >> On Fri, Oct 30, 2009 at 3:46 PM, Scott Hess wrote: >> >>> Just to be clear for those of us who are wobbly on C++, this is >>> because during the constructor or destructor, your object is of the >>> class in question, NOT of the class it will finally be, because in the >>> constructor the subclass has not been constructed, yet, and in the >>> destructor the subclass was already destructed. So calling to the >>> subclass virtual implementation would be bad. >>> >>> Scott Meyers says: http://www.artima.com/cppsource/nevercall.html >>> >>> Is there any way we could modify an object to assert that it can't >>> happen in development? Like scoped_vtable_killer declared in the >>> constructor and destructor which makes calling a virtual method on >>> that object fatal? >>> >> >> Or is there any sort of built in compiler warning that we could turn on? >> I did a bit of searching and was really surprised that I couldn't find any >> mention of such a thing. >> > > The compiler could find if it's called directly from the destructor, but > there's no way it'd find your case ! The virtual call happens on another > thread. > > To Scott's question: you can blit NULL into the vtable field, if you know > where it is (it's not too hard, but depends on the compiler). You'll know if > you call it - you'll die. > For the original issue this doesn't work b/c for virtual calls in the constructor/destructor, the compiler may optimize them to be non-virtual. Also, it looks like this keeps biting chromium: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33475 Better yet, you could have a static table of functions that print a message > before dying and blit that one, but that gets trickier. > > Antoine > > --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: PSA: Virtual dispatch doesn't work (as you might expect) in destructors!
On Fri, Oct 30, 2009 at 3:54 PM, Jeremy Orlow wrote: > On Fri, Oct 30, 2009 at 3:46 PM, Scott Hess wrote: > >> Just to be clear for those of us who are wobbly on C++, this is >> because during the constructor or destructor, your object is of the >> class in question, NOT of the class it will finally be, because in the >> constructor the subclass has not been constructed, yet, and in the >> destructor the subclass was already destructed. So calling to the >> subclass virtual implementation would be bad. >> >> Scott Meyers says: http://www.artima.com/cppsource/nevercall.html >> >> Is there any way we could modify an object to assert that it can't >> happen in development? Like scoped_vtable_killer declared in the >> constructor and destructor which makes calling a virtual method on >> that object fatal? >> > > Or is there any sort of built in compiler warning that we could turn on? I > did a bit of searching and was really surprised that I couldn't find any > mention of such a thing. > The compiler could find if it's called directly from the destructor, but there's no way it'd find your case ! The virtual call happens on another thread. To Scott's question: you can blit NULL into the vtable field, if you know where it is (it's not too hard, but depends on the compiler). You'll know if you call it - you'll die. Better yet, you could have a static table of functions that print a message before dying and blit that one, but that gets trickier. Antoine > > On Fri, Oct 30, 2009 at 3:12 PM, Jeremy Orlow wrote: >> > I've spent a good deal of this week trying to track down what turned out >> to >> > be a simple but fairly common problem: I forgot virtual dispatch only >> > partially works in destructors. There have been several email threads >> about >> > this, but it still bites us form time to time, so I thought it was worth >> > another reminder. >> > >> > Details: >> > I subclassed ChromeThread which subclasses base::Thread. base::Thread >> calls >> > CleanUp on the thread right before termination. CleanUp is virtual. >> Both >> > ChromeThread and my class override CleanUp(). base::Thread calls Stop() >> in >> > its destructor to stop the thread (if it hasn't already been stopped). >> But >> > by the time you hit destruction, the vtable is no longer available and >> thus >> > the destructor of base::Thread (and anything it calls) does NOT have >> access >> > to the vtable of ChromeThread (or my class). So, if you don't >> explicitly >> > call Stop(), your subclass's CleanUp method will NOT be called. Thus >> the >> > thread was going away without my CleanUp method ever being called. >> > Obviously this affects more than just base::Thread. And this is also >> how >> > you can hit errors with pure virtual methods being called. >> > J >> > > >> > >> > > > > > --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: PSA: Virtual dispatch doesn't work (as you might expect) in destructors!
On Fri, Oct 30, 2009 at 3:54 PM, Jeremy Orlow wrote: > On Fri, Oct 30, 2009 at 3:46 PM, Scott Hess wrote: >> Just to be clear for those of us who are wobbly on C++, this is >> because during the constructor or destructor, your object is of the >> class in question, NOT of the class it will finally be, because in the >> constructor the subclass has not been constructed, yet, and in the >> destructor the subclass was already destructed. So calling to the >> subclass virtual implementation would be bad. >> >> Scott Meyers says: http://www.artima.com/cppsource/nevercall.html >> >> Is there any way we could modify an object to assert that it can't >> happen in development? Like scoped_vtable_killer declared in the >> constructor and destructor which makes calling a virtual method on >> that object fatal? > > Or is there any sort of built in compiler warning that we could turn on? I > did a bit of searching and was really surprised that I couldn't find any > mention of such a thing. It would have to be a really smart compiler, because you could call a function which calls another object's function which calls back to a virtual in your object. That's why I suggested a way to make vtable references fatal, so at least when doing development you could get a notification (and not check it in). -scott --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: PSA: Virtual dispatch doesn't work (as you might expect) in destructors!
On Fri, Oct 30, 2009 at 3:46 PM, Scott Hess wrote: > > Just to be clear for those of us who are wobbly on C++, this is > because during the constructor or destructor, your object is of the > class in question, NOT of the class it will finally be, because in the > constructor the subclass has not been constructed, yet, and in the > destructor the subclass was already destructed. So calling to the > subclass virtual implementation would be bad. > > Scott Meyers says: http://www.artima.com/cppsource/nevercall.html > > Is there any way we could modify an object to assert that it can't > happen in development? Like scoped_vtable_killer declared in the > constructor and destructor which makes calling a virtual method on > that object fatal? > That's an intriguing idea. It seems like you could swap the real vtable ptr out on ctor/dtor entry, and replace it with a ptr to a poisoned vtable. This sounds like debug build stuff only. > > -scott > > > On Fri, Oct 30, 2009 at 3:12 PM, Jeremy Orlow wrote: > > I've spent a good deal of this week trying to track down what turned out > to > > be a simple but fairly common problem: I forgot virtual dispatch only > > partially works in destructors. There have been several email threads > about > > this, but it still bites us form time to time, so I thought it was worth > > another reminder. > > > > Details: > > I subclassed ChromeThread which subclasses base::Thread. base::Thread > calls > > CleanUp on the thread right before termination. CleanUp is virtual. > Both > > ChromeThread and my class override CleanUp(). base::Thread calls Stop() > in > > its destructor to stop the thread (if it hasn't already been stopped). > But > > by the time you hit destruction, the vtable is no longer available and > thus > > the destructor of base::Thread (and anything it calls) does NOT have > access > > to the vtable of ChromeThread (or my class). So, if you don't explicitly > > call Stop(), your subclass's CleanUp method will NOT be called. Thus the > > thread was going away without my CleanUp method ever being called. > > Obviously this affects more than just base::Thread. And this is also how > > you can hit errors with pure virtual methods being called. > > J > > > > > > > > > --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: PSA: Virtual dispatch doesn't work (as you might expect) in destructors!
On Fri, Oct 30, 2009 at 3:46 PM, Scott Hess wrote: > Just to be clear for those of us who are wobbly on C++, this is > because during the constructor or destructor, your object is of the > class in question, NOT of the class it will finally be, because in the > constructor the subclass has not been constructed, yet, and in the > destructor the subclass was already destructed. So calling to the > subclass virtual implementation would be bad. > > Scott Meyers says: http://www.artima.com/cppsource/nevercall.html > > Is there any way we could modify an object to assert that it can't > happen in development? Like scoped_vtable_killer declared in the > constructor and destructor which makes calling a virtual method on > that object fatal? > Or is there any sort of built in compiler warning that we could turn on? I did a bit of searching and was really surprised that I couldn't find any mention of such a thing. On Fri, Oct 30, 2009 at 3:12 PM, Jeremy Orlow wrote: > > I've spent a good deal of this week trying to track down what turned out > to > > be a simple but fairly common problem: I forgot virtual dispatch only > > partially works in destructors. There have been several email threads > about > > this, but it still bites us form time to time, so I thought it was worth > > another reminder. > > > > Details: > > I subclassed ChromeThread which subclasses base::Thread. base::Thread > calls > > CleanUp on the thread right before termination. CleanUp is virtual. > Both > > ChromeThread and my class override CleanUp(). base::Thread calls Stop() > in > > its destructor to stop the thread (if it hasn't already been stopped). > But > > by the time you hit destruction, the vtable is no longer available and > thus > > the destructor of base::Thread (and anything it calls) does NOT have > access > > to the vtable of ChromeThread (or my class). So, if you don't explicitly > > call Stop(), your subclass's CleanUp method will NOT be called. Thus the > > thread was going away without my CleanUp method ever being called. > > Obviously this affects more than just base::Thread. And this is also how > > you can hit errors with pure virtual methods being called. > > J > > > > > > > --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: PSA: Virtual dispatch doesn't work (as you might expect) in destructors!
Just to be clear for those of us who are wobbly on C++, this is because during the constructor or destructor, your object is of the class in question, NOT of the class it will finally be, because in the constructor the subclass has not been constructed, yet, and in the destructor the subclass was already destructed. So calling to the subclass virtual implementation would be bad. Scott Meyers says: http://www.artima.com/cppsource/nevercall.html Is there any way we could modify an object to assert that it can't happen in development? Like scoped_vtable_killer declared in the constructor and destructor which makes calling a virtual method on that object fatal? -scott On Fri, Oct 30, 2009 at 3:12 PM, Jeremy Orlow wrote: > I've spent a good deal of this week trying to track down what turned out to > be a simple but fairly common problem: I forgot virtual dispatch only > partially works in destructors. There have been several email threads about > this, but it still bites us form time to time, so I thought it was worth > another reminder. > > Details: > I subclassed ChromeThread which subclasses base::Thread. base::Thread calls > CleanUp on the thread right before termination. CleanUp is virtual. Both > ChromeThread and my class override CleanUp(). base::Thread calls Stop() in > its destructor to stop the thread (if it hasn't already been stopped). But > by the time you hit destruction, the vtable is no longer available and thus > the destructor of base::Thread (and anything it calls) does NOT have access > to the vtable of ChromeThread (or my class). So, if you don't explicitly > call Stop(), your subclass's CleanUp method will NOT be called. Thus the > thread was going away without my CleanUp method ever being called. > Obviously this affects more than just base::Thread. And this is also how > you can hit errors with pure virtual methods being called. > J > > > --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: PSA: Virtual dispatch doesn't work (as you might expect) in destructors!
On Fri, Oct 30, 2009 at 3:26 PM, Jeremy Orlow wrote: > On Fri, Oct 30, 2009 at 3:17 PM, Antoine Labour wrote: > >> On Fri, Oct 30, 2009 at 3:12 PM, Jeremy Orlow wrote: >> >>> I've spent a good deal of this week trying to track down what turned out >>> to be a simple but fairly common problem: I forgot virtual dispatch only >>> partially works in destructors. There have been several email threads about >>> this, but it still bites us form time to time, so I thought it was worth >>> another reminder. >>> >>> >>> Details: >>> I subclassed ChromeThread which subclasses base::Thread. base::Thread >>> calls CleanUp on the thread right before termination. CleanUp is virtual. >>> Both ChromeThread and my class override CleanUp(). base::Thread calls >>> Stop() in its destructor to stop the thread (if it hasn't already been >>> stopped). But by the time you hit destruction, the vtable is no longer >>> available and thus the destructor of base::Thread (and anything it calls) >>> does NOT have access to the vtable of ChromeThread (or my class). So, if >>> you don't explicitly call Stop(), your subclass's CleanUp method will NOT be >>> called. Thus the thread was going away without my CleanUp method ever being >>> called. >>> >>> Obviously this affects more than just base::Thread. And this is also how >>> you can hit errors with pure virtual methods being called. >>> >>> J >>> >> >> Suggestion: don't call CleanUp in the destructor, but check that someone >> did. >> > > I assume you mean Stop()? > Yes, sorry, looking at the code, that's what I meant. > > The problem is when you have a class like ChromeThread inherit from > base::Thread and then call Stop() in its destructor and then have someone > else subclass ChromeThread and expect its CleanUp to be called. > Yup, the pattern is dangerous, so instead of relying on Stop being called from the destructor, it should be explicitly done by the client (and moving Stop() to ~ChromeThread will only move the problem), and the destructor should check that it has been done. Antoine > > > On Fri, Oct 30, 2009 at 3:16 PM, Adam Barth wrote: > > I'm sorry for introducing this pattern in base::Thread. It's bitten >> use several times over the course of the project. If you see a better >> design, please don't hesitate to fix it. >> >> Adam > > > Already filed a bug: http://crbug.com/26365 > --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: PSA: Virtual dispatch doesn't work (as you might expect) in destructors!
On Fri, Oct 30, 2009 at 3:17 PM, Antoine Labour wrote: > On Fri, Oct 30, 2009 at 3:12 PM, Jeremy Orlow wrote: > >> I've spent a good deal of this week trying to track down what turned out >> to be a simple but fairly common problem: I forgot virtual dispatch only >> partially works in destructors. There have been several email threads about >> this, but it still bites us form time to time, so I thought it was worth >> another reminder. >> >> >> Details: >> I subclassed ChromeThread which subclasses base::Thread. base::Thread >> calls CleanUp on the thread right before termination. CleanUp is virtual. >> Both ChromeThread and my class override CleanUp(). base::Thread calls >> Stop() in its destructor to stop the thread (if it hasn't already been >> stopped). But by the time you hit destruction, the vtable is no longer >> available and thus the destructor of base::Thread (and anything it calls) >> does NOT have access to the vtable of ChromeThread (or my class). So, if >> you don't explicitly call Stop(), your subclass's CleanUp method will NOT be >> called. Thus the thread was going away without my CleanUp method ever being >> called. >> >> Obviously this affects more than just base::Thread. And this is also how >> you can hit errors with pure virtual methods being called. >> >> J >> > > Suggestion: don't call CleanUp in the destructor, but check that someone > did. > I assume you mean Stop()? The problem is when you have a class like ChromeThread inherit from base::Thread and then call Stop() in its destructor and then have someone else subclass ChromeThread and expect its CleanUp to be called. On Fri, Oct 30, 2009 at 3:16 PM, Adam Barth wrote: > I'm sorry for introducing this pattern in base::Thread. It's bitten > use several times over the course of the project. If you see a better > design, please don't hesitate to fix it. > > Adam Already filed a bug: http://crbug.com/26365 --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: PSA: Virtual dispatch doesn't work (as you might expect) in destructors!
On Fri, Oct 30, 2009 at 3:12 PM, Jeremy Orlow wrote: > I've spent a good deal of this week trying to track down what turned out to > be a simple but fairly common problem: I forgot virtual dispatch only > partially works in destructors. There have been several email threads about > this, but it still bites us form time to time, so I thought it was worth > another reminder. > > > Details: > I subclassed ChromeThread which subclasses base::Thread. base::Thread > calls CleanUp on the thread right before termination. CleanUp is virtual. > Both ChromeThread and my class override CleanUp(). base::Thread calls > Stop() in its destructor to stop the thread (if it hasn't already been > stopped). But by the time you hit destruction, the vtable is no longer > available and thus the destructor of base::Thread (and anything it calls) > does NOT have access to the vtable of ChromeThread (or my class). So, if > you don't explicitly call Stop(), your subclass's CleanUp method will NOT be > called. Thus the thread was going away without my CleanUp method ever being > called. > > Obviously this affects more than just base::Thread. And this is also how > you can hit errors with pure virtual methods being called. > > J > Suggestion: don't call CleanUp in the destructor, but check that someone did. Antoine --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: PSA: Virtual dispatch doesn't work (as you might expect) in destructors!
I'm sorry for introducing this pattern in base::Thread. It's bitten use several times over the course of the project. If you see a better design, please don't hesitate to fix it. Adam On Fri, Oct 30, 2009 at 3:12 PM, Jeremy Orlow wrote: > I've spent a good deal of this week trying to track down what turned out to > be a simple but fairly common problem: I forgot virtual dispatch only > partially works in destructors. There have been several email threads about > this, but it still bites us form time to time, so I thought it was worth > another reminder. > > Details: > I subclassed ChromeThread which subclasses base::Thread. base::Thread calls > CleanUp on the thread right before termination. CleanUp is virtual. Both > ChromeThread and my class override CleanUp(). base::Thread calls Stop() in > its destructor to stop the thread (if it hasn't already been stopped). But > by the time you hit destruction, the vtable is no longer available and thus > the destructor of base::Thread (and anything it calls) does NOT have access > to the vtable of ChromeThread (or my class). So, if you don't explicitly > call Stop(), your subclass's CleanUp method will NOT be called. Thus the > thread was going away without my CleanUp method ever being called. > Obviously this affects more than just base::Thread. And this is also how > you can hit errors with pure virtual methods being called. > J > > > --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
