531687 - Duplicate node names in TMFLAGS=aftersf printout

Status: RESOLVED FIXED

(Core Graveyard :: Nanojit, defect)

Description

[Julian Seward [:jseward]]

Test case:

  function Blah(faa, fbb, fcc, fdd) {
      this.aa = faa;
      this.bb = fbb;
      this.cc = fcc;
      this.dd = fdd;
      return this;
  }

  function addemup(b) {
      return b.aa + b.bb + b.cc + b.dd;
  }

  function main ( ) {
     var sum = 0;
     var qq1 = new Blah(1, 2, 3, 4);
     for (var i = 0; i < 5000000; i++)
         sum += addemup(qq1);
     print("sum is", sum);
  }

  main();

When run with TMFLAGS=aftersf, produces two traces.  The one with
FragID=000002 has this:

  43: shape = ld map[4]
  44: shape = int 180
  45: guard_kshape = eq shape, shape

which in this case is not too confusing, but in general it could be.
IMO all LIR nodes should be forced to have different names so as to
avoid any ambiguity when reading traces.

Comment 1

[Nicholas Nethercote [inactive]]

This is caused by the use of addName() to give instructions special names.  Special names should probably be given numeric suffixes just like normal names.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: NJ patch

As part of adding the suffix, I completely overhauled the debug printing
because it was a real mess.

Previously, the printing/naming work was shared between LabelMap and
LirNameMap.  LabelMap recorded names for addresses, LirNameMap recorded
names for instructions.  Both classes also did some printing of their info.
LabelMap also contained a circular buffer which was shared by both classes.
The handling of buffers was particularly confusing -- some printing
functions got passed a buffer and wrote to that, but others printed to the
shared buffer.

One consequence of this was large amounts of string copying.  For a call to
formatIns(), some substrings could be copied four times: formatRef()
generates a name string into a local buffer, then it copies into the shared
buffer;  then in formatIns() that same string is sprintf'd into a different
local buffer as part of a larger string, and that larger string is then
copied into the shared buffer.

Clean-ups included in the patch:

- Renamed LabelMap as AddrNameMap, which is clearer and mirrors LirNameMap.

- Introduced an LInsPrinter class, a single class responsible for all the
  printing stuff.  It contains an AddrNameMap and a LirNameMap, and those
  two classes now only store the address/name info;  LInsPrinter has all the
  smarts for printing.  External users of the printing services now only
  need to create a single LInsPrinter.

- All printing functions now take a buffer, and print into the buffer.
  There are no shared buffers, and all buffers are stack-allocated by the
  caller of the printing function.  This is a little more verbose, but
  consistent and easy to understand, and makes everything re-entrant -- bug
  547736 suggests that the current code may have some data races.

  (Nb: I tried a more C++ish approach using a single shared buffer and
  overloading the '<<' operator to avoid the excess copying.  But
  printf-style "%s = %d" strings ended up being simpler and clearer than
  'out << s << " = " << i' things;  plus this C++ish approach didn't make
  things re-entrant.)

  There are two kinds of buffers, InsBuf (bigger) and RefBuf (smaller).  The
  formatXyz() functions won't exceed the buffer's lengths (they use
  snprintf()).  I used two buffers with hard-coded sizes because it allows
  overflows to be avoided, but avoids having to pass around (buf,length)
  pairs everywhere.  (I checked the truncation works by temporarily
  shrinking the buffer sizes and checking the output.)

- The suffix is added for specially-named instructions.  There's an
  exception -- the suffix '1' isn't printed.  In many cases there's only a
  single instance of the name, so this avoids many unnecessary suffixes.
  '1' suffixes are still printed for non-special names, however.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: TM patch

Simple changes to account for the NJ changes.

Comment 4

[Nicholas Nethercote [inactive]]

Nb: no TR patch yet, because the lack of recent NJ-to-TR merges makes it difficult.

Comment 5

[Nicholas Nethercote [inactive]]

(In reply to comment #4)
> Nb: no TR patch yet, because the lack of recent NJ-to-TR merges makes it
> difficult.

It should only require two basic changes:
- Tweak the signatures of formatGuard() and formatGuardXov().
- Rename LirBuffer::names as LirBuffer::printer everywhere.

Comment 6

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/projects/nanojit-central/rev/c844bd17e4a5

http://hg.mozilla.org/tracemonkey/rev/42a26f80a36c
http://hg.mozilla.org/tracemonkey/rev/86ec43968976

Comment 7

[Edwin Smith]

http://hg.mozilla.org/tamarin-redux/rev/6482364f1484

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/42a26f80a36c