586262 - Get rid of blx_lr_bug

Status: RESOLVED FIXED

(Core Graveyard :: Nanojit, defect)

Description

[Mike Hommey [:glandium]]

the WinCE device emulator has a bug with 'blx lr', and a blx_lr_bug bool was introduced to avoid that bug and use some other construct when it occurs.

I seems more sensible to just avoid using 'blx lr' at all, avoiding additional runtime tests.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Proposed patch (against m-c)

Comment 2

[Edwin Smith]

+1

blx_lr_bug is a gross hack (i'm to blame)

The only hazard I can think of in this patch is that IP is not a managed register, and is used implicitly as scratch in various instructions.  I reviewed underrunProtect(), and it's not used there, I'm pretty sure.

The code path in asm_regarg that calls findSpecificRegFor(p, r), will be a problem if r == IP, since IP is not a managed register.

Comment 3

[Jacob Bramley [:jbramley]]

(In reply to comment #2)
> The code path in asm_regarg that calls findSpecificRegFor(p, r), will be a
> problem if r == IP, since IP is not a managed register.

Actually, that could be a problem as that code path looks like it'll be hit in some cases.

There are two obvious solutions that I can see:
  • Change the p->isExtant() check to "(p->isExtant()) || (r == IP)". This is perhaps a little fragile, but it looks sane at a glance.
  • Use the register allocation API to get a register for the value, then BLX to that register. This is more complicated perhaps, but safer.

Comment 4

[Mike Hommey [:glandium]]

Attachment: Updated patch

Edwin was right in comment 2. It crashed in the findSpecificRegFor path. Using Jacob's first suggestion, it seems to work properly.

Comment 5

[Mike Hommey [:glandium]]

Attachment: Get rid of blx_lr_bug

In then end, I haven't managed to get a better solution to work, and I don't care enough about this codepath (unused in tracemonkey) to spend more time on it, however interesting it can be.

I'm therefore going this road:
<edwsmith> hey, guys, here's another proposition for you
 why dont we rip out blr_lr_broken() and blr_lr_bug, and just put #ifdef WINCE around the "bug" case in asm_call()
<jbramley> Works for me.

I used UNDER_CE instead of WINCE, though.

Comment 6

[Mike Hommey [:glandium]]

The armv4t patch I'm working on doesn't depend on that anymore.

Comment 7

[Edwin Smith]

Attachment: Removes WinCEUtil.armasm

With this patch we no longer have to detect the broken emulator, so don't need this weird armasm file that didn't belong under MMgc anyway.

Comment 8

[Mike Hommey [:glandium]]

Landed the nanojit part.
http://hg.mozilla.org/projects/nanojit-central/rev/9e1146b15442

Comment 9

[Edwin Smith]

nanojit part in TR: http://hg.mozilla.org/tamarin-redux/rev/211232d5bd76

TR part in TR: http://hg.mozilla.org/tamarin-redux/rev/9804879bc296

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/6a7c7a36e49e