Bug 647389 removed WINCE/WINMO code from most of spidermonkey. However, it was requested that some parts of the patch be broken out into separate bugs, since they covered code from upstream sources. From bug 647389 comment 37: > > - js/src/nanojit is shared code with Adobe. We need an Adobe review, > > and the patch will land upstream, not our repo. njn knows how that works. > > This means file a follow-on bug, submit the subset of the patch from the > jssrc/nanojit/ directory, and ask nnethercote for review.

Relevant parts of patch from bug 647389. Regarding this (in NativeARM.h#156): -/* winmo builds error with C2057 and C2229 on usage of First/LastRegNum as R0/D7 */ static const uint32_t FirstRegNum = 0; /* R0 */ static const uint32_t LastRegNum = 23; /* D7 */ The comment and R0/D7 changes were introduced here: https://bugzilla.mozilla.org/attachment.cgi?id=485936&action=diff I removed the comment; since it's now no longer relevant - but didn't revert the |= 0;| and |= 23;| since it matches the style used in the other platform files (http://mxr.mozilla.org/mozilla-central/ident?i=FirstRegNum). Let me know if this isn't desired. Also, the comment here: http://mxr.mozilla.org/mozilla-central/source/js/src/nanojit/NativeARM.cpp#607 ...implies that the legacy ABI was used only for WinCE. Does this mean that all of #ifdef NJ_ARM_EABI and #ifdef NJ_ARM_EABI_HARD_FLOAT can be assumed true and thus dealt with accordingly? If so, more cruft can be removed in addition to that in patch v1. Finally, I presume I'll need to rebase this off of http://hg.mozilla.org/projects/nanojit-central/ yeah? Thanks!

Comment on attachment 524952 [details] [diff] [review] Patch v1 (based on TM rev 4ace629bb676) (In reply to comment #1) > > Regarding this (in NativeARM.h#156): > -/* winmo builds error with C2057 and C2229 on usage of First/LastRegNum as > R0/D7 */ > static const uint32_t FirstRegNum = 0; /* R0 */ > static const uint32_t LastRegNum = 23; /* D7 */ > > The comment and R0/D7 changes were introduced here: > https://bugzilla.mozilla.org/attachment.cgi?id=485936&action=diff > > I removed the comment; since it's now no longer relevant - but didn't revert > the |= 0;| and |= 23;| since it matches the style used in the other platform > files (http://mxr.mozilla.org/mozilla-central/ident?i=FirstRegNum). Let me know > if this isn't desired. Keeping it as-is seems fine. > Also, the comment here: > http://mxr.mozilla.org/mozilla-central/source/js/src/nanojit/NativeARM.cpp#607 > ...implies that the legacy ABI was used only for WinCE. Does this mean that all > of #ifdef NJ_ARM_EABI and #ifdef NJ_ARM_EABI_HARD_FLOAT can be assumed true and > thus dealt with accordingly? If so, more cruft can be removed in addition to > that in patch v1. I don't know. In general, I'm the best Mozilla person to ask for reviews on Nanojit changes. But one exception is that I don't know much about the ARM back-end. The patch seems fine to me, but Jacob Bramley is the expert here, so I've asked him for review. Also, when removing support for a platform we need to see if Adobe are happy with it, so I've also asked Ed Smith for a review. Adobe generally supports more platforms than Mozilla does. > Finally, I presume I'll need to rebase this off of > http://hg.mozilla.org/projects/nanojit-central/ yeah? That would be ideal. I'm in charge of doing merges from nanojit-central to tracemonkey, and there are few enough of them and I merge regularly enough that rebasing is usually a no-op. See https://developer.mozilla.org/en/NanojitMerge for full details of how nanojit patches work. Kudos for getting this far, there's a lot to learn when starting working on Firefox and the change you chose is one that involved having to learn about a lot of different pieces :)

(In reply to comment #2) > I don't know. In general, I'm the best Mozilla person to ask for reviews on > Nanojit changes. But one exception is that I don't know much about the ARM > back-end. The patch seems fine to me, but Jacob Bramley is the expert here, so > I've asked him for review. Thanks :-) > Also, when removing support for a platform we need to see if Adobe are happy > with it, so I've also asked Ed Smith for a review. Adobe generally supports > more platforms than Mozilla does. Makes sense. Will understand if Adobe needs the WinCE code and so this ends up being WONTFIXED; this patch is fairly small compared to those in bug 647389 and dependants, so didn't take long compared to the rest. > Kudos for getting this far, there's a lot to learn when starting working on > Firefox and the change you chose is one that involved having to learn about a > lot of different pieces :) Yeah true - I learnt a fair amount in bug 647389. Note to self: When embarking on a patch that touches 40 files in js/src/, make sure you base it on TM not m-c, unless you like sifting through .rej files!

Based off of nanojit-central rather than TM.

(In reply to comment #1) > Also, the comment here: > http://mxr.mozilla.org/mozilla-central/source/js/src/nanojit/NativeARM.cpp#607 > ...implies that the legacy ABI was used only for WinCE. Does this mean > that all of #ifdef NJ_ARM_EABI and #ifdef NJ_ARM_EABI_HARD_FLOAT can > be assumed true and thus dealt with accordingly? If so, more cruft can > be removed in addition to that in patch v1. The legacy ABI stuff was purely for WinCE, so you can assume that NJ_ARM_EABI is true. Hard-float is a variant of EABI, and is starting to be used by some Linux distributions. We need to support both, I'm afraid, so we can't assume anything about NJ_ARM_EABI_HARD_FLOAT. Background information: The hard-float EABI variant puts floating-point arguments in VFP registers. The soft-float EABI variant puts them in general-purpose registers. Hard-float is faster, but breaks ABI compatibility with older processors that didn't have floating-point hardware. (In reply to comment #2) > Also, when removing support for a platform we need to see if Adobe are > happy with it, so I've also asked Ed Smith for a review. Adobe > generally supports more platforms than Mozilla does. Adobe have asked to keep WinCE support in the past. That was a while ago, though, so they may no longer need it.

Comment on attachment 525030 [details] [diff] [review] Patch v1 (based on NJ rev 3b0667d8dc54) > } else { > // Indirect call: we assign the address arg to LR > if (ARM_ARCH_AT_LEAST(5)) { >-#ifndef UNDER_CE > // workaround for msft device emulator bug (blx lr emulated as no-op) > underrunProtect(8); > BLX(IP); > MOV(IP, LR); >-#else >- BLX(LR); >-#endif Ah, interesting. The #ifndef was actually the wrong way around. The correct code (for everything except the WinCE emulator) is "BLX(LR)". The rest looks good. I didn't check for other code that could potentially be removed (like the !NJ_ARM_EABI stuff). There'll be a bit of that in NativeARM.cpp.

Thanks for the speedy review :-) (In reply to comment #5) > The legacy ABI stuff was purely for WinCE, so you can assume that > NJ_ARM_EABI is true. > > Hard-float is a variant of EABI, and is starting to be used by some > Linux distributions. We need to support both, I'm afraid, so we can't > assume anything about NJ_ARM_EABI_HARD_FLOAT. Great - thanks for the clarification. (In reply to comment #6) > Ah, interesting. The #ifndef was actually the wrong way around. The > correct code (for everything except the WinCE emulator) is "BLX(LR)". Yeah I must admit the comment (and the fact that the workaround path was shorter) did make me re-read the #ifndef a couple of times; meant to ask about that, thanks for spotting it. > The rest looks good. I didn't check for other code that could > potentially be removed (like the !NJ_ARM_EABI stuff). There'll be a bit > of that in NativeARM.cpp. Out of interest, what's the etiquette for revised patches/reviews in cases like these? ie: should I fix the nits/!NJ_ARM_EABI, attach a full new patch and r? it all ; or should I attach an interdiff (or part 2 patch) to save re-review of the entire thing? (This is of course presuming Adobe are ok with WinCE removal; if not then presumably we can still at least fix the backwards #ifndef in NativeARM.cpp in a follow-up).

(In reply to comment #7) > Out of interest, what's the etiquette for revised patches/reviews in > cases like these? ie: should I fix the nits/!NJ_ARM_EABI, attach a > full new patch and r? it all ; or should I attach an interdiff (or > part 2 patch) to save re-review of the entire thing? In general, minor tweaks noted by the reviewer are Ok if r+ is given. In this case, you could swap around the BLX case and consider that Ok, for example. (You still need an r+ from Edwin, of course.) For removing the NJ_ARM_EABI stuff, the best way to do it varies from bug to bug, and with personal preference. For large, complex bugs, typically those adding big features, splitting patches into separate parts can be a good idea. (This is really handy for contributing whole Mercurial-Queue stacks.) In this specific case, however, I'd just merge the patches into one. In most cases, it's not a good idea to attach patches that aren't actually in patch format, mostly because that's what people expect to find. It's easiest if you just attach a new patch, and mark the old one as obsolete. This makes the last-known-good patch obvious. > (This is of course presuming Adobe are ok with WinCE removal; if not > then presumably we can still at least fix the backwards #ifndef in > NativeARM.cpp in a follow-up). Yep. Actually I thought I'd seen it fixed a while ago so I was surprised to see it there. In any case, we should fix it regardless of WinCE. (It's only an efficiency issue; there's no potential stability problem here.)

Fixes backwards #ifndef ; assumes NJ_ARM_EABI true so removes additional code/adjusts comments appropriately. Couple of questions: 1) The use of both NJ_ARM_EABI_HARD_FLOAT and ARM_EABI_HARD seems redundant. The latter is only used in a couple of places: http://mxr.mozilla.org/mozilla-central/search?string=ARM_EABI_HARD+&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central ...is there a reason why both are not combined? 2) Presuming the answer to the above is that they both have to be kept; this existing section in NativeARM.h can presumably still be optimised though? > #ifdef __ARM_PCS_VFP > # define NJ_ARM_EABI_HARD_FLOAT 1 > #endif > > #ifdef NJ_ARM_EABI_HARD_FLOAT > # define ARM_EABI_HARD true > #else > # define ARM_EABI_HARD false > #endif Thanks!

(In reply to comment #7) > > Out of interest, what's the etiquette for revised patches/reviews in cases like > these? ie: should I fix the nits/!NJ_ARM_EABI, attach a full new patch and r? > it all ; or should I attach an interdiff (or part 2 patch) to save re-review of > the entire thing? Bugzilla has a built-in interdiff function. See the buttons at the top-right of the "diff" view (ie. when you click on the "diff" link for a patch attachment). Unfortunately it fails sometimes. Even weirder, it seems to fail more often than the interdiff I have on my Linux box. As for whether a new patch needs to be attached, I'd lean towards "yes" because you're new at this, unless the resulting changes are only very minor.

(In reply to comment #8) > Yep. Actually I thought I'd seen it fixed a while ago so I was surprised > to see it there. In any case, we should fix it regardless of WinCE. Appears it was fixed in bug 586262, but then bug 552624 accidentally changed the ifdef to an ifndef: http://hg.mozilla.org/projects/nanojit-central/diff/6b09fdb0cbc6/nanojit/NativeARM.cpp#l1.152 (In reply to comment #10) > Bugzilla has a built-in interdiff function. See the buttons at the top-right > of the "diff" view (ie. when you click on the "diff" link for a patch > attachment). Unfortunately it fails sometimes. Even weirder, it seems to fail > more often than the interdiff I have on my Linux box. Yeah the few times I've used it the results have been completely off, so I had kind of dismissed it as being an option.

(In reply to comment #11) > > Yeah the few times I've used it the results have been completely off, so I had > kind of dismissed it as being an option. It works for simple interdiffs, eg. if there's no overlap between the areas being modified :/

Ed Smith: Do you know if Adobe need/want to support WinCE any more? Btw passed try (after repeating a few or the tests for intermittent oranges): http://dev.philringnalda.com/tbpl/?tree=Try&rev=cca063ae0577

Ed & Jacob, ping for review please. Also comment 9 questions. Thanks! :-)

(In reply to comment #9) > Created attachment 525076 [details] [diff] [review] [review] > Patch v2 (based on NJ rev 3b0667d8dc54) > > Fixes backwards #ifndef ; assumes NJ_ARM_EABI true so removes additional > code/adjusts comments appropriately. > > Couple of questions: > > 1) The use of both NJ_ARM_EABI_HARD_FLOAT and ARM_EABI_HARD seems redundant. I think ARM_EABI_HARD was added to match the style used by ARM_VFP and ARM_ARCH_AT_LEAST. I wouldn't object in principle to NJ_ARM_EABI_HARD_FLOAT and ARM_EABI_HARD being combined, but it might be more consistent (with ARM_VFP) to use ARM_EABI_HARD exclusively in NativeARM.cpp and reserve NJ_ARM_EABI_HARD_FLOAT for feature-selection purposes. (I don't know if it's already used in that way, but it looks like it could be.) Tero commented on it here: https://bugzilla.mozilla.org/show_bug.cgi?id=602834#c15 > 2) Presuming the answer to the above is that they both have to be kept; this > existing section in NativeARM.h can presumably still be optimised though? > > #ifdef __ARM_PCS_VFP > > # define NJ_ARM_EABI_HARD_FLOAT 1 > > #endif > > > > #ifdef NJ_ARM_EABI_HARD_FLOAT > > # define ARM_EABI_HARD true > > #else > > # define ARM_EABI_HARD false > > #endif If you need to keep both, how would you optimize that? I am assuming that NJ_ARM_EABI_HARD_FLOAT could be set as a build flag, or something like that.

Comment on attachment 525076 [details] [diff] [review] Patch v2 (based on NJ rev 3b0667d8dc54) Review of attachment 525076 [details] [diff] [review]: Yep, that looks like a good, clean patch.

Thanks for the review :-) (In reply to comment #15) > I think ARM_EABI_HARD was added to match the style used by ARM_VFP and > ARM_ARCH_AT_LEAST. I wouldn't object in principle to > NJ_ARM_EABI_HARD_FLOAT and ARM_EABI_HARD being combined, but it might be > more consistent (with ARM_VFP) to use ARM_EABI_HARD exclusively in > NativeARM.cpp and reserve NJ_ARM_EABI_HARD_FLOAT for feature-selection > purposes. (I don't know if it's already used in that way, but it looks > like it could be.) > > Tero commented on it here: > https://bugzilla.mozilla.org/show_bug.cgi?id=602834#c15 Ah, I'd missed Tero's comment there - thanks for the link. The current implementation only uses ARM_EABI_HARD twice (and in instances that could be rewritten as #ifndef NJ_ARM_EABI_HARD_FLOAT): http://mxr.mozilla.org/mozilla-central/source/js/src/nanojit/NativeARM.cpp#875 http://mxr.mozilla.org/mozilla-central/source/js/src/nanojit/NativeARM.cpp#2872 However, happy to do whatever you think best in this case (and besides it's slightly out of scope for this bug perhaps) - just wanted to point it out in case the inconsistency was unintentional (or at least unknown). > > 2) Presuming the answer to the above is that they both have to be kept; this > > existing section in NativeARM.h can presumably still be optimised though? > > > #ifdef __ARM_PCS_VFP > > > # define NJ_ARM_EABI_HARD_FLOAT 1 > > > #endif > > > > > > #ifdef NJ_ARM_EABI_HARD_FLOAT > > > # define ARM_EABI_HARD true > > > #else > > > # define ARM_EABI_HARD false > > > #endif > > If you need to keep both, how would you optimize that? I am assuming > that NJ_ARM_EABI_HARD_FLOAT could be set as a build flag, or something > like that. I meant just rewriting as: (slightly nit-picky on my part I guess) > #ifdef __ARM_PCS_VFP > # define NJ_ARM_EABI_HARD_FLOAT 1 > # define ARM_EABI_HARD true > #else > # define ARM_EABI_HARD false > #endif Thanks again :-)

> > #ifdef __ARM_PCS_VFP > > # define NJ_ARM_EABI_HARD_FLOAT 1 > > # define ARM_EABI_HARD true > > #else > > # define ARM_EABI_HARD false > > #endif Unfortunately, that doesn't work if NJ_ARM_EABI_HARD_FLOAT is overridden on the command line. That could occur, for example, if GCC doesn't define __ARM_PCS_VFP properly. I think some versions have that problem, but I don't remember the details. > Thanks again :-) No problem!

(In reply to comment #18) > Unfortunately, that doesn't work if NJ_ARM_EABI_HARD_FLOAT is overridden on the > command line. Ah indeed, good point!

Ed Smith, ping for review please :-) Thanks!

Comment on attachment 525076 [details] [diff] [review] Patch v2 (based on NJ rev 3b0667d8dc54) I've been informed that we will need support for windows mobile in the future, but only new versions which will not use the legacy ABI. However, its not clear when that support will be required, and the non-ABI bits of code in this patch (icache flushing, and an emulator workaround) are easy to recover when/if we need them again in the future.

Only change is updated commit message with more detail + the r=jbramley,edwsmith. Carrying forwards r+ -- Edwin thanks for the review :-)

Ed, I love that you have several dozen patches in Firefox now and almost all of them have a log message that starts with "Remove". Clean-ups are valuable, and a great way to get into the code. http://hg.mozilla.org/projects/nanojit-central/rev/37586d0b0785 http://hg.mozilla.org/tracemonkey/rev/eae7ad67ec58

cdleary-bot mozilla-central merge info: http://hg.mozilla.org/mozilla-central/rev/eae7ad67ec58