647389 - Remove WINCE stuff from Spidermonkey

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Paul Biggar]

We haven't supported WINCE in a long time, and never will again. Remove all uses of WINCE from Spidermonkey.

Comment 1

[Wesley W. Garland]

Why not? It's possible that we will want to release products for Windows Phone some day. No?

Comment 2

[Jeff Walden [:Waldo]]

I agree that "never will again" is a bit strong.  I can think of other code improvements I'd rather do, or see done, before this one.

Comment 3

[Paul Biggar]

(In reply to comment #2)
> I agree that "never will again" is a bit strong.  I can think of other code
> improvements I'd rather do, or see done, before this one.

To my knowledge, windows phones dont use WinCE anymore, which is why I said never again. Perhaps I'm mistaken.

File those improvements, and please mark them [good first bug] if appropriate.

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

If/when Windows Phone 7 publishes an NDK for the platform, and if/when we decide to port to it, people should add whatever support is required by that new environment.  In the meantime we should strip it out, IMO.

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

(Windows Phone 7 has the WinCE kernel -- I think 6.5? -- and the Windows 7 Compact profile for userspace, if I am not mistaken.  A pretty different beast.)

Comment 6

[Ed Morley [:emorley]]

(In reply to comment #2)
> I agree that "never will again" is a bit strong.  I can think of other code
> improvements I'd rather do, or see done, before this one.

As a newcomer to the gecko codebase (and as someone who is still getting to grips with C), these kind of bugs are the only ones I can tackle at this stage - but at least mean I can get familiar with the codebase, using hg/exporting correctly formatted patches and the review process/checkin process.

Many of the bugs marked [good first bug] are out of date or else are actually not beginner level bugs, but those that owners do not wish to do themselves. As such after spotting this potentially easy bug in the [good first bug] list, I was hoping that I would be able to take it on. 

However I'm struggling to tell from the above whether this change is wanted or not. Before I start a patch that could end up bit-rotting for the foreseeable future (given that the changes would touch 34 files), can someone confirm for sure whether a patch is going to be accepted please.

Comment 7

[Paul Biggar]

(In reply to comment #6)
> (In reply to comment #2)
> > I agree that "never will again" is a bit strong.  I can think of other code
> > improvements I'd rather do, or see done, before this one.

I think (and I hope I'm not putting words in Jeff's mouth) that he means it's not the best use of time for someone already familiar with the codebase.


> As a newcomer to the gecko codebase (and as someone who is still getting to
> grips with C), these kind of bugs are the only ones I can tackle at this stage
> - but at least mean I can get familiar with the codebase, using hg/exporting
> correctly formatted patches and the review process/checkin process.

Excellent! That's why I wrote this one up.


> Many of the bugs marked [good first bug] are out of date or else are actually

We were thinking of deleting all the [good first bugs] and starting again. Thoughts?
 
> However I'm struggling to tell from the above whether this change is wanted or
> not. Before I start a patch that could end up bit-rotting for the foreseeable
> future (given that the changes would touch 34 files), can someone confirm for
> sure whether a patch is going to be accepted please.

This change is wanted - and there are dozens more like it (preprocessor symbols which, when turned either on or off, wont build, and won't be resurrected either). Assuming no issues come up, I'd say there's a 99% chance of this getting in. I'll also personally find you a speedy review and handle the checkin.

Comment 8

[Ed Morley [:emorley]]

Hi Paul, thanks for your reply :-)

> I think (and I hope I'm not putting words in Jeff's mouth) that he means it's
> not the best use of time for someone already familiar with the codebase.

Great - thought that was possibly the case; just wanted to be sure (especially given the comments in bug 614720 too) before embarking on a patch.

> We were thinking of deleting all the [good first bugs] and starting again.
> Thoughts?

I'll send the reply to this off-bug, as it ended up being long enough to take this considerably off topic! :-)

==

Regarding this bug, I believe I've almost finished the patch, I just have a few questions on some stragglers:

1) Can these three lines be removed? ie: It would appear to be build error fixes specific to WINMO, but wanted to check...
http://mxr.mozilla.org/mozilla-central/source/js/src/nanojit/NativeARM.h#156

2) The comment on this line refers to "wince and some others". Do you know what "others" refers to so the comment can be updated? (Or even the line removed if others=nothing supported; and so now unnecessary):
http://mxr.mozilla.org/mozilla-central/source/js/src/ctypes/libffi/src/dlmalloc.c#459

3) 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?

4) I presume the four lines here can also be removed?
http://mxr.mozilla.org/mozilla-central/source/js/src/jsinttypes.h#127

