Closed Bug 601539 Opened 14 years ago Closed 14 years ago

nanojit: fix bogus generation of suffix names in LIR dumps

Categories

(Core Graveyard :: Nanojit, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: n.nethercote, Assigned: n.nethercote)

Details

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

Attachments

(1 file)

LIR dumps can contain hand-picked names, which are added with addName().  If
a name is used more than once in a fragment, a numeric suffix is added.  Eg.
we have "foo", "foo2", "foo3", etc.  'namecounts' is the table used to
generate these suffixes.

However, the code for doing this is buggy.  It stores strings (const char *)
in a CountMap, which is a HashMap.  The HashMap does its hashing and
comparison of the strings on the string pointers, rather than the string
contents.  In many cases this doesn't matter, because often the names are
unique static strings, so pointer comparison/hashing gives equivalent results
to contents comparison/hashing.  

However, in TraceMonkey at least, a lot of the string names are formed on
the stack using sprintf(), eg. "$stack<N>", or "guard(class is <Class>)".
As a result, non-equal strings can be considered equal, and vice versa.
This means that sometimes the suffixes grow too quickly, eg. we have "foo"
then "foo3", skipping "foo2".  And sometimes a suffix isn't added when it
should be.  These bugs make it harder to compare two separate LIR dumps for
differences;  minor real changes in names can cause knock-on bogus changes
due to minor changes in the stack layout.

This patch fixes this by creating a simple string wrapper struct, 'Str',
which defines '==' to compare string contents, and also struct StrHash,
which hashes Str objects according to the string contents.

The patch also avoids incrementing suffixes in CountMaps needlessly in the
case where an instruction is CSE'd, by moving the 'names.containsKey(ins)'
checks before the suffix generation (this is done for all three CountMaps:
lircounts, funccounts, namecounts).
Attachment #480552 - Flags: review?(rreitmai)
Comment on attachment 480552 [details] [diff] [review]
patch (against TM 54746:760b8a9c9a34)

Solution seems fine, but does not appear to compile for me with TR, although this might be due to TR being slightly out of data with what's in NC.
Attachment #480552 - Flags: review?(rreitmai) → review+
I didn't have any problems compiling TR on my Linux box, so...

http://hg.mozilla.org/projects/nanojit-central/rev/2c66185d4d35
Whiteboard: fixed-in-nanojit
Huh, it appears GCC on Linux allows making a reference to a reference, but other compilers (including GCC on Mac) do not.  I pushed a fix:

http://hg.mozilla.org/projects/nanojit-central/rev/12776aa248b9
http://hg.mozilla.org/tracemonkey/rev/67723e0d73fa
http://hg.mozilla.org/tracemonkey/rev/71e339dc6019
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/67723e0d73fa
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey → fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.