Make --enable-shared-js link again

RESOLVED FIXED in Firefox -esr52

Status

()

RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: bzbarsky, Assigned: sfink)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox-esr52 fixed, firefox55 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

'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.
Created attachment 8846109 [details] [diff] [review]
Make --enable-shared-js link again, at least for an opt mac build with intl api disabled

MozReview-Commit-ID: 264vsCvhh9Z
Attachment #8846109 - Flags: review?(sphink)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
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.)
Attachment #8846109 - Flags: review?(sphink) → review+
> 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

2 years ago
Pushed by bzbarsky@mozilla.com:
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
Backed out for build failures on Linux and maybe windows....  Looks like different compilers treat forward-declarations around this stuff differently.
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?
Flags: needinfo?(jwalden+bmo)

Comment 8

2 years ago
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.
Flags: needinfo?(jwalden+bmo)

Comment 9

a year ago
Pushed by bzbarsky@mozilla.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/16f71512d587
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 11

a year ago
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.
For my purposes, that's absolutely correct.
Created attachment 8885096 [details] [diff] [review]
Backport symbol visibility changes to esr52
Attachment #8885096 - Flags: review?(sphink)
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.
Also, could someone with permissions please make this bug block "sm.embedding"?
(Assignee)

Updated

a year ago
Attachment #8885096 - Flags: review?(sphink)
Attachment #8885096 - Flags: review+
Attachment #8885096 - Flags: approval-mozilla-esr52?
(Assignee)

Comment 16

a year ago
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
Thanks!
status-firefox-esr52: --- → affected
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.
Attachment #8885096 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Ugh, I rebased the wrong patch - I took the first attempt, it should have been the second attempt. Will attach again momentarily.
Flags: needinfo?(philip.chimento)
Created attachment 8889020 [details] [diff] [review]
Backport symbol visibility changes to esr52
Attachment #8889020 - Flags: review?(sphink)
Attachment #8885096 - Attachment is obsolete: true
(Assignee)

Comment 22

a year ago
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!
Attachment #8889020 - Flags: review?(sphink) → review+
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
Attachment #8889020 - Flags: feedback-
Flags: needinfo?(philip.chimento)
(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?
Flags: needinfo?(philip.chimento) → needinfo?(ryanvm)
Sounds like a question for Steve, not me.
Flags: needinfo?(ryanvm) → needinfo?(sphink)
(Assignee)

Comment 26

a year ago
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.
Flags: needinfo?(sphink)
(Assignee)

Comment 27

a year ago
Created attachment 8891686 [details] [diff] [review]
Backport symbol visibility changes to esr52

Same patch, with the |#undef min| added.
Attachment #8891686 - Flags: feedback?(ryanvm)
(Assignee)

Updated

a year ago
Assignee: bzbarsky → sphink
(Assignee)

Updated

a year ago
Attachment #8891686 - Flags: review+
(Assignee)

Updated

a year ago
Attachment #8889020 - Attachment is obsolete: true
Thanks for the updated patch. I appreciate the help, I'm not sure I would have spotted that.

Comment 29

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/324cc1bccf3d
status-firefox-esr52: affected → fixed
Attachment #8891686 - Flags: feedback?(ryanvm) → feedback+

Updated

9 months ago
See Also: → bug 1419002
You need to log in before you can comment on or make changes to this bug.