5) I can't tell if this comment refers to the WINCE ifdef that I've removed, or the subsequent #include <atlbase.h> & #include "oaidl.h". Comment indentation would indicate the latter, but yet it refers to "include" singular rather than plural; which makes me wonder if it was actually referring to the WINCE section instead:
http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/xpcprivate.h#140

Sorry for all the questions / thanks!

Comment 9

[Paul Biggar]

(In reply to comment #8)
> 1) Can these three lines be removed? ie: It would appear to be build error
> fixes specific to WINMO, but wanted to check...
> http://mxr.mozilla.org/mozilla-central/source/js/src/nanojit/NativeARM.h#156

For some of these things, it's going to be suck-it-and-see. Before we push your patch, you'll need to run it on tryserver to see if it actually breaks anything. Apply for level 1 access to get tryserver access, and CC me on the bug).



> 2) The comment on this line refers to "wince and some others". Do you know what
> "others" refers to so the comment can be updated? (Or even the line removed if
> others=nothing supported; and so now unnecessary):
> http://mxr.mozilla.org/mozilla-central/source/js/src/ctypes/libffi/src/dlmalloc.c#459

I don't honestly know. You can go either way with this and still be right. If OS/2, Solaris, etc, maintainers care, they'll file a follow-up.

 
> 3) 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?

I believe so. A good way to deal with this is to split your patch into two, with one part specifically dealing with the ABI. Then we can get a different reviewer for it.

Alternatively, leave it in and file a follow-on bug to deal with that.


> 4) I presume the four lines here can also be removed?
> http://mxr.mozilla.org/mozilla-central/source/js/src/jsinttypes.h#127

Yes (same disclaimers as above - you gotta try it).


> 5) I can't tell if this comment refers to the WINCE ifdef that I've removed, or
> the subsequent #include <atlbase.h> & #include "oaidl.h". Comment indentation
> would indicate the latter, but yet it refers to "include" singular rather than
> plural; which makes me wonder if it was actually referring to the WINCE section
> instead:
> http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/xpcprivate.h#140

Looks like the latter to me.

 
> Sorry for all the questions / thanks!

You should feel free to ask questions about anything; please do, and please don't apologize :)

Comment 10

[Ed Morley [:emorley]]

(In reply to comment #9)
> For some of these things, it's going to be suck-it-and-see. Before we push your
> patch, you'll need to run it on tryserver to see if it actually breaks
> anything. Apply for level 1 access to get tryserver access, and CC me on the
> bug).

Yeah thought as much - applied this morning in bug 648541. Will (understandably) no doubt take a while to get the committer's agreement processed, but at least the ball is rolling.

Thanks for the speedy follow up, I'll make those changes (except the ABI bits) and get a patch attached, ready for level 1 :-)

Comment 11

[Paul Biggar]

(In reply to comment #10)
 
> Thanks for the speedy follow up, I'll make those changes (except the ABI bits)
> and get a patch attached, ready for level 1 :-)

I can push that to try to unblock you.

Comment 12

[Jeff Walden [:Waldo]]

(In reply to comment #7)
> I think (and I hope I'm not putting words in Jeff's mouth) that he means it's
> not the best use of time for someone already familiar with the codebase.

Yes, exactly.  :-)

Comment 13

[Wesley W. Garland]

Dropping support for Solaris at this time would be a big mistake.

