479431 - introduce printf-style logging api, remove PrintWriter, OutputStream, ConsoleOutputStream, StringOutputStream

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Virtual Machine, enhancement)

Description

[Edwin Smith]

PrintWriter.formatV() contains extra printf-style formatting directives for the VM types (Traits, etc).  Need to refactor this to put output into a string buffer instead of a string, then use it from a uniform logging api.  VMPI_trace()?

- refactor formatting to be pure-string based.
- use refactored formatV from PrintWriter.formatV()
- use refactored formatV from new logging api
- (incrementally) replace uses of core->console with new api
- eventually remove PrintWriter and the other stream like classes.

add dependent and blocked tasks to this bug.

Comment 1

[Rishit Shah]

To recap the email discussion and bugs –
 
The goal is to replace stream-style APIs with a printf-style API (VMPI_trace) that would be used by the core (and shell?) code for logging messages.
 
For porting purposes, we’ll still need a platform-facing API to output messages.
 
//Sample code (somewhere in core code)
void VMPI_trace(const char* format, …);
{
      …
…
formatV (msgBuffer, format, args);  //refactored format
 
VMPI_Log(msgBuffer); //Platform api to output the message to console/file/socket
}

I would also propose adding a corresponding VMPI_trace_error() and VMPI_Log_Error() APIs to output error messages.
 
One question though is that do we need to call it VMPI_trace since the implementation is already platform independent.  The VMPI prefix does make sense for VMPI_Log/VMPI_Log_Error.  I have the same confusion regarding VMPI_alloca.

Comment 2

[Rishit Shah]

Edwin's response to Comment #1 -

agree, it should not be called VMPI_trace() if it isn't a platform api.  We don't have a core->console_error() so I don't think we need a trace_error() either.

Comment 3

[Rishit Shah]

(In reply to comment #2)
> Edwin's response to Comment #1 -
> 
> agree, it should not be called VMPI_trace() if it isn't a platform api.  We
> don't have a core->console_error() so I don't think we need a trace_error()
> either.

There are uses of fprintf(stderr, ...) in the core code that could replaced by an error logging api.

Comment 4

[Rishit Shah]

Both MMgc and Avmplus need to call tracing APIs.  However, I don't see a pattern that allows sharing the same APIs currently in the codebase.  I see the similar issue with DebugMsg APIs however, there implementations are duplicated as GCDebugMsg and AvmDebugMsg.

I would like to avoid doing this and have a single function.  However, implementing in avmplus would mean that MMgc now relies on avmplus.  I would think that is not desirable.

Any suggestions?

To me, MMgc project seems to be the place.

Comment 5

[Rishit Shah]

Spoke too soon, MMgc project might not work as well given that formatV has dependency on ScriptObject and suchlike.

Comment 6

[Lars T Hansen]

Some thoughts:

- you're right about VMPI_alloca, I think that it is misnamed.  Our thinking on
  VMPI_ functions has evolved.  The VMPI_alloca macro is not compatible with
  alloca because (a) it's a macro and (b) it has a different signature than
  standard alloca and (c) it diverts large requests to the heap and (d) it is
  in bed with the exception handling system and can't be called reliably from
  outside the VM.

- it would be ideal, IMO, if the true VMPI_ layer was independent of both mmgc
  and avmplus, and that it could be relied on by either of them without
  requiring the other to be present, and that it could be relied on by the
  shell and by other embedders for the functionality it provides without
  booting up mmgc or avmplus.  I don't know what it would cost to do this.

- even if the VMPI_ layer can't be independent of mmgc I see it as a mistake
  to think that it is part of mmgc.

- the VMPI_ layer should be for the lowest possible layer of platform
  functionality, eg, "write string to device".  Any formatting should take
  place on a higher level.  If the VMPI_ layer needs memory allocation
  services it should /not/ use mmgc but should rely on platform services
  directly, or it should use static buffers.  It is by its very nature a
  completely platform-dependent layer.

- by that thinking it doesn't matter if GCDebugMsg and AvmDebugMsg are
  different; lowest down they call VMPI_Log and VMPI_LogError (if two are
  even necessary, which I sort of doubt at this point).  Shared code at
  the intermediate level is good but hardly required.

Comment 7

[Rishit Shah]

In all cases so far VMPI_ is independent of MMgc and avmplus and as you said, it should remain that way.  I was trying to avoid the code size hit for duplicating formatV like functions.  As it turns out that MMgc, so far, does not require formatV.  Even if it did the additional code size is worth in order to direct dependency between the 2 projects.

As a result, I ended up implementing the tracing functions similar to what you've outlined in point# 4.

Also, if we consider that printf == fprintf(stdout, ...) == fprintf(stderr, ...) our our purposes then VMPI_LogError is not necessary.  I was being cautious about changing any existing logging behavior.

Comment 8

[Lars T Hansen]

One buggaboo with the printf-style formatting is that it loses in the presence of opaque data types like size_t, ptrdiff_t, uintptr_t, and intptr_t, many of which we depend on.

For example, Tommy's recent GC fixes has a number of portability problems in this area, relying on fprintf to format values of those types.  (GCC warns about these when it knows they're wrong, so the changes introduced a bunch of warnings in MacOS X builds.)

ISO C 99 has some modifiers to handle these values - %zu for size_t, %tu for ptrdiff_t - but it's easy to forget.  Portable (in ISO C 99) ways of printing uintptr_t include (a) casting to void* and using %p or (b) casting to uintmax_t and using %ju (j means 'uintmax_t' or 'intmax_t', depending) or (c) #defining a format string for printing the various types and then interpolating that string into our own format strings, eg, "limit=" SIZE_T_FMT, which works because of string concatenation in the compiler.

However, I have doubts that these types and modifiers are very portable in the sense that compilers actually support them.

All this argues against printf-style formatting, obviously.

Comment 9

[Tommy Reilly]

I tried to get them to the point where MSVC and GCC were happy, I think its close but there's still some GCC warnings.

Comment 10

[Steven Johnson]

(In reply to comment #9)
> I tried to get them to the point where MSVC and GCC were happy, I think its
> close but there's still some GCC warnings.

Understood, but IMHO we need to continue to take a hard line on no-warnings-allowed. For the warnings in GCC we can probably safely downcast to uint32.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.