Closed
Bug 704127
Opened 13 years ago
Closed 13 years ago
Implement MOZ_FINAL
Categories
(Core :: MFBT, defect)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: Waldo, Assigned: Waldo)
Details
(Keywords: dev-doc-complete)
Attachments
(2 files)
12.64 KB,
patch
|
cjones
:
review+
|
Details | Diff | Splinter Review |
20.11 KB,
patch
|
cjones
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
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)
Assignee | ||
Comment 2•13 years ago
|
||
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)
Assignee | ||
Comment 3•13 years ago
|
||
Er, in comment 0 that should be "let a static-analysis hacker actually kill it".
Assignee | ||
Comment 4•13 years ago
|
||
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•13 years ago
|
||
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.
Assignee | ||
Comment 6•13 years ago
|
||
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.
Assignee | ||
Comment 7•13 years ago
|
||
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.
Updated•13 years ago
|
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.
Assignee | ||
Comment 10•13 years ago
|
||
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
Assignee | ||
Comment 11•13 years ago
|
||
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
Comment 12•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/de0efe828f9b https://hg.mozilla.org/mozilla-central/rev/3edc79afc842
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 13•13 years ago
|
||
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
Assignee | ||
Comment 14•13 years ago
|
||
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
Assignee | ||
Updated•13 years ago
|
Component: General → MFBT
QA Contact: general → mfbt
Assignee | ||
Comment 16•13 years ago
|
||
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.
Description
•