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

MozReview-Commit-ID: 264vsCvhh9Z

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

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

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?

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

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.

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.

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.

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"?

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!

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.

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

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

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

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

Sounds like a question for Steve, not me.

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.

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

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