Closed Bug 859446 Opened 7 years ago Closed 7 years ago

Hundreds of lines of build warnings from "used but never defined" in js/*/Baseline* code

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: dholbert, Assigned: jandem)

References

(Blocks 1 open bug, )

Details

Attachments

(3 files, 1 obsolete file)

In a clobber build, I get over 415 lines of warnings from "used but never defined" warnings in Baseline-*.cpp files

Usually this means we (or some header we #include) are invoking an inline function, where the inline function's definition lives in a separate header that we're neglecting to include.

I'm attaching a text file with a dump of the warnings. Here's a sample of them:
{
In file included from /mozilla-central/js/src/vm/SPSProfiler.h:18:0,
                 from /mozilla-central/js/src/jscntxt.h:39,
                 from /mozilla-central/js/src/ion/BaselineIC.h:11,
                 from /mozilla-central/js/src/ion/BaselineInspector.cpp:8:
/mozilla-central/js/src/jsscript.h:663:24: warning: inline function ‘const char* JSScript::filename() const’ used but never defined [enabled by default]
In file included from /mozilla-central/js/src/jsatom.h:22:0,
                 from /mozilla-central/js/src/jscntxt.h:23,
                 from /mozilla-central/js/src/ion/BaselineIC.h:11,
                 from /mozilla-central/js/src/ion/BaselineInspector.cpp:8:
/mozilla-central/js/src/gc/Barrier.h:423:21: warning: inline function ‘js::HeapValue::HeapValue(const JS::Value&)’ used but never defined [enabled by default]
/mozilla-central/js/src/gc/Barrier.h:425:12: warning: inline function ‘js::HeapValue::~HeapValue()’ used but never defined [enabled by default]
In file included from /mozilla-central/js/src/vm/Shape.h:14:0,
                 from /mozilla-central/js/src/jsscript.h:21,
                 from /mozilla-central/js/src/vm/SPSProfiler.h:18,
                 from /mozilla-central/js/src/jscntxt.h:39,
                 from /mozilla-central/js/src/ion/BaselineIC.h:11,
                 from /mozilla-central/js/src/ion/BaselineInspector.cpp:8:
/mozilla-central/js/src/jsobj.h:433:35: warning: inline function ‘js::types::TypeObject* JSObject::getType(JSContext*)’ used but never defined [enabled by default]
In file included from /mozilla-central/js/src/ion/BaselineIC.h:11:0,
                 from /mozilla-central/js/src/ion/BaselineInspector.cpp:8:
/mozilla-central/js/src/jscntxt.h:1523:22: warning: inline function ‘JS::Zone* JSContext::zone() const’ used but never defined [enabled by default]
In file included from /mozilla-central/js/src/vm/SPSProfiler.h:18:0,
                 from /mozilla-central/js/src/jscntxt.h:39,
                 from /mozilla-central/js/src/ion/BaselineIC.h:11,
                 from /mozilla-central/js/src/ion/BaselineInspector.cpp:8:
/mozilla-central/js/src/jsscript.h:967:24: warning: inline function ‘static void JSScript::writeBarrierPost(js::RawScript, void*)’ used but never defined [enabled by default]
In file included from /mozilla-central/js/src/jsobj.h:27:0,
                 from /mozilla-central/js/src/vm/Shape.h:14,
                 from /mozilla-central/js/src/jsscript.h:21,
                 from /mozilla-central/js/src/vm/SPSProfiler.h:18,
                 from /mozilla-central/js/src/jscntxt.h:39,
                 from /mozilla-central/js/src/ion/BaselineIC.h:11,
                 from /mozilla-central/js/src/ion/BaselineInspector.cpp:8:
/mozilla-central/js/src/vm/ObjectImpl.h:1414:24: warning: inline function ‘static void js::ObjectImpl::writeBarrierPost(js::ObjectImpl*, void*)’ used but never defined [enabled by default]
In file included from /mozilla-central/js/src/jsscript.h:16:0,
                 from /mozilla-central/js/src/vm/SPSProfiler.h:18,
                 from /mozilla-central/js/src/jscntxt.h:39,
                 from /mozilla-central/js/src/ion/BaselineIC.h:11,
                 from /mozilla-central/js/src/ion/BaselineInspector.cpp:8:
/mozilla-central/js/src/jsinfer.h:1089:24: warning: inline function ‘static void js::types::TypeObject::writeBarrierPost(js::types::TypeObject*, void*)’ used but never defined [enabled by default]
In file included from /mozilla-central/js/src/jsscript.h:21:0,
                 from /mozilla-central/js/src/vm/SPSProfiler.h:18,
                 from /mozilla-central/js/src/jscntxt.h:39,
                 from /mozilla-central/js/src/ion/BaselineIC.h:11,
                 from /mozilla-central/js/src/ion/BaselineInspector.cpp:8:
/mozilla-central/js/src/vm/Shape.h:830:24: warning: inline function ‘static void js::Shape::writeBarrierPost(js::RawShape, void*)’ used but never defined [enabled by default]
}

The .cpp files that are ultimately triggering the warnings (in the attached .txt file) are:
{
mozilla-central/js/src/ion/BaselineInspector.cpp
mozilla-central/js/src/ion/shared/BaselineCompiler-shared.cpp
mozilla-central/js/src/ion/shared/BaselineCompiler-x86-shared.cpp
mozilla-central/js/src/ion/shared/BaselineIC-x86-shared.cpp
mozilla-central/js/src/ion/x64/BaselineCompiler-x64.cpp
mozilla-central/js/src/ion/x64/BaselineIC-x64.cpp
}
I'm pretty sure these warnings were introduced in the last few weeks, BTW. (presumably related to some code changes involved with this recent announcement: https://blog.mozilla.org/javascript/2013/04/05/the-baseline-compiler-has-landed/ )
(This was from a build with GCC 4.7 on Ubuntu 13.04, BTW)
Summary: Hundreds of lines of warnings from "used but never defined" in js/*/Baseline* code → Hundreds of lines of build warnings from "used but never defined" in js/*/Baseline* code
(It also looks like these "used but not defined" warnings are visible on TBPL build logs, too, so it's not a quirk of my system or anything:
 https://tbpl.mozilla.org/php/getParsedLog.php?id=21529075&tree=Mozilla-Central
Note that this TBPL log doesn't include the #include-backtrace-to-the-.cpp-file -- it just prints the warning itself -- and it appears to print the warning a bit late, a few lines after the compilation line .cpp file that triggered it, possibly due to parallel compilation.)
Unsurprisingly, this warning-spam started on m-c with the cset "Merge baseline compiler branch and mozilla-central":
 https://hg.mozilla.org/mozilla-central/rev/79542849f3f3

The m-c parent of that cset had zero instances of "used but never defined" warnings, in JS code at least:
 https://hg.mozilla.org/mozilla-central/rev/3a5929ebc886

So this was introduced by the baseline compiler merge (and it presumably was an undiscovered issue on the baseline compiler branch for some time).
Hm, bug 855264 fixed all MSVC/GCC 4.7 warnings when compiling the shell, maybe this is browser-only though (seems likely considering no SpiderMonkey devs filed this..)
I see these warnings when compiling the shell, too, with the following commands:
> cd js/src
> autoconf2.13
> mkdir build-debug
> cd build-debug
> ../configure  --enable-debug --disable-optimize
> make -s -j8
Attached patch Patch (obsolete) — Splinter Review
Fixes the warnings with GCC 4.7 and no new warnings with Clang 3.2.
Assignee: general → jdemooij
Status: NEW → ASSIGNED
Attachment #735129 - Flags: review?(bhackett1024)
Comment on attachment 735129 [details] [diff] [review]
Patch

Review of attachment 735129 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/BaselineIC.h
@@ +16,5 @@
>  #include "BaselineRegisters.h"
>  
>  #include "gc/Heap.h"
>  
> +#include "gc/Barrier-inl.h"

Which functions depend on this #include?  Could they be moved to a -inl.h inline header?  Generally this sort of include is good to avoid.
Attachment #735129 - Flags: review?(bhackett1024) → review+
Attached patch Patch v2Splinter Review
Moves a lot more code around to avoid the Barrier-inl.h include.
Attachment #735129 - Attachment is obsolete: true
Attachment #735201 - Flags: review?(bhackett1024)
Comment on attachment 735201 [details] [diff] [review]
Patch v2

Review of attachment 735201 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #735201 - Flags: review?(bhackett1024) → review+
Comment on attachment 735201 [details] [diff] [review]
Patch v2

>   protected:
[...]
>+    ~ICGetElemNativeStub();


> class ICGetIntrinsic_Constant : public ICStub
> {
>-    ICGetIntrinsic_Constant(IonCode *stubCode, HandleValue value)
>-      : ICStub(GetIntrinsic_Constant, stubCode),
>-        value_(value)
>-    {}
>+    ICGetIntrinsic_Constant(IonCode *stubCode, HandleValue value);
>+    ~ICGetIntrinsic_Constant();

This patch added these destructor decls, but didn't add impls for them.

I think that's why we're seeing build failures like:
> BaselineIC.obj : error LNK2001: unresolved external symbol "protected: __thiscall js::ion::ICGetElemNativeStub::~ICGetElemNativeStub(void)" (??1ICGetElemNativeStub@ion@js@@IAE@XZ)
https://tbpl.mozilla.org/php/getParsedLog.php?id=21665343&full=1&branch=mozilla-inbound


I verified locally (in an up-to-date m-i tree) that the destructors aren't defined anywhere:
> $ grep -r  "~ICGet" js
> js/src/ion/BaselineIC.h:    ~ICGetElemNativeStub();
> js/src/ion/BaselineIC.h:    ~ICGetIntrinsic_Constant();
> $

I think we want to just add {} after the destructors, to declare them as empty, yes?

Since there are several things busting the tree at the moment, I may just push that in the next hour or so to get it to an incrementally-better state, and get an after-the-fact review. (since I don't see either jandem and bhackett on IRC at the moment)

If either of you happen to see this bugmail soon and feel like chiming in on "that's totally correct" vs. "OMG DON'T DO THAT", please do, though. :)
Attached patch followupSplinter Review
Actually, we have to add the destructor definitions in the .cpp file -- not the .h file -- or else we're implicitly invoking all of the member-var destructors from within the .h file, and that triggers some build warnings for used-but-not-defined stuff.

So, this patch just adds the destructors to the .cpp file, below their respective constructors.  (using the local style of "{ }" for an empty function-body)
Attachment #736074 - Flags: review?(terrence)
Attachment #736074 - Flags: review?(terrence)
Attachment #736074 - Flags: review+
Attachment #736074 - Flags: feedback?(jdemooij)
d'oh, looks like this was just backed out for causing the bustage that my attached followup was (hopefully) *just* about to fix. :)

The backout cset was: https://hg.mozilla.org/integration/mozilla-inbound/rev/4705e2c47b2b

so I won't bother landing the followup after all -- jandem, if you think it's sane, feel free to just merge my followup into your original patch and re-land.

(Technically it might be good to run it through Try, to be sure there isn't any other Win 7 PGO bustage -- pgo builds take a little extra work and a good chunk of wait-time on Try: https://wiki.mozilla.org/ReleaseEngineering/TryChooser#What_if_I_want_PGO_for_my_build .  I'll leave it up to you to make the call of whether it's worth a try run or whether we can just call it likely-good.)
Attachment #736074 - Flags: feedback?(jdemooij) → feedback+
Thanks dholbert and terrence! I have no idea why only Windows PGO builds need these definitions. Anyway, pushed with that:

https://hg.mozilla.org/integration/mozilla-inbound/rev/76c52c1ca95a

I triggered a PGO run and it's been running for 75 minutes now, the other builds failed after 15-30 minutes (while linking the JS shell) so it seems to work now.
https://hg.mozilla.org/mozilla-central/rev/76c52c1ca95a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
sorry, but this got backed out because it looks like it regressed talos on windows pgo https://hg.mozilla.org/integration/mozilla-inbound/rev/94ad8254eb26
The PGO slowdown was caused by another patch, relanded:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e667c0efaa5

Third time's the charm, right?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Jan de Mooij [:jandem] from comment #18)
> The PGO slowdown was caused by another patch, relanded:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/2e667c0efaa5
> 
> Third time's the charm, right?

Thanks for merging that ListBaseGetter stub constructor in with the new push :)  Much appreciated.
https://hg.mozilla.org/mozilla-central/rev/2e667c0efaa5
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.