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

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(12 attachments)

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
Assignee

Description

3 years ago
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

3 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

3 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 7

3 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 9

3 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

3 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 12

3 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 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+
Attachment #8765928 - Flags: review?(nfroyd) → review+
Attachment #8765929 - Flags: review?(nfroyd) → review+
Attachment #8765930 - Flags: review?(nfroyd) → review+
Attachment #8765931 - Flags: review?(nfroyd) → review+
Attachment #8765932 - Flags: review?(nfroyd) → review+
Attachment #8765935 - Flags: review?(nfroyd) → review+
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 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+
Attachment #8765939 - Flags: review?(nfroyd) → review+
Attachment #8765940 - Flags: review?(nfroyd) → review+
(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

3 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

3 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

3 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.
Assignee

Updated

3 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 21

3 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

3 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

3 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

3 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
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

3 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

3 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

3 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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6c37be9cee51
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Depends on: 1285652
You need to log in before you can comment on or make changes to this bug.