574370 - printf %ll[dux] may not be portable

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Lars T Hansen]

WinMo 6 is still a supported platform.  Its printf functions do not support the "long long" specifiers and we must not use them in code.

Here's a regex to scan for them:   "%-?[0-9]*ll"

I've found occurrences in:

  GCMemoryProfiler.cpp
  LIR.cpp
  vprof.cpp

An imperfect workaround is to use %.0f and cast the value to double.  A better workaround is a formatting function that formats the value into a local string.

Comment 1

[Lars T Hansen]

Attachment: VMPI_longlongFormat

Introduces a new porting interface, VMPI_longlongFormat, that takes a formatting char, a buffer, a buffer size, and a value and formats the value according to the format into the buffer and returns a buffer pointer.  The idea is to be able to have multiple local buffer variables and just replace this:

  VMPI_snprintf("%llx %lld", v, w)

with this

  char b1[22], b2[22];
  VMPI_snprintf("%s %s", VMPI_longlongFormat('x', b1, sizeof(b1), v),
                         VMPI_longlongFormat('d', b2, sizeof(b2), w));

The value '22' is the size of buffer required for a 64-bit signed int with a NUL terminator, it's possible we want a #define for this exported from the VMPI layer based on the size of a long long value (after all it could be 128 bits :-)

Patch includes VMPI.h definition, and implementations for Posix and Windows systems including WinMo.  Has seen some testing.

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

quick initial feedback: this is mainly intended for internal use, ie debugging and instrumentation right?  Assuming performance is not an issue, why not just use the WinCE version universally?  Seems like keeping separate paths is just an invitation for subtle differences and subsequent confusion when trying to diagnose problems with log output later.

Comment 3

[Lars T Hansen]

You're right, performance should not be an issue, and that will probably be true even if we go outside the domain of debugging and instrumentation.

Also, the only portability issue with a common format would be different sizes of 'long long' on different platforms, but the common version should be able to deal with that.

I could be persuaded to just have a single version and move it out of the VMPI layer, though that means placing it in the GC (we have no common layer in the VM below the GC and the Core and above the VMPI layer - though we need one, when I moved AvmAssert out of core it ended up in VMPI but that was not ideal.)  I suppose we could try to create that common layer, but I don't know how it will impact the Flash Player.

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

second note: your patch introduces tab characters into WinPortUtils.cpp.

will be uploading a selftest file in a moment

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: selftest for longlongFormat (requires host snprintf)

Thought it would be good to double-check the implementation on some boundary cases.

Comment 6

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 453760 [details] [diff] [review]
VMPI_longlongFormat

Patch looks good, modulo subjective opinions about bracketing style.

I manually turned on the wince code in posix for testing; self-test did not find any problems.

My self-test did not exercise  handling of illegal specifier arguments; the logic here looks correct (it degenerates into same code that handles value=0).  Then again, perhaps an illegal specifier should not be silently returning 0?

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

(by "returning 0", I really meant putting the string "0" into the target buffer and returning it.)

Comment 8

[Lars T Hansen]

Decision is that WinMo 6 will never be supported, and we put this bug on ice until we can test WinMo 7 to see if the bug has been fixed there.

Comment 9

[Dan Smith]

Needs test on Windows Phone 7

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.