571229 - GCLog in GCMemoryProfiler missing argument; general cleanup necessary

Status: RESOLVED FIXED

(Tamarin Graveyard :: Profiler, defect, P3)

Description

[Lars T Hansen]

Near the end of DumpSimple, the call to GCLog needs to pass a value:

+        
+        GCLog("%u traces", num_traces);

BTW the types of arguments to GCLog elsewhere in that function are extremely dodgy and that all needs to be cleaned up too.

Also see bug #571227 and the initial patch on that bug.

Comment 1

[Lars T Hansen]

As for the general cleanup, the question is whether %lld and %llu are supported on WinMo or not.  My memory is that they are not (and thus some of the other GC logging code uses %.0f instead) but it could be that I'm mixing it up with my memory of %zd (for size_t) not being supported.  This needs to be investigated by testing, pretty much.

In email Tom Harwood says: "It's been a while, but I thought we had a macro that expanded to the right
formatting construct on WinMo and to normal %ll formatting on sane platforms.  Might be wishful thinking."

Comment 2

[Lars T Hansen]

There's also the issue of %p - is it supported on WinMo, and in general, how portable is it?  It's known that most platforms format with a leading 0x, and I believe the standard requires this (to be checked), but the Win32 libraries omit the leading 0x.  So uses of %p may also need to be worked around.

Comment 3

[Ruchi Lohani]

Regarding %p: it is supported on WinMo (from doc: http://msdn.microsoft.com/en-us/library/ms859877.aspx) and it seems its portable, though 0x is omitted on windows. We could resolve that by having a macro for pointer format string.
Something like: GCDebugMsg("Pointer: " PTR_FMT "\n", ptr); [PTR_FMT: "0x%p" on MSVC and "%p" otherwise] 

The usual way to use portable format strings is to use inttypes.h but it is also not included in MSVC by default. I think we should just include some compiler specific macros/defines to get rid of this issue for GCLogs.

Comment 4

[Lars T Hansen]

From blocking bug 574370 we have that %ll[udx] are not supported on WinMo 6, that bug has patches for a workaround.

Comment 5

[Lars T Hansen]

We have decided that we're not going to worry about WinMo 6, and that until we have WinMo 7 SDKs we're going to ignore the problem and assume it's been fixed.  Removing dependency on compatibility fix for %ll[dxu] for now.

Comment 6

[Lars T Hansen]

Attachment: Patch

- Adds the missing argument to GCLog in DumpSimple
- Uses %llu etc formats for size_t, uintptr_t, and pointers - it's the 
  only safe thing to do, the 'z' size_t format is not universally supported,
  and on Mac 64-bit builds (for example), size_t is 64 bits
- Rewrites one use of snprintf that overlaps source and destination,
  the behavior of that is undefined in ANSI/ISO C (I checked :-)

All these casts and formatting details suggest that moving to a C++-style formatter for GCLog might be a good idea, but that's for another day.

Comment 7

[Tommy Reilly]

Comment on attachment 454239 [details] [diff] [review]
Patch

For pointers should we use %p instead?

Comment 8

[Lars T Hansen]

(In reply to comment #7)
> Comment on attachment 454239 [details] [diff] [review]
> Patch
> 
> For pointers should we use %p instead?

%p is not portable, thanks to the visual studio libraries - no leading "0x" is printed, at least not in the 2008 version.  You can argue about which way is the right way, but absent portability ...  Anyway, %p for anything machine-readable is probably not right, for human-readable-only output it's probably OK.  (I'm not sure we should have any of the latter in the GC code.)

Comment 9

[Lars T Hansen]

tamarin-redux changeset:   5002:daabc4219aea