1560064 - js::MovableCellHasher<JSObject*> is not exported when SpiderMonkey is compiled with gcc

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Marco Trevisan (Treviño)]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:67.0) Gecko/20100101 Firefox/67.0

Steps to reproduce:

Compile mozjs in linux using g++ (note, this doesn't happen when using clang)

Actual results:

js::MovableCellHasher<JSObject*>::* symbols are not exported.

nm -DC libmozjs-60.so | grep MovableCellHasher\<JSObject

Returns an empty list, while other specializations for js::MovableCellHasher are all exported.

When compiling we get this:

js/src/gc/Barrier.cpp:218:31: warning: type attributes ignored after type is already defined [-Wattributes]
template struct JS_PUBLIC_API(MovableCellHasher<JSObject*>);

I suppose that g++ implicitly defines the template struct based on the usage in other headers, setting a hidden (default) visibility on that, ignoring the explicit definition.

More informations in this GNOME's gjs issue: https://gitlab.gnome.org/GNOME/gjs/issues/217

Expected results:

The symbols are exported and so:

nm -DC libmozjs-60.so | grep MovableCellHasher\<JSObject
0000000000769240 W js::MovableCellHasher<JSObject*>::ensureHash(JSObject* const&)
0000000000769570 W js::MovableCellHasher<JSObject*>::hash(JSObject* const&)
0000000000769900 W js::MovableCellHasher<JSObject*>::match(JSObject* const&, JSObject* const&)
00000000000d2f40 W js::MovableCellHasher<JSObject*>::rekey(JSObject*&, JSObject* const&)
00000000007551f0 W js::MovableCellHasher<JSObject*>::hasHash(JSObject* const&)

Comment 1

[Marco Trevisan (Treviño)]

Attachment: Bug 1560064 - Barrier: Set MovableCellHasher<JSObject*> visibility before being used

Under GCC, the type attributes can't be set more than once, and when this happens
only the firt definition they are ignored.

Since MovableCellHasher<JSObject*> is used in various headers g++ implicitly set
the symbol visibility to default (and thus hidden), making this symbol not to be
exported as it should be.

Move the template specialization with type attributes to Barrier.h, so that this
might happen before any other definition, muting the warning and making the symbol
to be really exported

Fixes https://gcc.gnu.org/bugzilla/show_bug.cgi?id=39159

Comment 2

[Marco Trevisan (Treviño)]

Attachment: Bug 1560064 - Barrier: Remove JS_BROKEN_GCC_ATTRIBUTE_WARNING guards

As per previous commit the warning isn't emitted anymore, so no need to guard
against it.

Also in this case the warning was actually right, so we need to make sure that
we track similar ones.

Depends on D35292

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/75d71e4211bb
Barrier: Set MovableCellHasher<JSObject*> visibility before being used r=sfink
https://hg.mozilla.org/integration/autoland/rev/ca7d45d235a6
Barrier: Remove JS_BROKEN_GCC_ATTRIBUTE_WARNING guards r=sfink

Comment 4

[Alexandru Michis [:malexandru]]

Backed out 2 changesets for causing build bustages.

Backout link: https://hg.mozilla.org/integration/autoland/rev/ea5d71035f64e954a6718cb9e84b51b41a4f21f4

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=ca7d45d235a61397589e68d0dc62a2bc82f9ea8c&selectedJob=252426152

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=252426152&repo=autoland&lineNumber=2553

[task 2019-06-19T08:18:58.621Z] /builds/worker/workspace/gcc/bin/ld: /builds/worker/workspace/build/src/obj-spider/dist/include/mozilla/HashTable.h:1697: undefined reference to `js::MovableCellHasher<JSObject*>::match(JSObject* const&, JSObject* const&)'
[task 2019-06-19T08:18:59.141Z] clang-8: error: linker command failed with exit code 1 (use -v to see invocation)
[task 2019-06-19T08:18:59.142Z] /builds/worker/workspace/build/src/config/rules.mk:525: recipe for target '../../../dist/bin/gdb-tests' failed
[task 2019-06-19T08:18:59.142Z] make[3]: *** [../../../dist/bin/gdb-tests] Error 1
[task 2019-06-19T08:18:59.142Z] make[3]: Leaving directory '/builds/worker/workspace/build/src/obj-spider/js/src/gdb'
[task 2019-06-19T08:18:59.142Z] /builds/worker/workspace/build/src/config/recurse.mk:74: recipe for target 'js/src/gdb/target' failed
[task 2019-06-19T08:18:59.142Z] make[2]: *** [js/src/gdb/target] Error 2
[task 2019-06-19T08:18:59.142Z] make[2]: *** Waiting for unfinished jobs....

Comment 5

[Marco Trevisan (Treviño)]

I've updated the patches, so that they're a no-op under CLANG while still fixing the issue for gcc

Comment 6

[Marco Trevisan (Treviño)]

As expected, under the hood this is a g++ issue, a simple test case for this is attached to the bug I've opened upstream:

• https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90942

Comment 7

[Jeff Walden [:Waldo]]

Driveby, I didn't read comments here super-carefully, but that bug might be a duplicate of the longstanding https://gcc.gnu.org/bugzilla/show_bug.cgi?id=50044 that we have hit earlier. Someone touches nose should fix that upstream.

Comment 8

[Jeff Walden [:Waldo]]

...which itself is apparently a duplicate. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=40068

Comment 9

[Andreea Pavel [:apavel]]

Landing is blocked with "Repository is not supported by Lando"

Aryx can we manually land these patches?

Comment 10

[Marco Trevisan (Treviño)]

One question, how this can be backported to esr60 too?
I wasn't able to find a way to do it via phabricator, as I didn't find a way to set the target branch.

What are the plans for a new point release? As this is getting critical for new gjs and GNOME 3.34, and we would like to have distros to include the change ASAP.

As per the patch itself, I'm also wondering if it's the case for doing it for the other types, to prevent this to happen again, maybe adding an -inl.h that is included in both the .h and the .c file (with guards)?

Comment 11

[Philip Chimento [:ptomato]]

I can help with some of that! For the backport request, set the approval-uplift-esr60 (and 68) flags to "?" and fill the form that appears, after the patches are landed.

But the point release we'll either have to do ourselves (no resolution to bug 1422930 yet), or build SpiderMonkey from the next Firefox point release tarball; looks like it would go in Firefox 60.8 according to https://wiki.mozilla.org/Release_Management/Calendar

Comment 12

[Marco Trevisan (Treviño)]

(In reply to Philip Chimento [:ptomato] from comment #11)

I can help with some of that! For the backport request, set the approval-uplift-esr60 (and 68) flags to "?" and fill the form that appears, after the patches are landed.

Ok, let's do that... As per the distro world I can cover debian and ubuntu, if you can fix the rpm side, too I think most of the things will be ready to accept new gjs (as I expect the arch guys won't have troubles in getting things done too :)).

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a94af4770d78
Barrier: Set MovableCellHasher<JSObject*> visibility before being used. r=sfink
https://hg.mozilla.org/integration/autoland/rev/359226eb7bf5
Barrier: Remove JS_BROKEN_GCC_ATTRIBUTE_WARNING guards. r=sfink

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

The patch was lacking the author information. In the future, please set up the .hgrc file in your home directory like described at https://developer.mozilla.org/en-US/docs/Mozilla/Mercurial/Installing_Mercurial

Comment 15

[Marco Trevisan (Treviño)]

(In reply to Sebastian Hengst [:aryx] (needinfo on intermittent or backout) from comment #14)

The patch was lacking the author information. In the future, please set up the .hgrc file in your home directory like described at https://developer.mozilla.org/en-US/docs/Mozilla/Mercurial/Installing_Mercurial

Oh weird I've it... Although I think for 2nd revision I've pushed I was using git... Not sure where it went missing.

Comment 16

[Marco Trevisan (Treviño)]

Ah, also not sure why the patch that landed was missing the whole description as it was in https://hg.mozilla.org/integration/autoland/rev/75d71e4211bb

Comment 17

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/a94af4770d78
https://hg.mozilla.org/mozilla-central/rev/359226eb7bf5

Comment 18

[Philip Chimento [:ptomato]]

Comment on attachment 9072747 [details]
Bug 1560064 - Barrier: Set MovableCellHasher<JSObject*> visibility before being used

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: The patch fixes a symbol that is not exported in the mozjs shared library, but should be. Downstream SpiderMonkey embedders need to use this symbol in order to implement a major performance improvement.
• User impact if declined: Downstreams will have to carry patches for ESR60 and ESR68 releases until the fix appears in ESR76(?) next year.
• Fix Landed on Version: 69
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It's a linker issue that I believe is only relevant to standalone builds of SpiderMonkey.
• String or UUID changes made by this patch: None

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9072747 [details]
Bug 1560064 - Barrier: Set MovableCellHasher<JSObject*> visibility before being used

Fixes a build issue for standalone Spidermonkey built with GCC, approved for 68rc1 and 60.8esr.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3a892f49828e
https://hg.mozilla.org/releases/mozilla-beta/rev/f3b929b28ab8

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/018a95ef3070
https://hg.mozilla.org/releases/mozilla-esr60/rev/40875ef8e478