Opened 2 years ago

Last modified 9 days ago

#4893 new enhancement

Build 0ad using system libmozjs-52 / SpiderMonkey52

Reported by: Ludovic Owned by:
Priority: Should Have Milestone: Alpha 24
Component: Core engine Keywords:
Cc: fabio, Krinkle Patch:

Description

I maintain the Debian package of 0ad. 0ad fails to build on arm64 because mozjs83 fails to build. The error is:

/<<PKGBUILDDIR>>/libraries/source/spidermonkey/mozjs-38.0.0/js/src/jit/shared/CodeGenerator-shared-inl.h:232:10: error: 'class js::jit::MacroAssembler' has no member named 'verifyHeapAccessDisassembly'
     masm.verifyHeapAccessDisassembly(begin, end,
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~

See https://buildd.debian.org/status/fetch.php?pkg=0ad&arch=arm64&ver=0.0.22-1&stamp=1508351579&raw=0 for the complete build log.

One solution would be to use the mozjs package provided by Debian and already available for arm64.

Debian unstable provides libmozjs-52-dev. I tried to patch 0ad (work-in-progress patch attached) to use this mozjs version but the build fails with:

../../../source/scriptinterface/NativeWrapperDecls.h:79:51: error: ‘jsval’ has not been declared
  static bool callMethod(JSContext* cx, uint argc, jsval* vp);
                                                   ^
../../../source/scriptinterface/NativeWrapperDecls.h:79:51: note: in definition of macro ‘OVERLOADS’

[...]

../../../source/scriptinterface/NativeWrapperDefns.h:133:50: error: cannot convert ‘int*’ to ‘JS::Value*’ for argument ‘2’ to ‘JS::CallArgs JS::CallArgsFromVp(unsigned int, JS::Value*)’
   JS::CallArgs args = JS::CallArgsFromVp(argc, vp); \

it looks like the 0ad code needs some changes to migrate from mozjs-38 to mozjs-52.

Is there any interest to support a more recent (and supported, with security bugs fixed, etc.) version of mozjs for 0ad?

Attachments (1)

mozjs52.patch (4.6 KB) - added by Ludovic 2 years ago.
preliminary patch to build 0ad using system libmozjs-52

Download all attachments as: .zip

Change History (30)

Changed 2 years ago by Ludovic

Attachment: mozjs52.patch added

preliminary patch to build 0ad using system libmozjs-52

comment:1 Changed 23 months ago by Ludovic

Nobody interested in using a supported libmozjs library?

comment:2 Changed 23 months ago by elexis

First we have to update to SpiderMonkey 45, work was started here https://github.com/leper/0ad/tree/sm45 but there are still a number of things todo (https://github.com/leper/0ad/blob/380f534acbeb04214307b7e6d8d49ee89146b39c/SM45_TODO).

You can find some previous SM upgrade tickets on trac, that is a considerable amount of work, much more than a small patch fixing some compile warnings.

For instance the SM38 upgrade was done in 35 commits: r18689

comment:3 Changed 23 months ago by elexis

Keywords: mozjs removed
Summary: Build 0ad using system libmozjs-52?Build 0ad using system libmozjs-52 / SpiderMonkey52

comment:4 Changed 23 months ago by stanislas69

Can't we skip to 52 directly ?

comment:5 Changed 23 months ago by elexis

Keywords: rfc added
Milestone: BacklogWork In Progress

We have to find out what has changed meanwhile. Pyrogenesis code that is incompatible to sm45 will be incompatble to sm52 too, so skipping will be more work as far as I understand.

On the other hand compile warnings should always be addressed, so we should review the uploaded patch either way.

comment:6 Changed 17 months ago by Michael Shigorin

I'd be willing to donate some money for this work, ALT's e2k port has mozjs52 but getting mozjs38 there is problematic right now.

comment:7 Changed 17 months ago by elexis

There is a patch with some days of started work for sm60: Phab:D1510. (I still don't know if it will be easier to jump to the latest stable or to progress one release at a time. The patch looked like it might be manageable to jump.)

comment:8 Changed 17 months ago by Michael Shigorin

Heh, I won't see 60 on e2k for quite some time I guess...

comment:9 Changed 17 months ago by elexis

Our sm60 patch could be next month or next decade too.

About the version, Itms mentioned in an internal thread that the next 'LTS' is SM60, but I couldn't verify this for now. According to https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Releases the latest release is SM52 and no sucessor officially scheduled yet.

comment:10 Changed 11 months ago by elexis

Keywords: rfc removed
Milestone: Work In ProgressAlpha 24

comment:11 Changed 11 months ago by Michael Shigorin

2 elexis: as previously noted, I'm willing to donate some money, especially if someone can do the transition from mozjs38 with an explicit stop at mozjs52 (that will gain a whole new target platform). Let it be 50 euros to be more specific, and please email me with proper contact if that's acceptable.

comment:12 Changed 11 months ago by elexis

Notice that our SM includes few patches: https://trac.wildfiregames.com/browser/ps/trunk/libraries/source/spidermonkey The updated version might also require some, implementation will tell.

comment:13 Changed 11 months ago by Michael Shigorin

Notice that our SM includes few patches

It's fine as long as these are fixes not upstream-incompatible kludges for a particular project :-) SM52 is getting skipped unconditionally, right?.. (I know the "you need it you do it" rule, no moaning)

comment:14 Changed 11 months ago by elexis

SM52 is getting skipped unconditionally, right?

By default 0ad is compiled with the spidermonkey version in 0ad/libraries/source/spidermonkey/, one can supposedly compile with a shared library using --with-system-mozjs38 wiki:BuildInstructions in case you mean that.

stop at mozjs52

Until there is a new version of spidermonkey that is tagged as "release" that is? Perhaps it wouldn't be too bad for pyrogenesis/0ad to to update beyond the most recent "release" as wraitii did, if the "release" doesn't really differ in stability or maintenance from the most recent version. Debian for instance has mozjs60 in their repository, but mozjs52 is still the most recent "release". The release packe for mozjs52 is completely empty https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Releases/52 and I can't even find the download via the method they describe after browsing through hundreds of mozjs52 patches. So I suppose "sm52" is the "prerelease" from https://ftp.mozilla.org/pub/spidermonkey/prereleases/52/pre1/mozjs-52.9.1pre1.tar.bz2 I'll review what wraitii and Itms did so far, an update of SM is overdue either way.

comment:15 Changed 11 months ago by elexis

In r20062:

Replace deprecated jsval with JS::Value.
Remove unused SGUIBaseSettings and GUI comment.
Fix indentation of a macro, refs D794.

Differential Revision: ​https://code.wildfiregames.com/D838
Review by: leper.
Itms came up with the same patch for the SpiderMonkey 45 update independently.

comment:16 Changed 11 months ago by fabio

Cc: fabio added

comment:17 Changed 10 months ago by Itms

In 22052:

SpiderMonkey-related changes in preparation for the upgrade to SpiderMonkey 45, refs #4893.

Reviewed By: wraitii, elexis
Differential Revision: https://code.wildfiregames.com/D1716

comment:18 Changed 5 months ago by Krinkle

Cc: Krinkle added

comment:19 Changed 5 months ago by Ludovic

The next Debian version (buster) should be released in July 2019. It will provide libmozjs-60-0 and libmozjs-52-0.

It would be fine to target support of libmozjs-60-0 (instead of version 52) if possible.

comment:20 Changed 4 months ago by Itms

In 22516:

Improvements to simulation hotloading before the SM upgrade, refs #4893.

SM45 will enforce property attributes described at https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/Property_attributes. We thus cannot hotload properties with the DontDelete? attribute.
Fix an oversight in the hotloading code.
Rename a confusing parameter.

Reviewed By: wraitii
Differential Revision: https://code.wildfiregames.com/D1844

comment:21 Changed 4 months ago by elexis

In 22622:

Replace ScriptInterface? ErrorReporter? layered hack from rP8063 with proper code, refs rP8975.

Avoids preparation for the SpiderMonkey 45 update, refs #4893 / D1510 .
Add JSAutoRequest missing from rP8063 too.

Differential Revision: https://code.wildfiregames.com/D2152
Reviewed By: Itms

comment:22 Changed 4 months ago by Itms

In 22627:

Upgrade SpiderMonkey to version 45.0.2, refs #4893.

Thanks to wraitii, elexis, Krinkle and historic_bruno for contributions and comments, and to gentz, madpilot, s0600204 and Stan for testing and indirect contributions.

Differential Revision: https://code.wildfiregames.com/D1510

comment:23 Changed 4 months ago by Itms

In 22629:

Revert use of the isRunOnce flag after removal in rP22627 of compile'n go, refs #4893.

This breaked a SM assertion, which was only caught in debug mode. It could have led to subtle bugs during the compilation of JS scripts.
Still set the flag to its default value, because SM devs wanted to change the default in the future.

comment:24 Changed 4 months ago by elexis

explicit stop at mozjs52

Meanwhile Firefox / SpiderMonkey is at revision 68, so it sounds hurtful not to go further.

I was wondering whether one could introduce a macro that changes the code to work with 52 at option, but that will probably not scale, because we probably want to start using more recnet spidermonkey version features, in specific rooting a std::vector or std::map in a JSContext as demonstrated in this sm60 example: https://github.com/spidermonkey-embedders/spidermonkey-embedding-examples/blob/esr60/examples/tracing.cpp

comment:25 Changed 4 months ago by Michael Shigorin

For one, I'm still eager to donate e.g. $50 for support of Firefox 52's mozjs *if* that comes feasible (I understand that it can be days or weeks of work more and the suggestion might be just inappropriate). My personal reason is that it's the topmost version of the engine I've got on e2k (Elbrus 2000) architecture, and getting 0ad here would be so nice :-)

comment:26 Changed 3 months ago by elexis

In 22660:

Update update-workspaces.sh and premake5.lua with the with-system-mozjs45 flag from SpiderMonkey 45 in rP22627, refs #4893.

Differential Revision: https://code.wildfiregames.com/D2170

comment:27 Changed 3 months ago by elexis

In 22661:

Replace JS_THIS_OBJECT and JS::CallReceiver? in preparation for SpiderMonkey 49 and 61.

Differential Revision: https://code.wildfiregames.com/D2173
Refs #4893, D1699, D2142, D844, rP7259, ...
Refs https://bugzilla.mozilla.org/show_bug.cgi?id=1270977
Refs https://bugzilla.mozilla.org/show_bug.cgi?id=1255800
Tested on: clang 8, VS2015, Jenkins

comment:28 Changed 3 months ago by wraitii

Wasn't too sure where to drop this, but it appears SM70 might have serious performance optimisations for our use cases: https://hacks.mozilla.org/2019/08/the-baseline-interpreter-a-faster-js-interpreter-in-firefox-70/

comment:29 Changed 9 days ago by elexis

TODO: It seems there is a SpiderMonkey segfault, see #5636, reproducible with:

	Object.defineProperty(this, "f", {
	    get: function() {
	        [].keys
	        return (function() {
	            f();
	        });
	    }
	});
	f();
Note: See TracTickets for help on using tickets.