Implement MOZ_FINAL

RESOLVED FIXED in mozilla11

Status

()

Core
MFBT
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

({dev-doc-complete})

unspecified
mozilla11
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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.
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.
Assignee: nobody → jwalden+bmo
Attachment #575853 - Flags: review?(jones.chris.g)
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.
Attachment #575855 - Flags: review?(jones.chris.g)
Er, in comment 0 that should be "let a static-analysis hacker actually kill it".
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
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.
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.
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.
Attachment #575853 - Flags: review?(jones.chris.g) → review+
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.
Attachment #575855 - Flags: review?(jones.chris.g) → review+
(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.
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
https://hg.mozilla.org/integration/mozilla-inbound/rev/de0efe828f9b
https://hg.mozilla.org/integration/mozilla-inbound/rev/3edc79afc842

The followup to comment 8 is bug 704687.
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/de0efe828f9b
https://hg.mozilla.org/mozilla-central/rev/3edc79afc842
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
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...
Keywords: dev-doc-needed
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
https://hg.mozilla.org/mozilla-central/rev/d896a4fb99f7
Component: General → MFBT
QA Contact: general → mfbt
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.