Solaris support is completely up-to-date and shipping in contrib builds on Mozilla.com. Firefox 4 runs on Solaris 10 and 11, with TM and JM for x86 and sparc CPUs. Support for Solaris is nearly 100% community-driven, with Oracle (Sun Beijing) doing the lion's share of the work. The next biggest contributor is Adobe, via sparc TM patches. 

Oracle also adds support for Sun's C++ compiler, which could be a viable alternative to GCC for codegen even on non-Sun platforms  (it supports whole-program PGO on Linux, for example).  Additionally, Oracle's patches make it possible to run SpiderMonkey on *BSD and Linux running on sparc CPUs, and we have active contributors supporting these distros.

We've seen a lot of effort poured into new-contributor engagement this week, let's not forget about contributor retention.  Ripping out Solaris support at this stage of the game would be effectively raising the middle finger at an entity that has spent significant effort contributing to the project -- a contributor that already has a hard enough time getting appropriate (IMO) attention from The Project (code reviews are extremely slow, Firefox binaries impossible to find unless you're already familiar with Moz's internal structure, etc).

As for OS/2 - I'm not an OS/2 guy, but I saw patches for OS/2 SpiderMonkey just this week for API changes that appeared after Firefox 4 branched! If the community is still trying to support it without being time-leeches, why not encourage them?  It would not be unheard of to turn dedicated-old-OS-hacker into dedicated-mozilla contributor.

So -- I suggest -- let's make this bug about removing support for WinCE and not "WinCE and some others".  If we are considering dropping support for other platforms, they should be in other bugs, and we should copy anybody who has recently (say, in the last year) contributed patches for said platform.

Comment 14

[Ed Morley [:emorley]]

Wesley, neither myself or Paul are suggesting support for those platforms should be dropped, please don't put words in our mouths. Paul's "If OS/2, Solaris, etc, maintainers care" is meaning "if they are in fact accidentally affected", not "who cares about those platforms".

What we actually said was that for the comment "wince and some others" here:
http://mxr.mozilla.org/mozilla-central/source/js/src/ctypes/libffi/src/dlmalloc.c#459
...we don't know what "others" is referring to. It's quite possible that nothing else needs it (others could be a guess or else be say referring to the numerous wince versions and platforms that have been used in the past), and as such the line can be removed just fine. Worst case scenario, it breaks a platform that try server doesn't test and can be added back in - ideally with a proper comment this time, actually indicating what platforms require it explicitly.

I've also looked up that line using HG blame; but it originated from when the file was created (http://hg.mozilla.org/mozilla-central/rev/eb97628a701b) - and the related bug doesn't explain why it was added - so doesn't really help at all. The libffi project repository doesn't go back far enough either: https://github.com/atgreen/libffi/blame/30ff28e1d8cd9ed5319f1fbe9c7cccacc8161fb3/src/dlmalloc.c

As for the patch, I'm trying to convince TortoiseHg to rebase my changes, but it first ate some of them (after hanging during a shelve :'() and now I can't figure out whether I should be using mercurial queues, shelve or the rebase extension...I guess serves me right for not just using command line hg...!

Comment 15

[Wesley W. Garland]

Hi, Ed!

Sorry -- I clearly misunderstood that comment!  Thanks for setting me straight.

As for that particular line -- you'll want to check with dwitte, but I suspect it should be left alone.

Libffi is not a Mozilla lib, but is being distributed within the moz source tree, with very minor changes; all of which IIUC are slated for inclusion by upstream libffi.  I believe WinCE support *should* be left in LibFFI, independent of what the rest of the source tree is doing, to make it easier to generate or apply upstream patches.

FWIW - I've verified that that line appears exactly as-is in libffi 3.0.8, which predates the GitHub repo.

As for your local repo: I bet you can use the command-line tools in your work directory. 

Finally, if removing WinCE accidentally breaks Solaris -- I volunteer to test and fix!

Wes

Comment 16

[Ed Morley [:emorley]]

Attachment: Patch v1

You make a good point - thanks for the feedback :-) 

