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)
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)
6.07 KB,
patch
|
rreitmai
:
review+
|
Details | Diff | Splinter Review |
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 1•14 years ago
|
||
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+
Assignee | ||
Comment 2•14 years ago
|
||
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
Assignee | ||
Comment 3•14 years ago
|
||
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
Assignee | ||
Comment 4•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/67723e0d73fa http://hg.mozilla.org/tracemonkey/rev/71e339dc6019
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tracemonkey
Comment 5•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/67723e0d73fa
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey → fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin
Comment 6•14 years ago
|
||
Comment on attachment 480552 [details] [diff] [review] patch (against TM 54746:760b8a9c9a34) http://hg.mozilla.org/tamarin-redux/rev/9cff23ae6ee7
Updated•10 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•