Last Comment Bug 704127 - Implement MOZ_FINAL
: Implement MOZ_FINAL
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: MFBT (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-21 07:12 PST by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-02-01 13:57 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
1 - Move the MOZ_* modifiers into a new header, mfbt/Attributes.h (12.64 KB, patch)
2011-11-21 07:14 PST, Jeff Walden [:Waldo] (remove +bmo to email)
cjones.bugs: review+
Details | Diff | Splinter Review
2 - Implement MOZ_FINAL (20.11 KB, patch)
2011-11-21 07:18 PST, Jeff Walden [:Waldo] (remove +bmo to email)
cjones.bugs: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-21 07:12:32 PST
In looking at the prodigious warning spew for a Mozilla build under Clang, I noticed a ton for non-virtual-dtor-with-virtual-member-functions when a pointer to such a class is deleted.  As you might hope/expect, marking the class as final kills the warning (no more danger of a static/runtime type mismatch).  So I implemented MOZ_FINAL so we can get rid of many of these warnings.

We already had NS_FINAL_CLASS for this (incompatibly placed), so in theory I could kill that and use MOZ_FINAL everywhere.  But of course it's not that simple.

First, adding MOZ_FINAL to nsCOMPtr_base::nsDerivedSafe causes ~22k undefined references linking libxul.  I smell a compiler/linker bug but have no proof.

Second, existing use of NS_FINAL_CLASS is totally broken.  We have classes marked with it now which get used in nsCOMPtr.  But nsCOMPtr uses the nsDerivedSafe hack to elide AddRef/Release from operator-> and get().  And the nsDerivedSafe hack works by inheriting from T.  Turns out static analysis that doesn't get run when compiling, breaks!  Surprise surprise, puppy surprise!  How many bad puppies are there inside the codebase?  I had to not mark nsFrameSelection, NameSpaceRule, StyleRule, GroupListRule, MediaRule, DocumentRule, nsCSSKeyframeStyleDeclaration, nsCSSKeyframesRule, and nsCSSStyleSheet final to make this compile.  I think that's everything, because with this patch the last NS_FINAL_CLASS hits are in nscore.h, nsCOMPtr.h, or in tests.  I'd prefer to let a static-analysis actually kill it.

Anyway.  I didn't expect to implement this so soon, but surprisingly the need arose.  So let's get 'er done.
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-21 07:14:41 PST
Created attachment 575853 [details] [diff] [review]
1 - Move the MOZ_* modifiers into a new header, mfbt/Attributes.h

A few places that want these macros don't yet work with the full mfbt experience.  Plus there's some conceptual distinction here.  So move MOZ_OVERRIDE and MOZ_DELETE (and next MOZ_FINAL) into mfbt/Attributes.h.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-21 07:18:57 PST
Created attachment 575855 [details] [diff] [review]
2 - Implement MOZ_FINAL

I replaced the NS_FINAL_CLASS modifiers here, and I added MOZ_FINAL to a few classes to silence the Clang warning noted in comment 0 to verify it worked.  As for the NS_FINAL_CLASS misuses, I just removed the attribute entirely.  I thought about it a little, and I can't think of a way to mark the classes as final yet somehow elide Addref/Release from them.  Maybe someone else will have a flash of insight here.
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-21 07:20:29 PST
Er, in comment 0 that should be "let a static-analysis hacker actually kill it".
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-21 10:17:56 PST
I sent mail to the Clang list about whether making nsDerivedSafe final could correctly affect linking behavior (seems reasonable to be sure this is a compiler bug before actually trying to get a usable testcase for fixing):

http://lists.cs.uiuc.edu/pipermail/cfe-dev/2011-November/018779.html
Comment 5 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2011-11-21 11:21:22 PST
Ah, I remember this is why I removed (and hated) nsDerivedSafe! If we have to choose between nsDerivedSafe and correctly marking classes which are in fact final, I'd prefer to remove nsDerivedSafe. Although in this case I suspect that what we could/should do is keep using nsDerivedSafe in nsCOMPtr but not use it in nsRefPtr, and use nsRefPtr with all these concrete classes.
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-21 12:39:00 PST
The Clang people (see responses to that mail) suggest it's a compiler bug, probably a bad optimization.  I'll probably try to get a reduced testcase going soon.
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-21 14:06:23 PST
Well, maybe not.  They were guessing that calls to virtual functions on a final class were being devirtualized, and that was causing the undefined references.  So I tried to string together some files and test that hypothesis, but I have no experience whatsoever at compiling anything other than single-file programs, and stumbling around via web search wasn't working, so I don't know if my tests were disproving that hypothesis or just not testing it properly.  And clearly reducing from libxul is Not Going To Work.  So I'm stuck on that point.
Comment 8 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-22 00:34:25 PST
Comment on attachment 575855 [details] [diff] [review]
2 - Implement MOZ_FINAL


>diff --git a/layout/generic/nsFrameSelection.h b/layout/generic/nsFrameSelection.h

>-class NS_FINAL_CLASS nsFrameSelection : public nsISupports {
>+class nsFrameSelection : public nsISupports {

Can you file a follow-up bug for fixing the previously-NS_FINAL_CLASS
classes?  I'd hate to lose that information if it was previously
meaningful but has been broken since first annotated.  If it's no
longer meaningful, ok.
Comment 9 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-22 00:35:30 PST
(In reply to Jeff Walden (remove +bmo to email) from comment #7)
> Well, maybe not.  They were guessing that calls to virtual functions on a
> final class were being devirtualized, and that was causing the undefined
> references.  So I tried to string together some files and test that
> hypothesis, but I have no experience whatsoever at compiling anything other
> than single-file programs, and stumbling around via web search wasn't
> working, so I don't know if my tests were disproving that hypothesis or just
> not testing it properly.  And clearly reducing from libxul is Not Going To
> Work.  So I'm stuck on that point.

You should be able to generate assembly for one of the callsites that's failing to link and inspect it directly.  |make -C foo/Bar.s| is the incantation, IIRC.
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-22 12:42:55 PST
I filed a bug with MS to disable their virtual-function-but-non-virtual-destructor warning for sealed classes:

https://connect.microsoft.com/VisualStudio/feedback/details/707497/msvc-should-not-emit-the-optional-warning-c4265-virtual-functions-non-virtual-destructor-for-a-class-marked-as-sealed
Comment 13 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-26 13:46:07 PST
Blog post:

http://whereswalden.com/2011/11/26/introducing-moz_final-prevent-inheriting-from-a-class-or-prevent-overriding-a-virtual-function/

And m.d.platform post:

http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/bdc9e7b63b6378bd

I really probably should get to putting docs on MDN at some point...
Comment 14 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-01 18:29:49 PST
Followup to fix a typo and a failure to use |final| in gcc4.7+ -std=c++0x:

http://hg.mozilla.org/integration/mozilla-inbound/rev/d896a4fb99f7
Comment 15 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-12-02 03:25:21 PST
https://hg.mozilla.org/mozilla-central/rev/d896a4fb99f7
Comment 16 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-22 14:18:37 PST
I consider the comments by these macros to be much better documentation than anything maintained externally, so I think this document roughly pointing people to it is reasonable documentation:

https://developer.mozilla.org/en/mfbt

Note that this macro no longer lives in Util.h, rather in Attributes.h now, so that people have a better chance guessing where it's defined in mfbt/ based solely on the functionality desired.

Note You need to log in before you can comment on or make changes to this bug.