536223 - Code changes to make JIT working in Symbian avmshell

Status: RESOLVED FIXED

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect)

Description

[Samuli Kekki]

User-Agent:       Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.1; Trident/4.0; GTB5; .NET CLR 3.0.04506.648; .NET CLR 3.5.21022; .NET CLR 3.0.4506.2152; .NET CLR 3.5.30729; .NET CLR 2.0.50727)
Build Identifier: Patch against current FlashRuntime mainline (12/20/2009)

Contains patches and new files.

Reproducible: Always

Comment 1

[Samuli Kekki]

Attachment: Patch to some files in MMgc folder.

Comment 2

[Samuli Kekki]

Attachment: Patch to some files in nanojit folder.

Comment 3

[Samuli Kekki]

Attachment: Patch to some files in platform folder.

Comment 4

[Samuli Kekki]

Attachment: Patch to some files in shell folder.

Comment 5

[Steven Johnson]

Comment on attachment 418678 [details] [diff] [review]
Patch to some files in MMgc folder.

Tommy needs to review the operator-new stuff, Rick or Edwin for the code-memory.

Comment 6

[Samuli Kekki]

Attachment: Patch to some files in VMPI folder.

In addition, please also replace the whole MMgcPortSymbian.cpp with the attached file. The patch looked too big/error prone.

Comment 7

[Steven Johnson]

Comment on attachment 418679 [details] [diff] [review]
Patch to some files in nanojit folder.

Changes to nanojit generally need to be review by someone at both Adobe and Mozilla.

Comment 8

[Steven Johnson]

Comment on attachment 418680 [details] [diff] [review]
Patch to some files in platform folder.

delegating to Tommy because of MAP_TO_OS_NEW... otherwise looks fine to me

Comment 9

[Samuli Kekki]

Attachment: Please replace the existing MMgcPortSymbian.cpp with this file (under VMPI)

Comment 10

[Samuli Kekki]

Attachment: New file, please put under platform/symbian.

Comment 11

[Steven Johnson]

Comment on attachment 418681 [details] [diff] [review]
Patch to some files in shell folder.

Rejecting solely because indentation style doesn't match rest of the file... please fix and resubmit. (Yes, we are picky about these things.)

Comment 12

[Samuli Kekki]

Attachment: New file, please put under platform/symbian

Comment 13

[Samuli Kekki]

Attachment: New file, please put under platform/symbian

Comment 14

[Samuli Kekki]

Attachment: New file, please put under platform/symbian

Comment 15

[Steven Johnson]

Comment on attachment 418682 [details] [diff] [review]
Patch to some files in VMPI folder.

Delegating to Rick due to the code-memory-allocation change.

Comment 16

[Samuli Kekki]

Attachment: New file, please put under platform/symbian

Comment 17

[Samuli Kekki]

Attachment: New file, please put under folder shell.

Comment 18

[Steven Johnson]

Comment on attachment 418688 [details]
New file, please put under platform/symbian

We prefer not to check in code with permanently-commented-out chunks; this file has what appears to be two empty stubs. We should either check in working code or empty stubs.

Comment 19

[Samuli Kekki]

Attachment: New version of this patch, now indentention is better.

Steven originally rejected a patch to this file. This should be better.

Comment 20

[Steven Johnson]

Comment on attachment 418689 [details]
New file, please put under folder shell.

How can this code work? avmshell::Run is commented out.

Comment 21

[Steven Johnson]

Comment on attachment 418690 [details] [diff] [review]
New version of this patch, now indentention is better.

There's still gratuitous indentation change in unrelated code; review+ is conditional on ignoring those.

Comment 22

[Samuli Kekki]

Attachment: A new version of this file, now commented code has been removed.

Steven rejected the original version of this file, this should help.

Comment 23

[Samuli Kekki]

