494157 - nanojit debug output uses inconsistent output channels

Status: VERIFIED FIXED

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect)

Description

[Julian Seward [:jseward]]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.10) Gecko/2009042700 SUSE/3.0.10-1.1 Firefox/3.0.10
Build Identifier: 

In js/src/nanojit/*.{cpp,h}, debugging output (as emitted by 
TRACEMONKEY=verbose ...) is sometimes printf'd, sometimes 
fprintf'd, and sometimes sent deferredly via Assembler::outputf et al.

Attached patch creates "void nj_dprintf( const char* format, ...)" and
sends all output through that instead.  Some notes:

* Assembler::outputf et al are unchanged, since it accumulates strings
  so they can be shown in reverse order later.  Of course when the
  strings are finally dumped, they go through nj_dprintf, just like
  everything else.

* nj_dprintf is in LIR.cpp.  I couldn't find a natural "misc helper fns"
  file to park it in.

* This change literally only deals with output from files in nanojit/,
  so as to keep nanojit independent.  In particular it doesn't deal with
  output from jstracer.cpp, even though that also emits lots of stuff
  with TRACEMONKEY=verbose enabled.
 
  On second thoughts it's probably easy enough to do if required, since 
  jstracer.cpp includes nanojit.h.

* nanojit.h __NanoAssertMsgf contains a fprintf(stderr ...) as part of
  an assertion failure handler.  I didn't change that on the assumption
  that all assertion failure text should go to stderr.  Right?


Reproducible: Always

Comment 1

[Julian Seward [:jseward]]

Attachment: patch to send all debug output from nanojit/*.{h,cpp} though a single function

Comment 2

[Julian Seward [:jseward]]

Attachment: patch extended to intercept debug output also from jstracer.cpp

Comment 3

[Julian Seward [:jseward]]

Re bullet point 3 of Description.  It's obviously trivial to extend 
use of nj_dprintf into jstracer.cpp.  A revised version of the patch
to do that, is attached at Comment #2.  That causes the vast majority
of the TRACEMONKEY=verbose output to go through nj_dprintf, but not
all of it.  I suspect to make this completely consistent would require
visiting all the files in jrs/src.  At a bare minimum we'd have to 
look at js_Disassemble1 in jsopcode.cpp.

Comments?

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 378851 [details] [diff] [review]
patch extended to intercept debug output also from jstracer.cpp

This is an improvement for sure.  I'm not worried about the bytecode disassembler stuff.

I am going to switch the output from stderr to stdout though.  By bulk most of it goes to stdout now (pre-patch), and I don't want to change that.  It's useful - debug spew interleaves with print() output.

Comment 5

[Julian Seward [:jseward]]

Attachment: Identical to patch v2 except sends via stdout, not stderr

Comment 6

[Andreas Gal :gal]

Remember to solicit feedback from Adobe for discussion-worthy structural changes to nanojit. This is a nice cleanup so I don't think anyone will mind though.

Comment 7

[Edwin Smith]

appreciate the nod, and yeah, cleanup is a big +1

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/7058d359fdc8

Comment 9

[Dragana Damjanovic [:dragana]]

Attachment: Part 4 - Add AltSvc support for Http3. r=mayhemer

Comment 10

[Phabricator Automation]

Comment on attachment 9094293 [details]
Part 4 - Add AltSvc support for Http3. r=mayhemer

Revision D46650 was moved to bug 1581637. Setting attachment 9094293 [details] to obsolete.