1282795 - Various new-enough clang warning fixes for the JS shell build

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

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.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Fix a bunch of UbiNode.h forward declaration warnings

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.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Make CodeGenerator final to silence some -Wdelete-non-virtual-dtor warnings

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.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Make JS::CompileOptions final to silence some -Wdelete-non-virtual-dtor warnings

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Make frontend::Parser final to silence some -Wdelete-non-virtual-dtor warnings

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Make VerifyPreTracer final to silence some -Wdelete-non-virtual-dtor warnings

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Make NativeRegExpMacroAssembler and InterpretedRegExpMacroAssembler final to silence some -Wdelete-non-virtual-dtor warnings

Comment 7

[Jeff Walden [:Waldo]]

Attachment: 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

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.)

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Make InvokeState final to silence some -Wdelete-non-virtual-dtor warnings

Comment 9

[Jeff Walden [:Waldo]]

Attachment: 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

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.

Comment 10

[Jeff Walden [:Waldo]]

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.

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Make a variable DebugOnly, and extract a side effect on that variable from an assertion expression to avoid a clang compiler warning

Comment 12

[Jeff Walden [:Waldo]]

Attachment: Extract a side effect on a variable from an assertion expression to avoid a clang compiler warning

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.

Comment 13

[Nathan Froyd [:froydnj]]

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.

Comment 14

[Nathan Froyd [:froydnj]]

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.

Comment 15

[Nathan Froyd [:froydnj]]

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.

Comment 16

[Jan de Mooij [:jandem]]

(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

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Jeff Walden [:Waldo]]

(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.

Comment 19

[Jeff Walden [:Waldo]]

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

[Carsten Book [:Tomcat]]

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

Comment 21

[Jeff Walden [:Waldo]]

Attachment: Declare JS::ubi::Concrete<T>::concreteTypeName within every JS::ubi::Concrete specialization to avoid use-before-declaration warnings related to templates

(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.)

Comment 22

[Jim Blandy :jimb]

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.

Comment 23

[Jeff Walden [:Waldo]]

(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

[Pulsebot]

Pushed by [e-mail]:
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

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 26

[Pulsebot]

Pushed by [e-mail]:
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

Comment 27

[Jeff Walden [:Waldo]]

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.

Comment 28

[Jim Blandy :jimb]

(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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6c37be9cee51