Closed Bug 965447 Opened 8 years ago Closed 7 years ago

Add Linux32 debug SpiderMonkey ARM simulator build

Categories

(Release Engineering :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jandem, Assigned: sfink)

References

Details

Attachments

(3 files)

I'd like to add a new shell build to TBPL, just like the Linux32 debug one but with an extra configure flag: --enable-arm-simulator

This flag is very useful for testing our ARM JIT backend, but I fear it will bitrot quickly if we don't have a build and jit-tests on TBPL.

Note that it has to be a 32-bit build, see [0] for more info.

[0] https://lists.mozilla.org/pipermail/dev-tech-js-engine-internals/2014-January/001582.html
Steve, I'm told you can help me with this :)
Flags: needinfo?(sphink)
We now have a 32-bit ARM simulator that needs to stay alive.
Attachment #8373709 - Flags: review?(bhearsum)
Assignee: nobody → sphink
Status: NEW → ASSIGNED
This is gross and awful. It special-cases arm-sim to be a 32-bit build.
Attachment #8373713 - Flags: review?(terrence)
Flags: needinfo?(sphink)
Comment on attachment 8373713 [details] [diff] [review]
Add an arm-sim variant script and modify spidermonkey.sh to handle 32-bit builds

Review of attachment 8373713 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure I'm the right reviewer for this; I'm not really sure who possibly could be the right reviewer though, so r=me.
Attachment #8373713 - Flags: review?(terrence) → review+
Depends on: 971208
Attachment #8373713 - Flags: checked-in+
Attachment #8373709 - Flags: checked-in+
Comment on attachment 8373709 [details] [diff] [review]
Add a new "arm-sim" spidermonkey variant

Review of attachment 8373709 [details] [diff] [review]:
-----------------------------------------------------------------

You checked this in before review? Very bad :).

At some point we may need to start clearing things like with some other person now that we have a firmer budget, but this should be fine for now since it's only for js/ commits.
Attachment #8373709 - Flags: review?(bhearsum) → review+
(In reply to Ben Hearsum [:bhearsum] from comment #7)
> Comment on attachment 8373709 [details] [diff] [review]
> Add a new "arm-sim" spidermonkey variant
> 
> Review of attachment 8373709 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You checked this in before review? Very bad :).

Argh! Sorry! I guess I got lost in my recent pile of patches, and incorrectly marked this one as having been reviewed. (I don't put r=anybody in the patch until it's actually gotten review. Unless I get confused about which one was reviewed.)

> At some point we may need to start clearing things like with some other
> person now that we have a firmer budget, but this should be fine for now
> since it's only for js/ commits.

Yeah, I was assuming that an r?bhearsum would also serve as a request for the additional resources. I really do try to be cognizant of the machine time I'm sucking up. My apologies again.

But in general, it does seem like it'd be good to have an rr? (resource review request) for things like this, at least so it's easier to keep track of the new stuff.

Now what patch *did* you review that I mistook for this one...?
Depends on: 971881
Live in production.
Should these be running on Try? I just did a "try: -b do -p all,linux64-asan,linux64-sh-haz,win64 -u all -t all" run and the SM(arm) build isn't running. All the others are.
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #10)
> Should these be running on Try? I just did a "try: -b do -p
> all,linux64-asan,linux64-sh-haz,win64 -u all -t all" run and the SM(arm)
> build isn't running. All the others are.

Those are off by default. Mostly because I always start with things off by default, and I sort of pushed this one live prematurely. Would you like it on by default? From https://tbpl.mozilla.org/?tree=Try&rev=03c4a63d7cfc it looks like you get them with -p linux.

The patch is trivial, though I'm not going to attach it here because I think I want to sneak in a change to the semantics of project try_by_default values that'll bitrot this.
If trunk bustage is something we care about (and given its current TBPL status, I believe that to be true), then it should run on Try along with the other SM builds :)
(In reply to Steve Fink [:sfink] from comment #11)
> The patch is trivial, though I'm not going to attach it here because I think
> I want to sneak in a change to the semantics of project try_by_default
> values that'll bitrot this.

That's in bug 974166.
Depends on: 974166
bhearsum: see comment 12.

And yes, we care about bustage in this build, especially since it's the only one that currently runs 'make check' for ARM code. That caught something already.
Attachment #8379826 - Flags: review?(bhearsum)
Try by default is nice, but the absolute minimum for visibility is "in trychooser." The answer when Luke asks how to run it so he can tell whether he has fixed the bustage he just landed on inbound should not be "um, there's some magic based on what desktop platform you choose, maybe it's -b d -p linux, not sure, probably a -b do -p all is safest." Even after it is try by default for people who know the magic platform, it's still not meeting the visiblity requirements if it's not in http://trychooser.pub.build.mozilla.org/.
That's, like, real work. The spidermonkey builds all run within a regular platform, so there's nowhere in the current trychooser UI to put those builds.

I could make -p linux[arm-sim] and -p all[arm-sim] work. Though I'd hoped to migrate those to mozharness builds, which have their own problem -- each of those requires its own platform (eg "linux64-br-haz"), which is obnoxious and difficult to manage. And the transition would break -p linux[arm-sim]. Hm, maybe I could keep -p all[arm-sim] working, though.

Requires thought.
Attachment #8379826 - Flags: review?(bhearsum) → review+
Blocks: 975466
Attachment #8379826 - Flags: checked-in+
in production.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.