Closed Bug 943719 Opened 6 years ago Closed 6 years ago

Build MFBT in unified mode

Categories

(Core :: MFBT, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
Attachment #8339006 - Attachment is obsolete: true
Attachment #8339018 - Flags: review?(nfroyd)
Comment on attachment 8339018 [details] [diff] [review]
Build MFBT in unified mode; r=glandium

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

Your try build looks a little busted.

Please verify that a standalone JS build succeeds (mkdir js-build; cd js-build; $srcdir/js/src/configure --blah && make -jN).

::: mfbt/common.mozbuild
@@ +83,5 @@
> +UNIFIED_SOURCES += ['%s/%s' % (mfbt_root, src) for src in mfbt_sources]
> +
> +# Decimal.cpp doesn't build in unified mode with gcc
> +SOURCES += [
> +    'decimal/Decimal.cpp',

It looks like you lost the |mfbt_root| prefixing by moving this to its separate SOURCES.  Please put it back.
Attachment #8339018 - Flags: review?(nfroyd)
Attachment #8339018 - Attachment is obsolete: true
Comment on attachment 8339349 [details] [diff] [review]
Build MFBT in unified mode; r=glandium

Thanks for the review, especially on the hint about standalone JS builds.  This indeed broke them, but this patch should fix things.
Attachment #8339349 - Flags: review?(nfroyd)
Comment on attachment 8339349 [details] [diff] [review]
Build MFBT in unified mode; r=glandium

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

I have no problem with all the mfbt/ changes.  r=me for those.

I don't understand what all the js/ changes are for...|using mozilla| and |using js| in the same (expanded due to unification) scope?  Please explain and/or r? a JS engine hacker to take a look at those.

We should probably discuss guidelines on toplevel |using| declarations now that unified sources are becoming more widespread.  My turn first: "Don't."  Discuss. :)
Attachment #8339349 - Flags: review?(nfroyd) → review+
JS doesn't have any |using namespace mozilla;| as best I know -- only |using mozilla::Foo;|.  |using namespace js;| is pretty common, tho.  At a glance I don't see the conflict with those, either.
NullPtr exists in both namespace js and namespace JS, so if you have code which says using those two namespaces, then NullPtr will be an ambiguous name.

Vector exists both in namespace js and in namespace mozilla, so if you have code which uses both of those namespaces, Vector will be an ambiguous name (the using mozilla thing is probably coming from mfbt).

The CreateThisInfo and ThrowThisInfo names were defined in two translation units.

TYPE_UINT32 in jsprf.cpp clashes with ScalarTypeRepresentation::TYPE_UINT32 IIRC.

The rest should be self-explanatory.
Comment on attachment 8339349 [details] [diff] [review]
Build MFBT in unified mode; r=glandium

I guess I'll ask for Kannan's blessing on this patch anyways.
Attachment #8339349 - Flags: review?(kvijayan)
Comment on attachment 8339349 [details] [diff] [review]
Build MFBT in unified mode; r=glandium

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

::: js/src/jsprf.cpp
@@ +1207,5 @@
>      }
>      return ss.base;
>  }
>  
> +#undef TYPE_UINT32

It seems incomplete to #undef this, but not all the other similar #define TYPE_XXX and #define FLAG_XXX that are in this code.

For completeness' sake, and to avoid any future issues, it might be good to do this.
Attachment #8339349 - Flags: review?(kvijayan) → review+
Hmm, this is failing JIT tests on try <https://tbpl.mozilla.org/?tree=Try&rev=184d00b6e6ed>, and fixing those will probably require some rocket science that I lack.

Nathan, is there a way to not build this stuff as unified when building the JS shell?
Flags: needinfo?(nfroyd)
I hope I speak for all the SpiderMonkey people when I say that having a unified/non-unified difference between the JS shell and JS in the browser seems really subpar, and at least at first glance I would r- in a heartbeat.  :-)  We should just debug the test failures, unless we can reduce the set of things unified to evade the issue and allow more leisurely debugging.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #13)
> I hope I speak for all the SpiderMonkey people when I say that having a
> unified/non-unified difference between the JS shell and JS in the browser
> seems really subpar, and at least at first glance I would r- in a heartbeat.
> :-)  We should just debug the test failures, unless we can reduce the set of
> things unified to evade the issue and allow more leisurely debugging.

Fair point!

Kannan hinted that VMFunction may have a non-trivial ctor, so moving those static variables can cause problems here.  And looking at the code, he's right.  Let me experiment with putting those static things back into the global scope and renaming them...
I couldn't reproduce the test failures locally (because that would be too easy!) so I pushed to try again: https://tbpl.mozilla.org/?tree=Try&rev=3aee30200562
Flags: needinfo?(nfroyd)
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #12)
> Nathan, is there a way to not build this stuff as unified when building the
> JS shell?

If we haven't ported the --disable-unified-builds (or however you spell it) flag to js's configure, we should probably do that.
(In reply to comment #16)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #12)
> > Nathan, is there a way to not build this stuff as unified when building the
> > JS shell?
> 
> If we haven't ported the --disable-unified-builds (or however you spell it)
> flag to js's configure, we should probably do that.

I think that it should just work, but I may be wrong.  Anyways, Waldo convinced me that I was wrong, and I now have a green patch!
Backed out, I think somebody bitrotted my patch :(

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a2d363a102e
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #19)
> Backed out, I think somebody bitrotted my patch :(

https://hg.mozilla.org/integration/mozilla-inbound/rev/06f844b81f3d#l2.12
https://hg.mozilla.org/mozilla-central/rev/36b6702581d4
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.