I've reverted the changes that had been made to the Libffi files, to ease maintaining it against the upstream copy. (Sorry if comment 14 was a bit abrupt - after having had TortoiseHg hang mid operation and this break my local repository and leave me with a load of rejected patch chunks that had to be manually sorted through, I was mildly unimpressed to say the least!)

(In reply to comment #8)
> 1) Can these three lines be removed? ie: It would appear to be build error
> fixes specific to WINMO, but wanted to check...
> http://mxr.mozilla.org/mozilla-central/source/js/src/nanojit/NativeARM.h#156

Scratch that, those lines are needed; but the comment can be removed. (The comment originated from the change here https://bugzilla.mozilla.org/attachment.cgi?id=485936&action=diff). I decided not to revert the R0/D7 changes in that diff as well, since the new style matches that currently in the other platform files (http://mxr.mozilla.org/mozilla-central/ident?i=FirstRegNum).

> 2) The comment on this line refers to "wince and some others". 

Have left, per comment 15.

> 4) I presume the four lines here can also be removed?

Have removed; will see what try says. (Committer's agreement has been acknowledged, just waiting on IT now).


Note to self: Remember to wontfix bug 515383 (was referred to by one of the now removed comments) + deal with legacy ABI in other bug.

Comment 17

[Ed Morley [:emorley]]

Attachment: Patch v2

Nit: Adjust indentation for js/src/configure.in section where case esac was removed.

Comment 18

[Ed Morley [:emorley]]

Attachment: Patch v2 (updated to 352ebc13e546)

Updated to tip.

I don't know if I'm just missing the obvious; but there doesn't seem a sensible way to merge/rebase mercurial queues. I've tried the simple qpop; pull; qpush approach, but that just gave lots of .rej files that couldn't even be used with the three way editor. I also tried doing a straight merge whilst the patch was qpushed, but that seemed to break mq. 

I finally resorted to the "qpush -a; qsave -e -c; pull; update -C tip; qpush -m -a" dance....but surely there must be an easier way?!?

Comment 19

[Paul Biggar]

I think you're right about mercurial queues - it's a pain in the hole, but I don't know a better way. I do |qpop -a|, |pull|, |update|, |qpush -a|, and then manually merge the .rej files using vimdiff. However, I believe there may be a better way using kdiff3 and specifying [merge-tools] in .hgrc, but I'm not familiar.

I'll push this to tryserver and find you some review for you.

Comment 20

[Wesley W. Garland]

IME, small-time contributors (specifically, those working on a single bug a time and that do not push to any repository except the try servers) can function efficiently without mercurial queues at all.

Just patch a reasonably current version of the tree, upload the hg diff or export, and don't worry about rebasing your patch unless it rots in somebody's review queue for months.

That said, there's nothing wrong with using mq to get the hang of it. I should probably do that. ;)

Comment 21

[Paul Biggar]

I tried to apply this, but I think it's based on mozilla-central?

JS stuff is always done on the tracemonkey tree (http://hg.mozilla.org/tracemonkey). Sorry for not checking that you knew this sooner - we need to document this stuff better.

Comment 22

[Paul Biggar]

(In reply to comment #20) 
> Just patch a reasonably current version of the tree, upload the hg diff or
> export, and don't worry about rebasing your patch unless it rots in somebody's
> review queue for months.

I think a patch like this one will rot very quickly - it touches so many places. I'm going to try to get a fast review so it doesn't rot.

Comment 23

[Ed Morley [:emorley]]

(In reply to comment #21)
> I tried to apply this, but I think it's based on mozilla-central?
> 
> JS stuff is always done on the tracemonkey tree
> (http://hg.mozilla.org/tracemonkey). Sorry for not checking that you knew this
> sooner - we need to document this stuff better.

Ah yes, whoops. I knew about TM but thought it was more used for major changes like methodjit (having seen merges both ways), my bad.

Ideas for quickest way to rebase off of tracemonkey?

Comment 24

[Paul Biggar]

hg export and hg import? Or maybe change .hg/hgrc and then rebase?

At breakfast, some folks mentioned a better way to rebase mq. Try `hg pull --rebase` (I've never tried it, save your data first).

Comment 25

[Ed Morley [:emorley]]

Attachment: Patch v3 (rebased on TM 48489a602029)

Rebased on TM tip. Won't make that mistake again!

Thanks for the suggestions :-)

Comment 26

[Josh Matthews [:jdm]]

`hg pull --rebase` is currently broken and dangerous.  Please don't use it until we know that the various problems that have plagued us in the past (random inverse versions of prior changesets suddenly appearing in outgoing pushes, IIRC) have been fixed.

Comment 27

[Wesley W. Garland]

Comment on attachment 524873 [details] [diff] [review]
Patch v3 (rebased on TM 48489a602029)

My experience with hg rebase matches jdm's; because of it, I have a project with more heads than an army of Hydras.

Casual observation - ifeq (,$(filter-out WINNT,$(OS_ARCH))) and ifeq ($(OS_ARCH),WINNT) are equivalent expressions.

Comment 28

[Ed Morley [:emorley]]

I wasn't sure how much I should rearrange existing code, given that there were already several variations used throughout:

ifeq (,$(filter-out XXXXX,$(OS_ARCH)))
ifneq (,$(filter XXXXX,$(OS_ARCH)))
ifeq ($(OS_ARCH),XXXXX)

...which are largely identical if XXXXX is only one platform. However, I'm presuming that for some of the platforms/instances in the code, |filter| is deliberately being used, since it will still match if version numbers are appended (unlike |filter-out| or a direct |ifeq (x,y)|.

I take it that it's a case of: "don't start mass adjusting code you aren't directly touching; but if you can optimise lines already being changed, then do so" yeah?

Comment 29

[Paul Biggar]

I'd be wary about adding too much. If there is cleanup of existing code, you should feel free to do it as a follow-on (or simple file a follow-on about it). Don't let someone else's mess hold you up.

Am just checking the code so I can push to tryserver.

Comment 30

[Paul Biggar]

You've hit yet another nasty secret we need to document.

So the build system in js/src is shared with the main build system, so we can't make the changes to it independently of the build system. (You can check this by running |make check-sync-dirs|. So I think you should split this up a bit:

- any changes in js/src/config/, js/src/build or js/src/configure.in should be split into a follow-on bug. The same changes should be made to the same files in the root dir, and you should check with :ted to make sure you're on the right track (ask for an f? with the existing diff on those files in js/src).

- the remaining changes should stay here. I've pushed them to tryserver (http://tbpl.mozilla.org/?tree=MozillaTry&rev=ce732e247c81) and will attach now.

Comment 31

[Paul Biggar]

Attachment: Minus build stuff

Comment 32

[Ed Morley [:emorley]]

(In reply to comment #30)
> So the build system in js/src is shared with the main build system, so we can't
> make the changes to it independently of the build system. 

I'll obviously file a follow up bug for this, but just to clarify, where should I be making the changes? mozilla-central, tracemonkey or http://hg.mozilla.org/projects/build-system/ ? (I've seen several merges from the latter into m-c recently)

Also, the files /mozilla-central/configure.in & /mozilla-central/js/src/configure.in are significantly different at present, is this intentional? I'm sure there is a reason for it, so this is more for my curiosity/learning; but why do the wince changes need to hit both configure.in files simultaneously, if the files are already non-identical? Is it to reduce the risk of them deviating even further apart due to the root file changes follow-up being forgotten?

> - any changes in js/src/config/, js/src/build or js/src/configure.in should be
> split into a follow-on bug. 

For my reference as much as anything, no changes were made to js/src/config/ or js/src/build; it's just js/src/configure.in I'll need to fix later.

Thanks for pushing to try! :-)

Comment 33

[Paul Biggar]

(In reply to comment #32)
> (In reply to comment #30)
> > So the build system in js/src is shared with the main build system, so we can't
> > make the changes to it independently of the build system. 
> 
> I'll obviously file a follow up bug for this, but just to clarify, where should
> I be making the changes? mozilla-central, tracemonkey or
> http://hg.mozilla.org/projects/build-system/ ? (I've seen several merges from
> the latter into m-c recently)
>
> Also, the files /mozilla-central/configure.in &
> /mozilla-central/js/src/configure.in are significantly different at present, is
> this intentional? I'm sure there is a reason for it, so this is more for my
> curiosity/learning; but why do the wince changes need to hit both configure.in
> files simultaneously, if the files are already non-identical? Is it to reduce
> the risk of them deviating even further apart due to the root file changes
> follow-up being forgotten?

For this, you should ask :ted. Yes, we want to avoid deviation (and I guess avoid it getting worse).


> > - any changes in js/src/config/, js/src/build or js/src/configure.in should be
> > split into a follow-on bug. 
> 
> For my reference as much as anything, no changes were made to js/src/config/ or
> js/src/build; it's just js/src/configure.in I'll need to fix later.

I saw maybe 5 changes to the contents of js/src/config/ and js/src/build/, and |make check-sync-dirs| failed.

Comment 34

[Ed Morley [:emorley]]

(In reply to comment #33)
> I saw maybe 5 changes to the contents of js/src/config/ and js/src/build/, and
> |make check-sync-dirs| failed.

Ah sorry...you are correct. I must have been looking at the wrong attached patch. I'll make sure those changes are covered in the build system follow-up bug too. Thanks!

Comment 35

[Ed Morley [:emorley]]

(In reply to comment #34)
> I must have been looking at the wrong attached patch. 

Actually I looked at the interdiff generated by bugzilla; didn't realise the warning also meant that it could completely omit files?

Comment 36

[Jason Orendorff [:jorendorff]]

Comment on attachment 524887 [details] [diff] [review]
Minus build stuff

r=me for everything EXCEPT:

  - 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.

  - js/src/assembler is shared or stolen code too, but I don't know the
    policy there. Maybe we've forked it. Paul, ask dmandelin or dvander?

As Graydon says, delete is my favorite key. Thanks, Ed! Paul will take this one from here, I think.

Comment 37

[Paul Biggar]

Landed! Great work Ed!

http://hg.mozilla.org/tracemonkey/rev/4ace629bb676


(In reply to comment #36)
> r=me for everything EXCEPT:
> 
>   - 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.

 
>   - js/src/assembler is shared or stolen code too, but I don't know the
>     policy there. Maybe we've forked it. Paul, ask dmandelin or dvander?

File a follow-on, submit the subset of the patch in js/src/assembler/ and ask cdleary for a review (This code originally landed in cdleary's Yarr patch, and it's unclear if this code came from upstream, in which case we won't delete it).

Thanks again, Ed!

Comment 38

[Ed Morley [:emorley]]

(In reply to comment #27)
> Casual observation - ifeq (,$(filter-out WINNT,$(OS_ARCH))) and ifeq
> ($(OS_ARCH),WINNT) are equivalent expressions.

Filed bug 648876.

(In reply to comment #30)
> any changes in js/src/config/, js/src/build or js/src/configure.in should be
> split into a follow-on bug. 

Bug 648866.

(In reply to comment #36)
>   - 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.

Bug 648862 (also covers the legacy ABI question from comment 8).

>   - js/src/assembler is shared or stolen code too, but I don't know the
>     policy there. Maybe we've forked it. Paul, ask dmandelin or dvander?

Bug 648865.

> As Graydon says, delete is my favorite key. Thanks, Ed! 

You're welcome :-) The quote: "Perfection is achieved, not when there is nothing more to add, but when there is nothing left to take away" seems fitting in this instance hehe.

If there are any other cruft-removal type bugs like this that spring to mind, let me know. Trivial bugs like this one are ideal for me; since the sheer act of having to wade through the codebase means I can at least learn a fair amount about it's layout, despite my currently minimal c/c++ experience.

Comment 39

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/4ace629bb676