Last Comment Bug 648862 - nanojit: Remove WINCE code (split from bug 647389)
: nanojit: Remove WINCE code (split from bug 647389)
Status: RESOLVED FIXED
fixed-in-nanojit, fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All Windows CE
: -- normal (vote)
: mozilla6
Assigned To: Ed Morley [:emorley]
:
:
Mentors:
Depends on: 518695
Blocks: 614720 647389
  Show dependency treegraph
 
Reported: 2011-04-10 07:37 PDT by Ed Morley [:emorley]
Modified: 2011-05-26 13:34 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (based on TM rev 4ace629bb676) (7.03 KB, patch)
2011-04-10 07:52 PDT, Ed Morley [:emorley]
no flags Details | Diff | Splinter Review
Patch v1 (based on NJ rev 3b0667d8dc54) (6.94 KB, patch)
2011-04-10 21:06 PDT, Ed Morley [:emorley]
Jacob.Bramley: review+
Details | Diff | Splinter Review
Patch v2 (based on NJ rev 3b0667d8dc54) (14.29 KB, patch)
2011-04-11 08:21 PDT, Ed Morley [:emorley]
Jacob.Bramley: review+
edwsmith: review+
Details | Diff | Splinter Review
Patch v2 (14.34 KB, patch)
2011-05-11 09:08 PDT, Ed Morley [:emorley]
emorley: review+
Details | Diff | Splinter Review

Description Ed Morley [:emorley] 2011-04-10 07:37:21 PDT
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.
Comment 1 Ed Morley [:emorley] 2011-04-10 07:52:26 PDT
Created attachment 524952 [details] [diff] [review]
Patch v1 (based on TM rev 4ace629bb676)

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 2 Nicholas Nethercote [:njn] 2011-04-10 17:26:25 PDT
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 :)
Comment 3 Ed Morley [:emorley] 2011-04-10 21:03:11 PDT
(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!
Comment 4 Ed Morley [:emorley] 2011-04-10 21:06:59 PDT
Created attachment 525030 [details] [diff] [review]
Patch v1 (based on NJ rev 3b0667d8dc54)

Based off of nanojit-central rather than TM.
Comment 5 Jacob Bramley [:jbramley] 2011-04-11 02:28:41 PDT
(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 6 Jacob Bramley [:jbramley] 2011-04-11 02:45:52 PDT
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.
Comment 7 Ed Morley [:emorley] 2011-04-11 04:01:27 PDT
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).
Comment 8 Jacob Bramley [:jbramley] 2011-04-11 06:32:21 PDT
(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.)
Comment 9 Ed Morley [:emorley] 2011-04-11 08:21:45 PDT
Created attachment 525076 [details] [diff] [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. 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!
Comment 10 Nicholas Nethercote [:njn] 2011-04-11 21:46:32 PDT
(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.
Comment 11 Ed Morley [:emorley] 2011-04-19 04:04:14 PDT
(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.
Comment 12 Nicholas Nethercote [:njn] 2011-04-19 13:17:02 PDT
(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 :/
Comment 13 Ed Morley [:emorley] 2011-04-23 09:44:03 PDT
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
Comment 14 Ed Morley [:emorley] 2011-05-01 08:54:55 PDT
Ed & Jacob, ping for review please. Also comment 9 questions.

Thanks! :-)
Comment 15 Jacob Bramley [:jbramley] 2011-05-03 01:21:06 PDT
(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 16 Jacob Bramley [:jbramley] 2011-05-03 03:28:28 PDT
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.
Comment 17 Ed Morley [:emorley] 2011-05-03 03:57:12 PDT
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 :-)
Comment 18 Jacob Bramley [:jbramley] 2011-05-03 04:06:46 PDT
> > #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!
Comment 19 Ed Morley [:emorley] 2011-05-03 04:10:34 PDT
(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!
Comment 20 Ed Morley [:emorley] 2011-05-11 07:48:52 PDT
Ed Smith, ping for review please :-)

Thanks!
Comment 21 Edwin Smith 2011-05-11 08:27:34 PDT
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.
Comment 22 Ed Morley [:emorley] 2011-05-11 09:08:29 PDT
Created attachment 531649 [details] [diff] [review]
Patch v2

Only change is updated commit message with more detail + the r=jbramley,edwsmith.
Carrying forwards r+

--

Edwin thanks for the review :-)
Comment 23 Nicholas Nethercote [:njn] 2011-05-17 23:35:24 PDT
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
Comment 24 Chris Leary [:cdleary] (not checking bugmail) 2011-05-23 14:10:45 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/eae7ad67ec58

Note You need to log in before you can comment on or make changes to this bug.