Status

defect
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey)

Attachments

(3 attachments, 1 obsolete attachment)

Something I came across while working on bug 529430:  there's this AvmCore
class that is used throughout Nanojit.  It's a Tamarin-ism, AFAICT, and
isn't needed, and it's quite confusing.  All that *is* needed is the Config
struct that AvmCore (statically) contains.  So I decided to clean this up.

The NJ patch:

- Removes AvmCore from the nanojit/avmplus.h file that TM and lirasm use
  (but TR doesn't).  Removes all traces of AvmCore elsewhere.

- Removes some other dead stuff from avmplus.h.

- nInit() is now passed no arguments;  none of the back-ends need an argument.

- Removes 'has_cmov' from the Sparc back-end;  it is dead.

- Moves use_cmov() from AvmCore to Config.

- Updates lirasm for all the above changes, by adding a nanojit::Config to
  class Lirasm.
Attachment #495751 - Flags: review?(edwsmith)
The TM patch:

- Adds a nanojit::Config variable to jstracer.cpp, which gets passed to
  Writer, just like LogController.

- Updates things for the NJ changes.
Attachment #495752 - Flags: review?(gal)
The TR patch:

- Updates for the NJ changes.

- Fixes a signed/unsigned comparison error that was preventing it from building.
Attachment #495753 - Flags: review?(edwsmith)
Attachment #495752 - Attachment description: TMJ patch (against TM 58520:8921e3faccd2) → TM patch (against TM 58520:8921e3faccd2)
Comment on attachment 495752 [details] [diff] [review]
TM patch (against TM 58520:8921e3faccd2)

myConfig is a terrible name. How about just config? And instead of njConfig just config as well? Its obvious that its the nanojit config and its a member name. We can as well call it config.
Attachment #495752 - Flags: review?(gal) → review+
(In reply to comment #3)
> Comment on attachment 495752 [details] [diff] [review]
> TM patch (against TM 58520:8921e3faccd2)
> 
> myConfig is a terrible name.

Yes it is!  Well spotted.  That was a temporary name that I later changed but looks like I didn't change it in the ARM-only code.  It's NJConfig in the rest of jstracer.cpp.

I like the NJ/nj prefix because it indicates that it's a configuration relating to Nanojit, not the rest of TM.
Attachment #495751 - Flags: review?(edwsmith) → review+
Attachment #495753 - Flags: review?(edwsmith) → review+
Nice change, I'd been wanting to do this for a while and kept getting distracted.  Reviewing the code rekindled my desire to eliminate the use_cmov configuration flag as well (bug 586472).
Blocks: 624604
Blocks: 559132
rev TR patch
Attachment #495753 - Attachment is obsolete: true
changeset: 6077:8e3c1b63f9fa
user:      Nicholas Nethercote <nnethercote@mozilla.com>
summary:   Bug 617244 - nanojit: remove AvmCore (NJ-specific part).  r=edwsmith.

http://hg.mozilla.org/tamarin-redux/rev/8e3c1b63f9fa
changeset: 6078:59c829fda638
user:      Nicholas Nethercote <nnethercote@mozilla.com>
summary:   Fix ARM breakage from bug 617244.  r=me.

http://hg.mozilla.org/tamarin-redux/rev/59c829fda638
changeset: 6086:fc8730a8d886
user:      Nicholas Nethercote <nnethercote>
summary:   Bug 617244 - nanojit: remove AvmCore (r=edwsmith)
.
. attachment 495753 [details] [diff] [review] - TR patch (against TR 5632:00830e7731c4)
.
Created attachment 495751 [details] [diff] [review]
NJ patch (against TM 58520:8921e3faccd2)

Something I came across while working on bug 529430:  there's this AvmCore
class that is used throughout Nanojit.  It's a Tamarin-ism, AFAICT, and
isn't needed, and it's quite confusing.  All that *is* needed is the Config
struct that AvmCore (statically) contains.  So I decided to clean this up.

The NJ patch:

- Removes AvmCore from the nanojit/avmplus.h file that TM and lirasm use
  (but TR doesn't).  Removes all traces of AvmCore elsewhere.

- Removes some other dead stuff from avmplus.h.

- nInit() is now passed no arguments;  none of the back-ends need an argument.

- Removes 'has_cmov' from the Sparc back-end;  it is dead.

- Moves use_cmov() from AvmCore to Config.

- Updates lirasm for all the above changes, by adding a nanojit::Config to
  class Lirasm.

http://hg.mozilla.org/tamarin-redux/rev/fc8730a8d886
Does Tamarin Bot really need to duplicate earlier comments?  It's kind of annoying...
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.