--enable-shared-js builds are totally broken
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox44 | --- | affected |
People
(Reporter: bzbarsky, Unassigned, NeedInfo)
References
Details
Attachments
(2 obsolete files)
This is fallout from bug 1123237 and some others I have not determined yet. The basic problem is that the MemoryProfiler is linked into libxul, as far as I can tell, but directly calls MemProfiler::* functions that are in libmozjs in --enable-shared-js builds. Those functions aren't marked JS_PUBLIC_API or JS_FRIEND_API, so they're not exported from libmozjs and linking libxul fails. Of course even before that bug we seem to have issues with JSPropertyDescriptor::trace being used from inside libxul, and a bunch of JS::ubi stuff... If we don't support SpiderMonkey as a shared library, we should kill off the configure option or something.
Comment 1•9 years ago
|
||
I have no strong opinion here, but I'd lean towards maintaining it unless we have some compelling reason not to. I'd be happy to make patches for anything that's in my wheelhouse.
Comment 2•9 years ago
|
||
And I'm happy to do the JS::ubi stuff. Is this basically adding JS_{PUBLIC,FRIEND}_API to everything exposed to embedders?
Reporter | ||
Comment 3•9 years ago
|
||
As far as I know, yes.
Comment 4•9 years ago
|
||
As far as JS::ubi is concerned, those are meant to be public APIs; if we haven't marked them up with the proper attributes, that's our mistake.
Comment 5•9 years ago
|
||
Comment 6•9 years ago
|
||
Try push for attachment 8671659 [details] [diff] [review]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6ce590c3901
Comment 7•9 years ago
|
||
Comment on attachment 8671659 [details] [diff] [review] Use JS_PUBLIC_API in js/public/UbiNode*.h Review of attachment 8671659 [details] [diff] [review]: ----------------------------------------------------------------- wfm
Comment 8•9 years ago
|
||
Updated•9 years ago
|
Comment 9•9 years ago
|
||
Hopefully that will fix the bustage on the last try push. https://treeherder.mozilla.org/#/jobs?repo=try&revision=b57aa243bb5c
Comment 10•9 years ago
|
||
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #9) > Hopefully that will fix the bustage on the last try push. > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=b57aa243bb5c That fixed the majority of bustage, but OSX 10.7 is still busted. I haven't been able to repro the bustage on OSX 10.10. I don't understand the given linker error: > ld: bad codegen, pointer diff in __ZN2js14DebuggerMemory10takeCensusEP9JSContextjPN2JS5ValueE to global weak symbol __ZTVN2JS3ubi11RootedCountE for architecture i386 First of all, that's somewaht esoteric and I'm not really sure what the error means; googling hasn't enlightened me. Secondly, RootedCount is already marked JS_PUBLIC_API. It inherits from JS::CustomAutoRooter which is JS_PUBLIC_API, and wraps a CountBasePtr which is an alias for UniquePtr<CountBase, CountDeleter>. Both CountBase and CountDeleter are marked JS_PUBLIC_API. UniquePtr is not marked JS_PUBLIC_API, but its just a header so I feel like it should be getting inlined and we shouldn't have ABI issues with UniquePtr. Maybe that is a misguided assumption? Anyone have any hints? Thanks.
Comment 11•9 years ago
|
||
Reminder to self:
> 4:23 PM <Waldo> fitzgen: maybe you might need to mark UniquePtr as MFBT_API; we have some markers like that on SHA1Sum
Comment 12•9 years ago
|
||
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #11) > Reminder to self: > > > 4:23 PM <Waldo> fitzgen: maybe you might need to mark UniquePtr as MFBT_API; we have some markers like that on SHA1Sum Adding MFBT_API to UniquePtr built fine locally, but try doesn't seem to like it much: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9c6dbad4bc6
Comment 13•8 years ago
|
||
I successfully built an --enable-shared-js build the other day, so I think this has since been resolved. Reopen if this isn't the case.
Updated•8 years ago
|
Reporter | ||
Comment 14•8 years ago
|
||
That's odd. MemProfiler::GetMemProfiler is not js public api or friend api that I can see, and is used from tools/memory-profiler/MemoryProfiler.cpp...
Reporter | ||
Comment 15•8 years ago
|
||
OK, I just tried this on current tip as of earlier today. The build fails like so: 0:15.99 Undefined symbols for architecture x86_64: 0:15.99 "_icudt56_dat", referenced from: 0:15.99 openCommonData(char const*, int, UErrorCode*) in udata.o 0:15.99 ld: symbol(s) not found for architecture x86_64 which is not identical to the problem this bug was filed on, but just means that we never even got to trying to link libxul; we failed to link even libmozjs.
Comment 16•8 years ago
|
||
Could bug 1125784 be a potential cause here? It seems backing that out improves buildability.
Comment 17•6 years ago
|
||
The leave-open keyword is there and there is no activity for 6 months. :sdetar, maybe it's time to close this bug?
Comment 18•6 years ago
|
||
Jason, should we close this bug?
Reporter | ||
Comment 19•6 years ago
|
||
You should either make the option work or remove it. Either way, code changes are needed here.
Comment 20•6 years ago
|
||
I'm strongly in favor of making the option work. Having a separate js lib instead of having it in the ever-growing xul lib behemoth has some building and debugging advantages.
Comment 22•5 years ago
|
||
The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?
Comment 23•5 years ago
|
||
How about fixing it, instead?
Reporter | ||
Comment 24•5 years ago
|
||
Bug 1554056 just removed the option.
Updated•5 years ago
|
Updated•5 years ago
|
Comment 25•5 years ago
|
||
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #24)
Bug 1554056 just removed the option.
Well, that's one way to "solve " it, sure. I guess the points I made about the building/debugging, even if agreed with, were not considered.
Thanks for applying the steamroller. [/sarcasm]
Reporter | ||
Comment 26•5 years ago
|
||
I think they were considered, fwiw. If this had been a simple matter of exporting some more things, etc, I would have just done the work to make it work, since I have used this to good effect in profiling in the past.
But the ICU situation is a mess and no one was really available to solve that mess...
Description
•