[PATCH] D52162: [XRay] Support for Fuchsia

[Roland McGrath via Phabricator via llvm-commits]

mcgrathr added inline comments.


================
Comment at: compiler-rt/lib/xray/xray_allocator.h:54
+                 Vmo, 0, sizeof(T), &B);
+  if (Status != ZX_OK) {
+    if (Verbosity())
----------------
Need `_zx_handle_close(Vmo);` here (before the error check but after the use).


================
Comment at: compiler-rt/lib/xray/xray_allocator.h:92
+    if (Verbosity())
+      Report("XRay Profiling: Failed to create VM object of size %d: %s\n",
+             S, _zx_status_get_string(Status));
----------------
Be consistent: either "VMO" or "VM object" in all messages.
s/%d/%zu/


================
Comment at: compiler-rt/lib/xray/xray_allocator.h:100
+                 Vmo, 0, S, &B);
+  if (Status != ZX_OK) {
+    if (Verbosity())
----------------
Need `_zx_handle_close(Vmo);` here (before the error check but after the use).


================
Comment at: compiler-rt/lib/xray/xray_allocator.h:102
+    if (Verbosity())
+      Report("XRay Profiling: Failed to create memory mapping of size %d: %s\n",
+             S, _zx_status_get_string(Status));
----------------
s/%d/%zu/


================
Comment at: compiler-rt/lib/xray/xray_allocator.h:123
+#if SANITIZER_FUCHSIA
+  _zx_vmar_unmap(_zx_vmar_root_self(), reinterpret_cast<uintptr_t>(B), S);
+#else
----------------
It wouldn't hurt to just define internal_munmap this way for Fuchsia if it reduces the `#if` load.


================
Comment at: compiler-rt/lib/xray/xray_buffer_queue.cc:19
+#if SANITIZER_FUCHSIA
+#include <zircon/process.h>
+#include <zircon/syscalls.h>
----------------
These are unused in this file.  Just use `#if !SANITIZER_FUCHSIA` around the `#include "sanitizer_common/sanitizer_posix.h"`


================
Comment at: compiler-rt/lib/xray/xray_init.cc:20
 #include "sanitizer_common/sanitizer_common.h"
+#include "sanitizer_common/sanitizer_platform.h"
 #include "xray_defs.h"
----------------
unused


================
Comment at: compiler-rt/lib/xray/xray_interface.cc:289
+#if SANITIZER_FUCHSIA
+  MProtectLen = RoundUpTo(MProtectLen, PageSize);
+#endif
----------------
Just make MProtectHelper do this itself.


================
Comment at: compiler-rt/lib/xray/xray_profile_collector.cc:21
 #include "xray_segmented_array.h"
+#include "xray_utils.h"
 #include <memory>
----------------
unused


================
Comment at: compiler-rt/lib/xray/xray_utils.cc:40
+#if SANITIZER_FUCHSIA
+static const char ProfileSinkName[] = "llvm-xray";
+
----------------
I'd use `inline constexpr const char* ProfileSinkName = "llvm-xray";`.
That makes the string constant mergeable.


================
Comment at: compiler-rt/lib/xray/xray_utils.cc:56
+  // Resize the VMO to ensure there's sufficient space for the data.
+  zx_status_t Status = _zx_vmo_set_size(Vmo, Offset + TotalBytes);
+  if (Status != ZX_OK) {
----------------
You can save the syscall when Offset % PAGE_SIZE == (Offset + TotalBytes) % PAGE_SIZE.


================
Comment at: compiler-rt/lib/xray/xray_utils.cc:72
+void LogWriter::Flush() XRAY_NEVER_INSTRUMENT {
+}
+
----------------
Comment: // Nothing to do here since WriteAll writes directly into the VMO.


================
Comment at: compiler-rt/lib/xray/xray_utils.cc:75
+LogWriter *openLog() XRAY_NEVER_INSTRUMENT {
+  // Get information about the current process.
+  zx_info_handle_basic_t Info;
----------------
Since this is just for the PID, I'd move it down next to where that's used and change the comment to say "Get the KOID of the current process to use in the VMO name."


================
Comment at: compiler-rt/lib/xray/xray_utils.cc:81
+  if (Status != ZX_OK) {
+    Report("XRay: cannot get information about the current process: %s\n",
+           _zx_status_get_string(Status));
----------------
I think it's more useful if this message says `zx_object_get_info(_zx_process_self(), ZX_INFO_HANDLE_BASIC) failed: %s`.


================
Comment at: compiler-rt/lib/xray/xray_utils.cc:100
+
+  // Duplicate the handle since __sanitizer_publish_data consumes it.
+  zx_handle_t Handle;
----------------
... and LogWriter needs to hold onto it.


================
Comment at: compiler-rt/lib/xray/xray_utils.cc:104
+  if (Status != ZX_OK) {
+    Report("XRay: cannot duplicate VMO: %s\n", _zx_status_get_string(Status));
+    return nullptr;
----------------
s/VMO/& handle/


================
Comment at: compiler-rt/lib/xray/xray_utils.cc:108
+
+  // Publish the VMO which contains profile data to the system.
+  __sanitizer_publish_data(ProfileSinkName, Handle);
----------------
// Publish the VMO that receives the logging.  Note the VMO's contents can grow and change after publication.  The contents won't be read out until after the process exits.


================
Comment at: compiler-rt/lib/xray/xray_utils.cc:112
+  // Use the dumpfile symbolizer markup element to write the name of VMO.
+  Report("XRay: {{{dumpfile:%s:%s}}}\n", ProfileSinkName, VmoName);
+
----------------
Put "{{{dumpfile:%s:%s}}}" into a macro in sanitizer_symbolizer_fuchsia.h and make sanitizer_coverage_fuchsia.cc and this both use it.
Unfortunately it can't be a constexpr like the others there because you need string concatenation with it.


================
Comment at: compiler-rt/lib/xray/xray_utils.cc:123
+}
+#else
 void printToStdErr(const char *Buffer) XRAY_NEVER_INSTRUMENT {
----------------
Comment after `#else` and `#endif` below.


================
Comment at: compiler-rt/lib/xray/xray_x86_64.cc:109
+#elif SANITIZER_FUCHSIA
+uint64_t getTSCFrequency() XRAY_NEVER_INSTRUMENT {
+    return static_cast<uint64_t>(zx_ticks_per_second());
----------------
This shouldn't be here.  On Fuchsia, all of this logic to read the TSC and so forth should not be used.
Instead, on all machines it should call _zx_ticks_get() to sample and _zx_ticks_per_second() to set CycleFrequency.



Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D52162