[PATCH] D47208: [profile] Support profiling runtime on Fuchsia

[Roland McGrath via Phabricator via llvm-commits]

mcgrathr added a comment.

I don't have enough context on how the profiling runtime works to properly review this implementation strategy for Fuchsia.  Perhaps adequate comments here would tell me enough.  Certainly it needs a lot more comments.
But probably we just need to talk through the implementation plan in person first.



================
Comment at: clang/lib/Driver/ToolChains/Fuchsia.cpp:106
   AddLinkerInputs(ToolChain, Inputs, Args, CmdArgs, JA);
+  ToolChain.addProfileRTLibs(Args, CmdArgs);
 
----------------
addSanitizerRuntimes should also be after AddLinkerInputs.


================
Comment at: compiler-rt/lib/profile/InstrProfilingPlatformFuchsia.c:1
+/*===- InstrProfilingPlatformFuchsia.c - Profile data Linux platform ------===*\
+|*
----------------
s/Linux/Fuchsia/


================
Comment at: compiler-rt/lib/profile/InstrProfilingPlatformFuchsia.c:9
+\*===----------------------------------------------------------------------===*/
+
+#if defined(__Fuchsia__)
----------------
This needs an overview comment explaining the implementation scheme.  Overall the code needs a lot more comments.
See sanitizer_coverage_fuchsia.cc for examples.


================
Comment at: compiler-rt/lib/profile/InstrProfilingPlatformFuchsia.c:14
+#include <stdlib.h>
+#include <sys/types.h>
+
----------------
Why are these needed?  stdlib.h is for atexit.  <sys/types.h> should never be used in new code that is not POSIXy.
stdio.h should not be used here.


================
Comment at: compiler-rt/lib/profile/InstrProfilingPlatformFuchsia.c:24
+
+static const size_t MappingSize = sizeof(uintptr_t) << 32;
+const char ProfileSinkName[] = "profile";
----------------
This needs comments explaining why this is a reliable upper bound.


================
Comment at: compiler-rt/lib/profile/InstrProfilingPlatformFuchsia.c:25
+static const size_t MappingSize = sizeof(uintptr_t) << 32;
+const char ProfileSinkName[] = "profile";
+
----------------
This should be static.
The sink name should be something less generic, since it needs to identify the file format unambiguously.
Maybe "llvm-profile"?


================
Comment at: compiler-rt/lib/profile/InstrProfilingPlatformFuchsia.c:36
+                               uint32_t NumIOVecs) {
+  struct WriterContext *Ctx = (struct WriterContext *)This->WriterCtx;
+  zx_status_t Status;
----------------
C does not require such casts from void *.  If LLVM C style does require them, you can leave it.


================
Comment at: compiler-rt/lib/profile/InstrProfilingPlatformFuchsia.c:37
+  struct WriterContext *Ctx = (struct WriterContext *)This->WriterCtx;
+  zx_status_t Status;
+  uint32_t I;
----------------
Use C99 style: declare only at first initialization.


================
Comment at: compiler-rt/lib/profile/InstrProfilingPlatformFuchsia.c:47
+  uint64_t Size;
+  Status = zx_vmo_get_size(Ctx->Vmo, &Size);
+  if (Status != ZX_OK)
----------------
You should use only the _zx_* entry points.


================
Comment at: compiler-rt/lib/profile/InstrProfilingPlatformFuchsia.c:92
+
+  char VmoName[ZX_MAX_NAME_LEN];
+  zx_status_t Status;
----------------
Move decls to first use.


================
Comment at: compiler-rt/lib/profile/InstrProfilingPlatformFuchsia.c:109
+  /* Give the VMO a name including our process KOID so it's easy to spot. */
+  snprintf(VmoName, sizeof(VmoName), "%s.%zu", ProfileSinkName,
+           Info.koid);
----------------
%zu is for size_t.  KOIDs are uint64_t, so use "%" PRIu64 (<inttypes.h>).


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D47208