557706 - Allow LogControl printf method to be overridden

Status: RESOLVED FIXED

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

Description

[Rick Reitmaier]

if LogControl.printf is made a virtual method then consumers of nanojit can more easily control how the output is managed.

Comment 1

[Rick Reitmaier]

Attachment: ver1

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 437481 [details] [diff] [review]
ver1

"ver1"?  Are you planning further versions of this patch? :P

I assume you're planning to subclass this within TM?  Seems fine.

Comment 3

[Rick Reitmaier]

(In reply to comment #2)
> (From update of attachment 437481 [details] [diff] [review])
>  Are you planning further versions of this patch? :P

Certainly hope not, but it pays to be ready.

> I assume you're planning to subclass this within TM?  Seems fine.

Yes, exactly.  It will allow us to redirect the output more easily.

Comment 4

[Rick Reitmaier]

landed http://hg.mozilla.org/projects/nanojit-central/rev/7fa9ad2e2d57

Comment 5

[Edwin Smith]

This breaks tamarin-redux, because in release builds, NJ_VERBOSE is not enabled, and so there is a link error due to the missing vtable for LogControl.

If we only need build-time variation of how LogControl::printf() is implemented, then it should be good enough to move the code from LIR.cpp to avmplus.h, or somewhere in TM, and provide an alternate implementation in CodegenLIR.cpp, like we do in several other cases.

If we actually need *runtime* polymorphism, then we need the virtual method, and the code needs to be rearranged so we never reference class LogControl at all when NJ_VERBOSE is defined, or to ensure a stubbed out printf() method is provided.

If you can fix this soon, I can hold off importing from nanojit-central.  Otherwise we should revert the one-line change.

Comment 6

[Rick Reitmaier]

(In reply to comment #5)
> If you can fix this soon, I can hold off importing from nanojit-central. 
> Otherwise we should revert the one-line change.

Fixing, should have a patch by later today.

Comment 7

[Rick Reitmaier]

Attachment: ver 2 - seems we did need this after all.

Comment 8

[Rick Reitmaier]

Attachment: tamarin patch that uses core->console for output

Comment 9

[Rick Reitmaier]

Even after these changes we still have a problem with output in tamarin release debugger builds since NJ_VERBOSE is set to 0 in avmshell-features.h

Comment 10

[Nicholas Nethercote [inactive]]

Comment on attachment 437977 [details] [diff] [review]
ver 2 - seems we did need this after all.

Works for TM, though I'm not sure why the destructor is needed.

Comment 11

[Rick Reitmaier]

dtor is probably technically not needed unless you derive from LogControl, so for TM its fine, but TR will be using 'class AvmLogControl : public LogControl'

Comment 12

[Edwin Smith]

(In reply to comment #9)
> Even after these changes we still have a problem with output in tamarin release
> debugger builds since NJ_VERBOSE is set to 0 in avmshell-features.h

That's a separate problem.  NJ_VERBOSE could/should be getting enabled whenever AVMPLUS_VERBOSE is enabled; this stopped working at some point and I think Steven has some rationale for it.

Comment 13

[Rick Reitmaier]

(In reply to comment #12)
> (In reply to comment #9)
> > Even after these changes we still have a problem with output in tamarin release
> > debugger builds since NJ_VERBOSE is set to 0 in avmshell-features.h
> 
> That's a separate problem.  NJ_VERBOSE could/should be getting enabled whenever
> AVMPLUS_VERBOSE is enabled; this stopped working at some point and I think
> Steven has some rationale for it.

Yes separate issue but wanted to surface it since I'm also in talks with Steven regarding it.

Comment 14

[Edwin Smith]

Comment on attachment 437978 [details] [diff] [review]
tamarin patch that uses core->console for output

conditional R+

must fix:
- use VMPI_vsnprintf instead of vsprintf

please fix:
- the 4K buffer doesn't need to be a member of AvmLogControl since its only a temp buffer for printf() to use.  stack allocation would be better.  If the output lines get clipped anyway, a lower clip limit is probably fine.

- fix indentation of coment after AvmLogControl decl in CodeMgr.h

Also, i noticed that CodegenLIR.log is only used in two places, could replace &log with &pool->codeMgr->log, and remove the AvmLogControl declaration in CodeMgr.

Comment 15

[Rick Reitmaier]

tamarin changes
  - 4K buffer reduced to 1K , now stack allocated in printf()
  - VMPI_vsnprintf() used
  - whitespace cleanup
  - extraneous LogControl removed from CodegenLIR object

pushed http://asteam/hg/tamarin-redux/rev/c31cc23c4a8a

nanojit :

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

Comment 16

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/61bfaaac38b5

Comment 17

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/0e2c79fd8abf