1346389 - Make --enable-shared-js link again

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

'tis the season: one tries to build it, and it fails because of the things that got added in the meantime.

Caveat: I only did this for an opt build on Mac.  Debug or other platforms may have other failures.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Make --enable-shared-js link again, at least for an opt mac build with intl api disabled

MozReview-Commit-ID: 264vsCvhh9Z

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8846109 [details] [diff] [review]
Make --enable-shared-js link again, at least for an opt mac build with intl api disabled

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

How do you test this? I tried building just the JS shell with --enable-shared-js, but it still statically linked. I built the whole browser with --enable-shared-js --enable-export-js, but it choked on ICU. (Does the whole browser build without ICU? I tried that and it complained about something else.)

I tried hacking the JS link line to link against the shared lib instead of the static lib, but that gave me 500 or so lines of error messages about other things that needed to be exported too.

But anyway... if this works for you, I'm fine with you landing it.

::: js/public/CallArgs.h
@@ +301,5 @@
>      /*
>       * Returns true if there are at least |required| arguments passed in. If
>       * false, it reports an error message on the context.
>       */
> +    JS_PUBLIC_API(bool) requireAtLeast(JSContext* cx, const char* fnname, unsigned required) const;

Why just this method? It seems like the whole class should be JS_PUBLIC_API instead. (But I haven't figured out how to test that locally.)

Comment 3

[Boris Zbarsky [:bzbarsky]]

> How do you test this? 

I build Firefox with --without-intl-api (to work around the intl data link problems) and --enable-shared-js --enable-export-js.  I also had to apply the hackypatch from bug 1346341 (real patch coming up) and an equivalent of the patch in bug 1346098 to make --without-intl-api link.

> Why just this method?

It's the only non-inline bit.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1978f7d49c4
Make --enable-shared-js link again, at least for an opt mac build with intl api disabled.  r=sfink

Comment 5

[Boris Zbarsky [:bzbarsky]]

Backed out for build failures on Linux and maybe windows....  Looks like different compilers treat forward-declarations around this stuff differently.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So current status:  The Windows bits are fixed.

The Linux failures are more exciting.  They're coming from this bit in Barrier.cpp:

  template struct JS_PUBLIC_API(MovableCellHasher<JSObject*>);

I also have this bit in RootingAPI.h:

  template <typename T>
  struct JS_PUBLIC_API(MovableCellHasher)

but that on its own is not enough to make the hasHash/ensureHash/hah/match/rekey bits link on Mac, hence the bits in Barrier.cpp.

Unfortunately, the Barrier.cpp thing runs into a gcc bug, <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=50044> which causes gcc to emit a warning, which fails the build due to -Werror=attributes.

So I guess I have a few options.  I could try to continu look for ways to trick the compiler into correctly exporting the hash/etc bits (e.g. via explicit template instantiations of them that are JS_PUBLIC_API, and removing JS_PUBLIC_API from the class?  I'm not sure what would work here).  I could try to do something, I'm not sure what, to avoid that buggy gcc warning.  Or I could try to make that warning not fail the build.

Waldo, any opinions?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6a24c5176c3787dc8e348ef8f35db9172af576c&selectedJob=83226529 btw.

Comment 8

[Jeff Walden [:Waldo]]

Compiler-targeted #ifdefs?

The general approach for warnings on attributes on declarations that aren't definitions, is to move the attributes onto the definition.  But if that doesn't work for template stuff on explicit instantiations of the class, I think you're mostly out of luck.  Preprocessor tricks seem about it.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/16f71512d587
Make --enable-shared-js link again, at least for an opt mac build with intl api disabled.  r=sfink, a=waldo on the gcc-specific bits.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/16f71512d587

Comment 11

[Steve Fink [:sfink] [:s:]]

For the record, bz wants --enable-shared-js so that he can profile in Instruments and separate out the time spent in Spidermonkey from time spent elsewhere, and that's easy to do if it's in a separate shared library.

bz can correct me if I'm wrong, but I believe that if there was a good alternative way of bucketing this stuff, then he would no longer need --enable-shared-js.

Comment 12

[Boris Zbarsky [:bzbarsky]]

For my purposes, that's absolutely correct.

Comment 13

[Philip Chimento [:ptomato]]

Attachment: Backport symbol visibility changes to esr52

Comment 14

[Philip Chimento [:ptomato]]

See bug 1334338; the patch here fixes that bug. I'd like to request it for backport to ESR52, but the original patch didn't apply cleanly, so here it is reapplied to ESR52.

Comment 15

[Philip Chimento [:ptomato]]

Also, could someone with permissions please make this bug block "sm.embedding"?

Comment 16

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8885096 [details] [diff] [review]
Backport symbol visibility changes to esr52

Approval Request Comment
[Feature/Bug causing the regression]: various, over time
[User impact if declined]: some spidermonkey embedders have a harder time, and in theory some profiling difficulties using esr52
[Is this code covered by automated tests?]: yes, by the basic build
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: this is only changing linking visibility, and should not affect the shipped Firefox binary. The backport to esr52 is requested because embedders use esr versions as stable checkpoints of spidermonkey.
[String changes made/needed]: none

Comment 17

[Philip Chimento [:ptomato]]

Thanks!

Comment 18

[Gerry Chang [:gchang]]

Comment on attachment 8885096 [details] [diff] [review]
Backport symbol visibility changes to esr52

This is needed for SpiderMonkey. Let's uplift it to ESR52.3.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

I attempted to land the ESR52 patch but had to backout due to various broken SM jobs.

https://treeherder.mozilla.org/logviewer.html#?job_id=115708728&repo=mozilla-esr52
https://treeherder.mozilla.org/logviewer.html#?job_id=115708436&repo=mozilla-esr52
https://treeherder.mozilla.org/logviewer.html#?job_id=115708405&repo=mozilla-esr52
https://treeherder.mozilla.org/logviewer.html#?job_id=115708729&repo=mozilla-esr52

Comment 20

[Philip Chimento [:ptomato]]

Ugh, I rebased the wrong patch - I took the first attempt, it should have been the second attempt. Will attach again momentarily.

Comment 21

[Philip Chimento [:ptomato]]

Attachment: Backport symbol visibility changes to esr52

Comment 22

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8889020 [details] [diff] [review]
Backport symbol visibility changes to esr52

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

::: js/src/jstypes.h
@@ +98,5 @@
> +// <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=50044>.  Add a way to detect
> +// that so we can locally disable that warning.
> +#if !defined(__clang__) && defined(__GNUC__) && (__GNUC__ < 6 || (__GNUC__ == 6 && __GNUC_MINOR__ <= 4))
> +#define JS_BROKEN_GCC_ATTRIBUTE_WARNING
> +#endif

Oh, nice. Thanks!

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8889020 [details] [diff] [review]
Backport symbol visibility changes to esr52

Overall, Try likes this a lot better than the previous patch. But unfortunately, Windows is still broken :(

https://treeherder.mozilla.org/logviewer.html#?job_id=119132635&repo=try

Comment 24

[Philip Chimento [:ptomato]]

(In reply to Steve Fink [:sfink] [:s:] from comment #22)
> Comment on attachment 8889020 [details] [diff] [review]
> Backport symbol visibility changes to esr52
> 
> Review of attachment 8889020 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jstypes.h
> @@ +98,5 @@
> > +// <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=50044>.  Add a way to detect
> > +// that so we can locally disable that warning.
> > +#if !defined(__clang__) && defined(__GNUC__) && (__GNUC__ < 6 || (__GNUC__ == 6 && __GNUC_MINOR__ <= 4))
> > +#define JS_BROKEN_GCC_ATTRIBUTE_WARNING
> > +#endif
> 
> Oh, nice. Thanks!

I can't take credit for it, it was already in bz's patch :-)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #23)
> Comment on attachment 8889020 [details] [diff] [review]
> Backport symbol visibility changes to esr52
> 
> Overall, Try likes this a lot better than the previous patch. But
> unfortunately, Windows is still broken :(
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=119132635&repo=try

Hmm, difficult. I looked at the offending lines and it seems that somehow these modifications broke std::min() on Windows?!
https://dxr.mozilla.org/mozilla-esr52/source/mfbt/BufferList.h#351

In any case, it looks like the header is included from js/public/StructuredClone.h, which the patch modifies. I don't personally need JSStructuredCloneData to be public so I can attach another patch that simply doesn't modify that file. However, it's a shot in the dark, and I'd need people to keep try-pushing the patches for me.

Another option would be to simply give up on backporting the main patch and just try to get a patch into esr25 making JS::CallArgs::requireAtLeast() public, which is the main reason I need this patch.

Ryan, do you prefer one or the other?

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Sounds like a question for Steve, not me.

Comment 26

[Steve Fink [:sfink] [:s:]]

Looks like esr52 doesn't have my patch to remove all of the direct windows.h includes.

#include <windows.h> has the charming side-effect of redefining a bunch of tokens, including 'min'. Within the JS tree, we should always include windows.h via jswin.h, which undoes that damage. BufferList.h is a bit of a weird case, in that it uses std::min but is in mfbt, and outside of the JS tree there is no standard way of avoiding this problem. People aren't all that aware of jswin.h, so I fixed a bunch of instances that had crept in back in March with bug 1350998.

So the right fix would be to backport bug 1350998 to esr52. I don't know how much we want to backport here, so I did a try push with a minimal spot-fix to #undef min in BufferList.h: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2618f36a38a7364e6d3353ef671c68210c0f94b1

As for try pushes, let me know if you need me to vouch for you. I am more than happy to.

Comment 27

[Steve Fink [:sfink] [:s:]]

Attachment: Backport symbol visibility changes to esr52

Same patch, with the |#undef min| added.

Comment 28

[Philip Chimento [:ptomato]]

Thanks for the updated patch. I appreciate the help, I'm not sure I would have spotted that.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/324cc1bccf3d