519856 - NJ merge: move VMPI stuff from avmplus

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Graydon Hoare :graydon]

Attachment: move stuff around

Some portions of Adobe code reference "VMPI.h", and it seems like it'd be good for keeping the stub-code relationships simple to match the layout of their portability layers. This patch moves some of the avmplus-resident "VMPI" code to a separate VMPI.h / VMPI.cpp pair.

There's no new code here, just movement.

Will need to try-server it a bit to make sure various platforms are ok with it, but it seems to work here.

This is prerequisite for getting nanojit-central running.

Comment 1

[Nicholas Nethercote [inactive]]

Comment on attachment 403910 [details] [diff] [review]
move stuff around

r=me if it passes try server.

Comment 2

[Graydon Hoare :graydon]

Attachment: update

This patch has grown just a little bit of hair that escapes the nanojit/ subdirectory; namely the definition of _STDINT_H in the end of jsstdint.h. This is a lame but workable way of communicating between the two insulation layers here (jsstdint.h and VMPI.h) that the former has been seen, and not to include the stdint bits of the latter.

r?'ing brendan for approval in this mess. It ain't pretty but I can't think of a nicer way that doesn't pollute the VMPI.h with JS stuff, and I'm trying to get all JS stuff out of nanojit/

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 404139 [details] [diff] [review]
update

Gonna defer to jimb, otherwise rs=me.

/be

Comment 4

[Jim Blandy :jimb]

Comment on attachment 404139 [details] [diff] [review]
update

There are two things I don't understand about this patch:

- Why do we check HAVE_CONFIG_H and #include "config.h" in avmplus.h?  avmplus.h is our shim between nanojit and SM, isn't it?  Whose config.h would that be?

- Why did the WinCE AllocCodeChunk and FreeCodeChunk cases disappear in avmplus.cpp?

Comment 5

[Graydon Hoare :graydon]

The config.h bit is actually ... used in nanojit-central, not here. The idea with this merge is to at least sync up tracemonkey's avmplus.h with the avmplus.h that exists in nanojit-central. We can't be identical to the avmplus.h that's in tamarin-redux, at least not at the moment, because it actually *does* pull in heaps of tamarin-specific code. But I figured identity with *one* side of the merge would be better than none. Maybe at some point we can edit out dependency on avmplus.h entirely.

I suppose I can guard that bit with #define NANOJIT_CENTRAL as well, if you like? This is all sort of a stop-gap. I figure once we have an established rhythm of patch-flow between repositories, coordinating further incremental cleanup in the shims and include-dependencies will be a bit easier, as we'll have many eyes focused on the task of deciding the best arrangement.

The loss of the WinCE bits was accidental during code movement, I'll restore them.

Comment 6

[Jim Blandy :jimb]

You mean #ifdef NANOJIT_CENTRAL?  That might make it clearer; a comment would be fine.

Comment 7

[Graydon Hoare :graydon]

I mean:

-#ifdef HAVE_CONFIG_H
+#if defined(HAVE_CONFIG_H) && defined(NANOJIT_CENTRAL)

But can just conditional on NANOJIT_CENTRAL as well. Figured I'd keep the autoconf @DEFS@-based mechanism of switching between a config.h file and a list of -D flags functioning as-is, just conditionalize the *whole thing* on NANOJIT_CENTRAL.

Either way, I don't care much.

Comment 8

[Graydon Hoare :graydon]

Attachment: refresh the patch

This variant incorporates the described changes.

Comment 9

[Jim Blandy :jimb]

Comment on attachment 404373 [details] [diff] [review]
refresh the patch

Looks good.  My only thought is, since <stdint.h> and "jsstdint.h" both define INT8_MAX, etc., would it make sense to test for that macro's definedness instead of introducing a new one that is a shared secret between jsstdint.h and avmplus.h?

Either way, r+.

Comment 10

[Graydon Hoare :graydon]

http://hg.mozilla.org/tracemonkey/rev/9c18491c7b39

Comment 11

[Graydon Hoare :graydon]

Fun and games. INT8_MAX might work too, sure, but those in turn are only active in a C++ include environment that happens to have defined(__STDC_LIMIT_MACROS). Overall I think the approach in the patch is plausibly the least gnarly. For now.

Comment 12

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/9c18491c7b39