From de6e5bf33f6083842ffb0cd63c6578d1f9f23983 Mon Sep 17 00:00:00 2001
From: Brian Osman <brianosman@google.com>
Date: Fri, 21 Jul 2017 15:30:14 -0400
Subject: Tracing cleanup and bugfixes

TraceID was unused, remove it.

Simplify casting logic by using the same union helper as the macros.
This fixes a bug that was present in the bool handling - we were
treating the union value as a pointer, so we were dereferencing
random stack memory. Luckily it never crashes, we did get the wrong
values for bools.

Bug: skia:
Change-Id: I15d44756214f34c1f6479980d9a487ac7f3d8f6c
Reviewed-on: https://skia-review.googlesource.com/25801
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Salomon <bsalomon@google.com>
---
 tools/trace/SkChromeTracingTracer.cpp | 22 +++++++++++++++-------
 tools/trace/SkDebugfTracer.cpp        | 13 ++++++-------
 2 files changed, 21 insertions(+), 14 deletions(-)

(limited to 'tools/trace')

diff --git a/tools/trace/SkChromeTracingTracer.cpp b/tools/trace/SkChromeTracingTracer.cpp
index 4d76d2956d..0cfb1bf5f9 100644
--- a/tools/trace/SkChromeTracingTracer.cpp
+++ b/tools/trace/SkChromeTracingTracer.cpp
@@ -23,6 +23,9 @@ SkEventTracer::Handle SkChromeTracingTracer::addTraceEvent(char phase,
                                                            const uint8_t* argTypes,
                                                            const uint64_t* argValues,
                                                            uint8_t flags) {
+    // TODO: Respect flags (or assert). INSTANT events encode scope in flags, should be stored
+    // using "s" key in JSON. COPY flag should be supported or rejected.
+
     Json::Value traceEvent;
     char phaseString[2] = { phase, 0 };
     traceEvent["ph"] = phaseString;
@@ -32,30 +35,35 @@ SkEventTracer::Handle SkChromeTracingTracer::addTraceEvent(char phase,
     std::chrono::duration<double, std::nano> ns = now.time_since_epoch();
     traceEvent["ts"] = ns.count() * 1E-3;
     traceEvent["tid"] = static_cast<Json::Int64>(SkGetThreadID());
-    traceEvent["pid"] = 1; // TODO
+
+    // Trace events *must* include a process ID, but for internal tools this isn't particularly
+    // important (and certainly not worth adding a cross-platform API to get it).
+    traceEvent["pid"] = 0;
 
     if (numArgs) {
         Json::Value args;
+        skia::tracing_internals::TraceValueUnion value;
         for (int i = 0; i < numArgs; ++i) {
+            value.as_uint = argValues[i];
             switch (argTypes[i]) {
                 case TRACE_VALUE_TYPE_BOOL:
-                    args[argNames[i]] = (*reinterpret_cast<bool*>(argValues[i]) ? true : false);
+                    args[argNames[i]] = value.as_bool;
                     break;
                 case TRACE_VALUE_TYPE_UINT:
-                    args[argNames[i]] = static_cast<uint32_t>(argValues[i]);
+                    args[argNames[i]] = static_cast<Json::UInt64>(argValues[i]);
                     break;
                 case TRACE_VALUE_TYPE_INT:
-                    args[argNames[i]] = static_cast<int32_t>(argValues[i]);
+                    args[argNames[i]] = static_cast<Json::Int64>(argValues[i]);
                     break;
                 case TRACE_VALUE_TYPE_DOUBLE:
-                    args[argNames[i]] = *SkTCast<const double*>(&argValues[i]);
+                    args[argNames[i]] = value.as_double;
                     break;
                 case TRACE_VALUE_TYPE_POINTER:
-                    args[argNames[i]] = reinterpret_cast<void*>(argValues[i]);
+                    args[argNames[i]] = value.as_pointer;
                     break;
                 case TRACE_VALUE_TYPE_STRING:
                 case TRACE_VALUE_TYPE_COPY_STRING:
-                    args[argNames[i]] = reinterpret_cast<const char*>(argValues[i]);
+                    args[argNames[i]] = value.as_string;
                     break;
                 default:
                     args[argNames[i]] = "<unknown type>";
diff --git a/tools/trace/SkDebugfTracer.cpp b/tools/trace/SkDebugfTracer.cpp
index fe0da30b84..d694b8ce9a 100644
--- a/tools/trace/SkDebugfTracer.cpp
+++ b/tools/trace/SkDebugfTracer.cpp
@@ -24,11 +24,11 @@ SkEventTracer::Handle SkDebugfTracer::addTraceEvent(char phase,
         } else {
             args.append(" ");
         }
+        skia::tracing_internals::TraceValueUnion value;
+        value.as_uint = argValues[i];
         switch (argTypes[i]) {
             case TRACE_VALUE_TYPE_BOOL:
-                args.appendf("%s=%s",
-                             argNames[i],
-                             (*reinterpret_cast<bool*>(argValues[i]) ? "true" : "false"));
+                args.appendf("%s=%s", argNames[i], value.as_bool ? "true" : "false");
                 break;
             case TRACE_VALUE_TYPE_UINT:
                 args.appendf("%s=%u", argNames[i], static_cast<uint32_t>(argValues[i]));
@@ -37,16 +37,15 @@ SkEventTracer::Handle SkDebugfTracer::addTraceEvent(char phase,
                 args.appendf("%s=%d", argNames[i], static_cast<int32_t>(argValues[i]));
                 break;
             case TRACE_VALUE_TYPE_DOUBLE:
-                args.appendf("%s=%g", argNames[i], *SkTCast<const double*>(&argValues[i]));
+                args.appendf("%s=%g", argNames[i], value.as_double);
                 break;
             case TRACE_VALUE_TYPE_POINTER:
-                args.appendf("%s=0x%p", argNames[i], reinterpret_cast<void*>(argValues[i]));
+                args.appendf("%s=0x%p", argNames[i], value.as_pointer);
                 break;
             case TRACE_VALUE_TYPE_STRING:
             case TRACE_VALUE_TYPE_COPY_STRING: {
                 static constexpr size_t kMaxLen = 20;
-                const char* str = reinterpret_cast<const char*>(argValues[i]);
-                SkString string(str);
+                SkString string(value.as_string);
                 size_t truncAt = string.size();
                 size_t newLineAt = SkStrFind(string.c_str(), "\n");
                 if (newLineAt > 0) {
-- 
cgit v1.2.3