Closed
Bug 1282795
Opened 8 years ago
Closed 8 years ago
Various new-enough clang warning fixes for the JS shell build
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: Waldo, Assigned: Waldo)
References
Details
Attachments
(12 files)
1.93 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
1.13 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
1.28 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
1.19 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
1.13 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
2.13 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
6.53 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
994 bytes,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
2.05 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
1.28 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
1.28 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
22.88 KB,
patch
|
jimb
:
review+
|
Details | Diff | Splinter Review |
I stop building with clang for awhile, and/or I don't update my clang build recently enough, and when I come back the JS engine loses its mind.
Assignee | ||
Comment 1•8 years ago
|
||
This appears to be because people are calling TracerConcrete<T>::typeName() in the template class, which returns TracerConcrete<T>::concreteTypeName, before the latter variable is declared. Add declarations to UbiNode.h for such users so that the declaration is visible whenever those typeName() calls occur.
Attachment #8765925 -
Flags: review?(nfroyd)
Assignee | ||
Comment 2•8 years ago
|
||
Looks like we have a bunch of Maybe<Foo>s around, where Foo has virtual methods but not a virtual destructor. That triggers clang warnings. The simplest fix is to mark the classes final so that clang knows ~Foo won't miss destroying any derived-class bits. A few more of these to follow, split up to verify each of the warnings got fixed.
Attachment #8765928 -
Flags: review?(nfroyd)
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8765929 -
Flags: review?(nfroyd)
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8765930 -
Flags: review?(nfroyd)
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8765931 -
Flags: review?(nfroyd)
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8765932 -
Flags: review?(nfroyd)
Assignee | ||
Comment 7•8 years ago
|
||
https://www.securecoding.cert.org/confluence/display/cplusplus/EXP58-CPP.+Pass+an+object+of+the+correct+type+to+va_start That may or may not have clarified anything. Long and short of it is that because C++11 [conv.prom]p3 lets us promote the enum to int, the enum can't be the type of the last argument before the ellipsis. Reordering arguments to put a pointer last, solves things. (Really we shouldn't use varargs at all here, but that's a bigger change and not for now.)
Attachment #8765934 -
Flags: review?(nfroyd)
Assignee | ||
Comment 8•8 years ago
|
||
Attachment #8765935 -
Flags: review?(nfroyd)
Assignee | ||
Comment 9•8 years ago
|
||
The warning makes sense enough -- warn about side-effecty things that are used in unevaluated contexts -- but because MOZ_ALWAYS_* *does* evaluate its argument, and the unevaluated context in play here is just checking expression-type, we're in the clear here. Fixing the underlying macros is the right move, versus a per-user fix. I was also thinking maybe just _Pragma some stuff, but it seems easier just to reuse our existing primitives, than to open up that can of worms.
Attachment #8765937 -
Flags: review?(nfroyd)
Assignee | ||
Comment 10•8 years ago
|
||
Oh stab, the s/0/false/ there probably fails because that header is supposed to be C and C++ both. Pretend I undid that. How long til C dies in our codebase, srsly.
Assignee | ||
Comment 11•8 years ago
|
||
Attachment #8765939 -
Flags: review?(nfroyd)
Assignee | ||
Comment 12•8 years ago
|
||
Patchbombing complete. Some of this I'd just land r=themaid usually, but the unevaluated context and ellipsis goop is goopy enough someone competent should skim as well.
Attachment #8765940 -
Flags: review?(nfroyd)
Comment 13•8 years ago
|
||
Comment on attachment 8765925 [details] [diff] [review] Fix a bunch of UbiNode.h forward declaration warnings Review of attachment 8765925 [details] [diff] [review]: ----------------------------------------------------------------- It would be splendid if some of the explanations you provided for these patches appeared as commit messages for these patches.
Attachment #8765925 -
Flags: review?(nfroyd) → review+
Updated•8 years ago
|
Attachment #8765928 -
Flags: review?(nfroyd) → review+
Updated•8 years ago
|
Attachment #8765929 -
Flags: review?(nfroyd) → review+
Updated•8 years ago
|
Attachment #8765930 -
Flags: review?(nfroyd) → review+
Updated•8 years ago
|
Attachment #8765931 -
Flags: review?(nfroyd) → review+
Updated•8 years ago
|
Attachment #8765932 -
Flags: review?(nfroyd) → review+
Updated•8 years ago
|
Attachment #8765935 -
Flags: review?(nfroyd) → review+
Comment 14•8 years ago
|
||
Comment on attachment 8765934 [details] [diff] [review] Don't make the last non-ellipsis argument to ExpandErrorArgumentsVA an enumeration that would be subject to integral promotion, because this would invoke undefined behavior Review of attachment 8765934 [details] [diff] [review]: ----------------------------------------------------------------- Just...wow.
Attachment #8765934 -
Flags: review?(nfroyd) → review+
Comment 15•8 years ago
|
||
Comment on attachment 8765937 [details] [diff] [review] Silence warnings when MOZ_ALWAYS_{TRUE,FALSE} are passed expressions with embedded side effects, that would ordinarily trigger side effects but don't inside certain unevaluated contexts within MOZ_ASSERT, which then triggers compiler warnings with new-eno Review of attachment 8765937 [details] [diff] [review]: ----------------------------------------------------------------- Ew...but necessary.
Attachment #8765937 -
Flags: review?(nfroyd) → review+
Updated•8 years ago
|
Attachment #8765939 -
Flags: review?(nfroyd) → review+
Updated•8 years ago
|
Attachment #8765940 -
Flags: review?(nfroyd) → review+
Comment 16•8 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #0) > I stop building with clang for awhile, and/or I don't update my clang build > recently enough, and when I come back the JS engine loses its mind. Just curious, which Clang version? Clang 3.8.0 is warning free here.
Comment 17•8 years ago
|
||
Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/mozilla-inbound/rev/763642feedb0 Forward-declare a bunch of TracerConcrete<T>::concreteTypeName[] so that the default virtual TracerConcrete<T>::typeName() can refer to those declarations before their definitions, without triggering compiler warnings up the wazoo. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/2733f167b8cd Make CodeGenerator final to silence some -Wdelete-non-virtual-dtor warnings caused by Maybe<CodeGenerator>. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/3decbf5a262f Make JS::CompileOptions final to silence some -Wdelete-non-virtual-dtor warnings caused by Maybe<JS::CompileOptions>. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/94cb0c8e7feb Make frontend::Parser final to silence some -Wdelete-non-virtual-dtor warnings caused by Maybe<Parser>. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/0c2f1c16b763 Make VerifyPreTracer final to silence some -Wdelete-non-virtual-dtor warnings caused by Maybe<VerifyPreTracer>. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/1aeffbc50a98 Make NativeRegExpMacroAssembler and InterpretedRegExpMacroAssembler final to silence some -Wdelete-non-virtual-dtor warnings caused by Maybe<those types>. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/9c4675f6970f Don't make the last non-ellipsis argument to ExpandErrorArgumentsVA an enumeration that would be subject to integral promotion, because this would invoke undefined behavior. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/ddd5a4c1edc8 Make InvokeState final to silence some -Wdelete-non-virtual-dtor warnings caused by Maybe<InvokeState>. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/cd248c790c8c Silence warnings when MOZ_ALWAYS_{TRUE,FALSE} are passed expressions with embedded side effects, that would ordinarily trigger side effects but don't inside certain unevaluated contexts within MOZ_ASSERT, which then triggers compiler warnings with new-enough clang. You are not expected to understand this. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/6fef5909e617 Make a variable DebugOnly, and extract a side effect on that variable from an assertion expression to avoid a clang compiler warning when a side-effectful expression appears in unevaluated context. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/00aadbafd114 Extract a side effect on a variable from an assertion expression to avoid a clang compiler warning when a side-effectful expression occurs in an unevaluated context. r=froydnj
Assignee | ||
Comment 18•8 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #16) > Just curious, which Clang version? Clang 3.8.0 is warning free here. [jwalden@find-waldo-now clean]$ clang-tip --version clang version 3.9.0 (trunk 272446) Target: x86_64-unknown-linux-gnu Thread model: posix InstalledDir: /home/jwalden/.programs/new-compilers This is basically svn tip from maybe three weeks ago.
Assignee | ||
Comment 19•8 years ago
|
||
Backed out the UbiNode.h warning fix -- Windows doesn't like a const declared without being defined (unless it's extern) and compile-errors. Because static consts inside of classes have extern linkage already if the enclosing class has linkage (that is, the class isn't in an anonymous namespace), I claim this is an MSVC compiler bug. Now as to what to do. Um. This is what the clang warning in full looks like (and similar for the various other declarations I'd added): /home/jwalden/moz/clean/js/src/dbg/dist/include/js/UbiNode.h:1014:56: warning: instantiation of variable 'JS::ubi::TracerConcrete<js::LazyScript>::concreteTypeName' required here, but no definition is available [-Wundefined-var-template] const char16_t* typeName() const override { return concreteTypeName; } ^ /home/jwalden/moz/clean/js/src/dbg/dist/include/js/UbiNode.h:1019:14: note: in instantiation of member function 'JS::ubi::TracerConcrete<js::LazyScript>::typeName' requested here explicit TracerConcrete(Referent* ptr) : Base(ptr) { } ^ /home/jwalden/moz/clean/js/src/jsscript.h:2560:46: note: in instantiation of member function 'JS::ubi::TracerConcrete<js::LazyScript>::TracerConcrete' requested here explicit Concrete(js::LazyScript *ptr) : TracerConcrete<js::LazyScript>(ptr) { } ^ /home/jwalden/moz/clean/js/src/dbg/dist/include/js/UbiNode.h:1023:27: note: forward declaration of template entity is here static const char16_t concreteTypeName[]; ^ /home/jwalden/moz/clean/js/src/dbg/dist/include/js/UbiNode.h:1014:56: note: add an explicit instantiation declaration to suppress this warning if 'JS::ubi::TracerConcrete<js::LazyScript>::concreteTypeName' is explicitly instantiated in another translation unit const char16_t* typeName() const override { return concreteTypeName; } ^ My patch here *did* "add an explicit instantiation declaration to suppress this warning". So I'm not sure what else can be done here, that MSVC will like. This particular pattern being performed here is IMO fairly dodgy. The simplest solution, to me, would seem to be to move |TracerConcrete<T>::concreteTypeName| out of a template class -- just put it as a member of the various T (all of which do seem to be class types). I'll experiment along these lines, because of all the warnings the patchwork here fixed, the UbiNode ones were *by far* the most spewiest, loudest, least tolerable, &c.
Comment 20•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2733f167b8cd https://hg.mozilla.org/mozilla-central/rev/3decbf5a262f https://hg.mozilla.org/mozilla-central/rev/94cb0c8e7feb https://hg.mozilla.org/mozilla-central/rev/0c2f1c16b763 https://hg.mozilla.org/mozilla-central/rev/1aeffbc50a98 https://hg.mozilla.org/mozilla-central/rev/9c4675f6970f https://hg.mozilla.org/mozilla-central/rev/ddd5a4c1edc8 https://hg.mozilla.org/mozilla-central/rev/cd248c790c8c https://hg.mozilla.org/mozilla-central/rev/6fef5909e617 https://hg.mozilla.org/mozilla-central/rev/00aadbafd114
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Assignee | ||
Updated•8 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 21•8 years ago
|
||
(The s/struct/class/g is because clangen compiling some of this was hitting -Wmismatched-tag for struct-class disagreements, so I figured while I was in the area I'd standardize things.)
Attachment #8768233 -
Flags: review?(jimb)
Comment 22•8 years ago
|
||
Comment on attachment 8768233 [details] [diff] [review] Declare JS::ubi::Concrete<T>::concreteTypeName within every JS::ubi::Concrete specialization to avoid use-before-declaration warnings related to templates Review of attachment 8768233 [details] [diff] [review]: ----------------------------------------------------------------- This all looks good to me; I assume all our compilers think it's okay too. Thanks for making the ordering of the members in the Concrete specializations more consistent. ::: js/public/UbiNode.h @@ +664,2 @@ > template<typename Referent> > +class Concrete; Why is it better for the unspecialized Concrete to have no members? (Question, not objection.) At least when it's in the code instead of a comment, its syntax is checked.
Attachment #8768233 -
Flags: review?(jimb) → review+
Assignee | ||
Comment 23•8 years ago
|
||
(In reply to Jim Blandy :jimb from comment #22) > ::: js/public/UbiNode.h > @@ +664,2 @@ > > template<typename Referent> > > +class Concrete; > > Why is it better for the unspecialized Concrete to have no members? > (Question, not objection.) At least when it's in the code instead of a > comment, its syntax is checked. This isn't "no members" -- that would be |template<typename Referent> class Concrete {};|. This is *declaring* the class without *defining* it. In the former case, Thus any use of Concrete<T> that requires Concrete<T> be complete (sizeof, as a base class, referring to a member of it, etc.), where Concrete<T> doesn't refer to a deliberate specialization, is an immediate compile error. Doing it the other way, of having the template class have a definition, means that accidental reference to the template class (because no specialization was provided) would compile. Eventually, it wouldn't link -- when that Concrete<T>::concreteTypeName or Concrete<T>::construct wasn't defined. But that's awhile off in the compilation pipeline. Much better to error immediately when the first reference occurs.
Comment 24•8 years ago
|
||
Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/mozilla-inbound/rev/2620e5ba1067 Declare JS::ubi::Concrete<T>::concreteTypeName within every JS::ubi::Concrete specialization to avoid use-before-declaration warnings related to templates. r=jimb
Comment 25•8 years ago
|
||
Backed out for 'Concrete' redeclaration in HeapSnapshot.h (static failure): https://hg.mozilla.org/integration/mozilla-inbound/rev/015a827edf56 Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=2620e5ba1067b251c31fc29ef6f507db97ac3ffb Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=31368654&repo=mozilla-inbound 10:14:12 INFO - /builds/slave/m-in-l64-st-an-d-0000000000000/build/src/obj-firefox/dist/include/mozilla/devtools/HeapSnapshot.h:49:10: error: struct 'Concrete' was previously declared as a class [-Werror,-Wmismatched-tags] 10:14:12 INFO - friend struct JS::ubi::Concrete<JS::ubi::DeserializedNode>; 10:14:12 INFO - ^ 10:14:12 INFO - /builds/slave/m-in-l64-st-an-d-0000000000000/build/src/obj-firefox/dist/include/mozilla/devtools/DeserializedNode.h:247:7: note: previous use is here 10:14:12 INFO - class Concrete<DeserializedNode> : public Base 10:14:12 INFO - ^ 10:14:12 INFO - /builds/slave/m-in-l64-st-an-d-0000000000000/build/src/obj-firefox/dist/include/mozilla/devtools/HeapSnapshot.h:49:10: note: did you mean class here? 10:14:12 INFO - friend struct JS::ubi::Concrete<JS::ubi::DeserializedNode>; 10:14:12 INFO - ^~~~~~ 10:14:12 INFO - class 10:14:13 INFO - 1 error generated. 10:14:13 INFO - gmake[5]: *** [DeserializedNode.o] Error 1
Flags: needinfo?(jwalden+bmo)
Comment 26•8 years ago
|
||
Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/mozilla-inbound/rev/6c37be9cee51 Declare JS::ubi::Concrete<T>::concreteTypeName within every JS::ubi::Concrete specialization to avoid use-before-declaration warnings related to templates. r=jimb
Assignee | ||
Comment 27•8 years ago
|
||
Fixed the missed class/struct mismatch, also added some template function instantiations to placate builds on certain platforms -- another class of failure picked up in the backed-out push. Try came back green for that: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b392bc23489c so relanded with those changes.
Flags: needinfo?(jwalden+bmo)
Comment 28•8 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #23) > This isn't "no members" -- that would be |template<typename Referent> class > Concrete {};|. This is *declaring* the class without *defining* it. In the > former case, Thus any use of Concrete<T> that requires Concrete<T> be > complete (sizeof, as a base class, referring to a member of it, etc.), where > Concrete<T> doesn't refer to a deliberate specialization, is an immediate > compile error. OOOOOH. That's very nice. I will use this in the future.
Comment 29•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6c37be9cee51
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•