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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: text file: the 415 lines of build warning spam

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
}

Comment 1

[Daniel Holbert [:dholbert]]

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

Comment 2

[Daniel Holbert [:dholbert]]

(This was from a build with GCC 4.7 on Ubuntu 13.04, BTW)

Comment 3

[Daniel Holbert [:dholbert]]

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

Comment 4

[Daniel Holbert [:dholbert]]

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

Comment 5

[Jan de Mooij [:jandem]]

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

Comment 6

[Daniel Holbert [:dholbert]]

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

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Patch

Fixes the warnings with GCC 4.7 and no new warnings with Clang 3.2.

Comment 8

[Brian Hackett [Laid off!]]

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.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Patch v2

Moves a lot more code around to avoid the Barrier-inl.h include.

Comment 10

[Brian Hackett [Laid off!]]

Comment on attachment 735201 [details] [diff] [review]
Patch v2

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

Thanks!

Comment 11

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0e0bc4ccff0

(Try: https://tbpl.mozilla.org/?tree=Try&rev=57b0adcab23e)

Comment 12

[Daniel Holbert [:dholbert]]

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

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: followup

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)

Comment 14

[Daniel Holbert [:dholbert]]

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

Comment 15

[Jan de Mooij [:jandem]]

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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/76c52c1ca95a

Comment 17

[Trevor Saunders (:tbsaunde)]

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

Comment 18

[Jan de Mooij [:jandem]]

The PGO slowdown was caused by another patch, relanded:

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

Third time's the charm, right?

Comment 19

[Kannan Vijayan [:djvj]]

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2e667c0efaa5