(In reply to comment #20)
> (From update of attachment 418689 [details])
> How can this code work? avmshell::Run is commented out.

Hi Steven. Only the return value is commented out. Do you want that I clean up the file at that location?

Comment 24

[Steven Johnson]

Comment on attachment 418692 [details]
A new version of this file, now commented code has been removed.

Does your compiler complain about "unused parameters"? If so, might need to tweak this to prevent warnings.

Comment 25

[Steven Johnson]

Comment on attachment 418689 [details]
New file, please put under folder shell.

Ah, you are correct, my bad.

Comment 26

[Samuli Kekki]

(In reply to comment #21)
> (From update of attachment 418690 [details] [diff] [review])
> There's still gratuitous indentation change in unrelated code; review+ is
> conditional on ignoring those.

You are right, the patch looks bad. I try to produce a new one, unless you can use this.

Comment 27

[Samuli Kekki]

(In reply to comment #24)
> (From update of attachment 418692 [details])
> Does your compiler complain about "unused parameters"? If so, might need to
> tweak this to prevent warnings.

You are right, I do it right away.

Comment 28

[Samuli Kekki]

Attachment: A third version of OSDepSymbian.cpp.

Unused variable names are now commented out.

Comment 29

[Samuli Kekki]

Attachment: Third version of the patch for avmshell-features.h

No only my code additions are included in the patch.

Comment 30

[Nicholas Nethercote [inactive]]

(In reply to comment #7)
> (From update of attachment 418679 [details] [diff] [review])
> Changes to nanojit generally need to be review by someone at both Adobe and
> Mozilla.

Do they?  Not according to https://developer.mozilla.org/en/NanojitMerge, at
least not for NJ-only changes:

"Review for a nanojit internal patch (no changes to external API or embedding
assumptions) requires at least one member of either team.
Review for a nanojit external patch (API changes or embedding / environment
changes) requires sign off from at least one member of both teams."

The "either team" wording isn't that clear, but given the following sentence I
take it to mean that only one reviewer is necessary.  Having said that, for
larger NJ-only patches I think having a reviewer from each team is a good idea.

I'm not a good person to review this patch, BTW, as I don't know much about ARM
or the allocators.  I'd suggest Graydon or Jacob.

Comment 31

[Steven Johnson]

(In reply to comment #30)
> Do they?  Not according to https://developer.mozilla.org/en/NanojitMerge, at
> least not for NJ-only changes:

I stand corrected.

Comment 32

[Nicholas Nethercote [inactive]]

CC'ing people from the other side is probably a good idea for all NJ changes.

Comment 33

[Rick Reitmaier]

Comment on attachment 418678 [details] [diff] [review]
Patch to some files in MMgc folder.

Not sure about the MAP_TO_OS_NEW define. 

I imagine we want to update avmfeatures.as and/or associated machinery once we start building tamarin symbian

Comment 34

[Rick Reitmaier]

Comment on attachment 418679 [details] [diff] [review]
Patch to some files in nanojit folder.

it looks like we've made the #if more restrictive.  What will be the case if ARMCC is not defined?

Comment 35

[Rick Reitmaier]

Comment on attachment 418682 [details] [diff] [review]
Patch to some files in VMPI folder.

I'm not sure how this is working, since CodeAlloc calls VMPI_SetPageProtection. 

I would instead suggest that you just leave the empty stubs.

Also I'm wondering if the new VMPI api should be ifdef'd SYMBIAN?

Requesting Lars to have a look; he may have suggestions on how to best factor this concept.

Comment 36

[Samuli Kekki]

(In reply to comment #35)
> (From update of attachment 418682 [details] [diff] [review])
> I'm not sure how this is working, since CodeAlloc calls VMPI_SetPageProtection. 
> I would instead suggest that you just leave the empty stubs.
> Also I'm wondering if the new VMPI api should be ifdef'd SYMBIAN?
> Requesting Lars to have a look; he may have suggestions on how to best factor
> this concept.

VMPI_SetPageProtection is moved to MMgcPortSymbian.cpp. There is an empty stub.

Comment 37

[Samuli Kekki]

(In reply to comment #34)
> (From update of attachment 418679 [details] [diff] [review])
> it looks like we've made the #if more restrictive.  What will be the case if
> ARMCC is not defined?

In the else case there is the same code implemented in C. This intrinsic didn't compile with RVCT 2.2. However I didn't find a list of supported intrinsics in this compiler.

Comment 38

[Samuli Kekki]

(In reply to comment #33)
> (From update of attachment 418678 [details] [diff] [review])
> Not sure about the MAP_TO_OS_NEW define. 
> I imagine we want to update avmfeatures.as and/or associated machinery once we
> start building tamarin symbian

Good catch. I think it should be defined in avmhost-features.h (Flash Player file) instead of symbian-platform.h.

Comment 39

[Lars T Hansen]

Comment on attachment 418682 [details] [diff] [review]
Patch to some files in VMPI folder.

I do not like the documentation for VMPI_allocCodeMemory - writable executable memory is a really bad idea, so to specify that it's impossible to change the page protection on memory allocated using this call seems wrong.  More appropriate would be to state that VMPI_setPageProtection may have no effect, but that's probably true anyway (IMO it's a bug that the docs of that function aren't better than they are).  On some platforms it may be necessary to use VMPI_allocCodeMemory but on those platforms it may also be possible to use VMPI_setPageProtection; it seems unreasonable to prevent this from working.

I would approve it with those changes and changes to the VMPI_setPageProtection documentation, except that this API needs to be factored as a separate patch (including how it ties into the rest of the VM) and needs to be reviewed by Edwin at least.  VMPI_ changes are too important to be buried inside other concerns.

Comment 40

[Tommy Reilly]

Comment on attachment 418678 [details] [diff] [review]
Patch to some files in MMgc folder.

Why do we need MAP_TO_OS_NEW exactly and what does it have to do with getting the JIT to work?

Comment 41

[Tommy Reilly]

Comment on attachment 418680 [details] [diff] [review]
Patch to some files in platform folder.

is __GNUC__ not defined on symbian or something?  Still looking for explanation of MAP_TO_OS_NEW...

Comment 42

[Tommy Reilly]

Comment on attachment 418680 [details] [diff] [review]
Patch to some files in platform folder.

is __GNUC__ not defined on symbian or something?  Still looking for explanation of MAP_TO_OS_NEW...

Comment 43

[Samuli Kekki]

(In reply to comment #40)
> (From update of attachment 418678 [details] [diff] [review])
> Why do we need MAP_TO_OS_NEW exactly and what does it have to do with getting
> the JIT to work?

Hi Thomas. If MMGC_OVERRIDE_GLOBAL_NEW is defined MMgc wants to override global new operator. You can not do that on Symbian. MAP_TO_OS_NEW is to prevent MMgc from overriding global new. It is not required if MMGC_OVERRIDE_GLOBAL_NEW is not defined.

It is not related to JIT, just to make avm work at all.

Comment 44

[Samuli Kekki]

(In reply to comment #42)
> (From update of attachment 418680 [details] [diff] [review])
> is __GNUC__ not defined on symbian or something?  Still looking for explanation
> of MAP_TO_OS_NEW...

You are right, gcc is not used in Symbian tool chain.

Comment 45

[Samuli Kekki]

(In reply to comment #43)
> (In reply to comment #40)
> > (From update of attachment 418678 [details] [diff] [review] [details])
> > Why do we need MAP_TO_OS_NEW exactly and what does it have to do with getting
> > the JIT to work?
> 
> Hi Thomas. If MMGC_OVERRIDE_GLOBAL_NEW is defined MMgc wants to override global
> new operator. You can not do that on Symbian. MAP_TO_OS_NEW is to prevent MMgc
> from overriding global new. It is not required if MMGC_OVERRIDE_GLOBAL_NEW is
> not defined.
> 
> It is not related to JIT, just to make avm work at all.

Please let me know if there is a more elegant way of doing this. I'm always happy to clean up the code.

Comment 46

[Lars T Hansen]

(In reply to comment #43)
> (In reply to comment #40)
> > (From update of attachment 418678 [details] [diff] [review] [details])
> > Why do we need MAP_TO_OS_NEW exactly and what does it have to do with getting
> > the JIT to work?
> 
> Hi Thomas. If MMGC_OVERRIDE_GLOBAL_NEW is defined MMgc wants to override global
> new operator. You can not do that on Symbian. MAP_TO_OS_NEW is to prevent MMgc
> from overriding global new. It is not required if MMGC_OVERRIDE_GLOBAL_NEW is
> not defined.
> 
> It is not related to JIT, just to make avm work at all.

If that's the case, then would not the simpler solution be

#ifdef MMGC_OVERRIDE_GLOBAL_NEW
    #error "configuration not supported"
#endif

in symbian-platform.h?

(There's been some discussion about whether MMGC_OVERRIDE_GLOBAL_NEW is a good idea, and whether it's needed now that we have the memory allocation macros.  I don't think that discussion is finished yet.)

Comment 47

[Samuli Kekki]

Until very recently the memory allocation macros were not in place in Symbian build so I had to make the "old" code path to work also. But yes, most likely supporting MMGC_OVERRIDE_GLOBAL_NEW is not needed anymore on Symbian either, unless you want to compare the behavior between the two builds (memory macros on and off).

Comment 48

[Samuli Kekki]

To summarise this review, I should
* remove MAP_TO_OS_NEW feature as Tom suggested
* change comments in VMPI.h as Lars suggested
* create a new patch that contains the new function in VMPI API and also all vm files that uses that API for Edwin's review

Then it's good to go?

Comment 49

[Samuli Kekki]

(In reply to comment #34)
> (From update of attachment 418679 [details] [diff] [review])
> it looks like we've made the #if more restrictive.  What will be the case if
> ARMCC is not defined?

Hi Rick, do you think this change is OK? Originally you submitted Review-. Then I replied to your comment.

Comment 50

[Rick Reitmaier]

I see 4 patches that have been -'d, which means that once you've addressed the issues you'll need to re-submit the patches and have them reviewed by those that -'d or are still pending.

Comment 51

[Samuli Kekki]

Attachment: Makes Symbian to use the new VMPI API for allocating code memory

Comment 52

[Samuli Kekki]

Attachment: Changes for handling JIT memory on Symbian plus a compilation fix

Comment 53

[Samuli Kekki]

Attachment: Make non-mmgc-macro build not supported on Symbian

Comment 54

[Samuli Kekki]

Attachment: Adds new code memory allocation API that is used only on Symbian for now.

Comment 55

[Samuli Kekki]

(In reply to comment #50)
> I see 4 patches that have been -'d, which means that once you've addressed the
> issues you'll need to re-submit the patches and have them reviewed by those
> that -'d or are still pending.

Thanks for the tip Rick. Could you please review patch below because you -'d it originally. It's memory related so I put Thomas to review it.

https://bug536223.bugzilla.mozilla.org/attachment.cgi?id=420756

Comment 56

[Tommy Reilly]

Comment on attachment 420756 [details] [diff] [review]
Changes for handling JIT memory on Symbian plus a compilation fix 

deferring my review to edwin since this is nj code

Comment 57

[Tommy Reilly]

Comment on attachment 420759 [details] [diff] [review]
Adds new code memory allocation API that is used only on Symbian for now.

generally I'd expect to see the implementations of this new API in the same patch as the definition.  Where are they?

Comment 58

[Edwin Smith]

It seems odd to have VMPI_allocCodeMemory (an abstraction) which is only used on one platform.  If Lars and Tommy are okay with this, then so am I, but it seems odd.

Where is the patch that contains the Symbian specific implementation of VMPI_allocCodeMemory?  The only thing I see above was the patch that supported ICACHE flushing.

Comment 59

[Samuli Kekki]

Hi Edwin, it is in MMgcPortSymbian.cpp. I decided to put all memory related functionality there.

As Lars already suggested earlier, I can open a new bug/create a new patch that only has all the files that have VMPI_allocCodeMemory in them but nothing more.

https://bugzilla.mozilla.org/attachment.cgi?id=418683&action=edit

Comment 60

[Rick Reitmaier]

Attachment: symbian NJ changes - v1

Comment 61

[Rick Reitmaier]

Attachment: vmpi - v1

Comment 62

[Rick Reitmaier]

Attachment: mmgc - v1

Comment 63

[Rick Reitmaier]

Attachment: v2

Comment 64

[Samuli Kekki]

Attachment: mmgc - v2

Previous version removed some code that it was not supposed to.

Comment 65

[Samuli Kekki]

Hi Rick. I tried to make mmgc-v1 patch obsolete but got a message that I was not allowed to edit the bug.

I sent a new patch that should replace mmgc-v1.

Comment 66

[Rick Reitmaier]

Attachment: mmgc - v3

- adds comment to non virtual mem sample code and provides ifdefs around it.
- convert all tabs to spaces

Comment 67

[Rick Reitmaier]

Now that bug 545295 has landed, can we proceed landing these changes?

Comment 68

[Samuli Kekki]

SymbianPortUtils.cpp was changed by bug 545295, you may not be able to land that patch as is.

Comment 69

[Samuli Kekki]

The milestone for this bug can be set "Future".

Comment 70

[Rick Reitmaier]

Marking fixed, lets open a new bug with any remaining specific changes needed.