Closed Bug 1560064 Opened 6 months ago Closed 6 months ago

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

Categories

(Core :: JavaScript: GC, defect, major)

Unspecified
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox-esr60 --- fixed
firefox68 --- fixed
firefox69 --- fixed

People

(Reporter: mail, Assigned: mail)

References

(Blocks 1 open bug, )

Details

Attachments

(2 files)

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

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

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

Severity: normal → major
OS: Unspecified → Linux
Pushed by sfink@mozilla.com:
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

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

Flags: needinfo?(mail)

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

Flags: needinfo?(mail)

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:

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.

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

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

Aryx can we manually land these patches?

Flags: needinfo?(aryx.bugmail)
Keywords: checkin-needed

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

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

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

Pushed by archaeopteryx@coole-files.de:
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

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

Flags: needinfo?(aryx.bugmail)

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

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

Status: UNCONFIRMED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

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
Attachment #9072747 - Flags: approval-mozilla-esr68?
Attachment #9072747 - Flags: approval-mozilla-esr60?
Attachment #9072748 - Flags: approval-mozilla-esr68?
Attachment #9072748 - Flags: approval-mozilla-esr60?

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.

Attachment #9072747 - Flags: approval-mozilla-esr68?
Attachment #9072747 - Flags: approval-mozilla-esr60?
Attachment #9072747 - Flags: approval-mozilla-esr60+
Attachment #9072747 - Flags: approval-mozilla-beta+
Attachment #9072748 - Flags: approval-mozilla-esr68?
Attachment #9072748 - Flags: approval-mozilla-esr60?
Attachment #9072748 - Flags: approval-mozilla-esr60+
Attachment #9072748 - Flags: approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.