Closed Bug 571229 Opened 14 years ago Closed 14 years ago

GCLog in GCMemoryProfiler missing argument; general cleanup necessary

Categories

(Tamarin Graveyard :: Profiler, defect, P3)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED
Q3 11 - Serrano

People

(Reporter: lhansen, Assigned: lhansen)

References

Details

(Whiteboard: has-patch)

Attachments

(1 file)

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.
Component: Garbage Collection (mmGC) → Profiler
QA Contact: gc → profiler
Priority: -- → P3
Target Milestone: --- → flash10.2
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."
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.
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.
Depends on: 574370
From blocking bug 574370 we have that %ll[udx] are not supported on WinMo 6, that bug has patches for a workaround.
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.
No longer depends on: 574370
Attached patch PatchSplinter Review
- 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.
Attachment #454239 - Flags: review?(treilly)
Whiteboard: has-patch
Comment on attachment 454239 [details] [diff] [review]
Patch

For pointers should we use %p instead?
Attachment #454239 - Flags: review?(treilly) → review+
(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.)
tamarin-redux changeset:   5002:daabc4219aea
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Flags: flashplayer-bug+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: