1209684 - --enable-shared-js builds are totally broken

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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

[Terrence Cole [:terrence]]

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

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

And I'm happy to do the JS::ubi stuff. Is this basically adding JS_{PUBLIC,FRIEND}_API to everything exposed to embedders?

Comment 3

[Boris Zbarsky [:bzbarsky]]

As far as I know, yes.

Comment 4

[Jim Blandy :jimb]

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

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Use JS_PUBLIC_API in js/public/UbiNode*.h

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push for attachment 8671659 [details] [diff] [review]:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6ce590c3901

Comment 7

[Terrence Cole [:terrence]]

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

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Use JS_PUBLIC_API in js/public/UbiNode*.h

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Hopefully that will fix the bustage on the last try push.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b57aa243bb5c

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(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

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(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

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 14

[Boris Zbarsky [:bzbarsky]]

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

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

[Mark Straver]

Could bug 1125784 be a potential cause here? It seems backing that out improves buildability.

Comment 17

[BugBot [:suhaib / :marco]]

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

[Steven DeTar [:sdetar]]

Jason, should we close this bug?

Comment 19

[Boris Zbarsky [:bzbarsky]]

You should either make the option work or remove it.  Either way, code changes are needed here.

Comment 20

[Mark Straver]

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 21

[Jason Orendorff [:jorendorff]]

Agreed.

Comment 22

[BugBot [:suhaib / :marco]]

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

[Mark Straver]

How about fixing it, instead?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Bug 1554056 just removed the option.

Comment 25

[Mark Straver]

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

Comment 26

[Boris Zbarsky [:bzbarsky]]

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