[PATCH] [sanitizer] Support sandboxing in sanitizer coverage.

[Alexander Potapenko]

================
Comment at: include/sanitizer/common_interface_defs.h:27
@@ -26,1 +26,3 @@
 #endif
+  struct __sanitizer_sandbox_arguments {
+    int coverage_sandboxed;
----------------
I think we need some "version" parameter to distinguish between different versions of this structure.

================
Comment at: include/sanitizer/common_interface_defs.h:30
@@ +29,3 @@
+    int coverage_fd;
+    size_t coverage_max_block_size;
+  };
----------------
Should this really be passed through the sandbox arguments? I don't think the clients need to care much.

================
Comment at: lib/sanitizer_common/sanitizer_coverage.cc:46
@@ -45,3 +45,3 @@
 // pc_array is the array containing the covered PCs.
 // To make the pc_array thread- and AS- safe it has to be large enough.
 // 128M counters "ought to be enough for anybody" (4M on 32-bit).
----------------
While at it, can you please replace "AS- safe" with "async-signal-safe"?

================
Comment at: lib/sanitizer_common/sanitizer_coverage.cc:55
@@ +54,3 @@
+static bool cov_sandboxed = false;
+static int cov_fd = -1;
+static uptr cov_max_block_size = 0;
----------------
Please use kInvalidFd.

================
Comment at: lib/sanitizer_common/sanitizer_coverage.cc:80
@@ +79,3 @@
+  int pid;
+  unsigned int module_length;
+  uptr data_size;
----------------
This should be u32, like the offsets.
I'd even suggest to define something like module_off_t in this file.

================
Comment at: lib/sanitizer_common/sanitizer_coverage.cc:96
@@ +95,3 @@
+    internal_write(cov_fd, &header, sizeof(header));
+    internal_write(cov_fd, module, module_length);
+    internal_write(cov_fd, blob, blob_size);
----------------
I think it's more common to keep the trailing zeroes, YMMV.

================
Comment at: lib/sanitizer_common/sanitizer_coverage.cc:105
@@ +104,3 @@
+    uptr max_payload_size = cov_max_block_size - header_size_with_module;
+    char *block_bytes = block.data();
+    internal_memcpy(block_bytes, &header, sizeof(header));
----------------
s/block_bytes/block_pos ?

================
Comment at: lib/sanitizer_common/sanitizer_coverage.cc:116
@@ +115,3 @@
+      blob_bytes += payload_size;
+      // Note: this assumes proper alignment of block.data().
+      ((CovHeader *)block.data())->data_size = payload_size;
----------------
Please define "proper"

================
Comment at: lib/sanitizer_common/sanitizer_coverage.cc:159
@@ -111,1 +158,3 @@
+                       offsets.size() * sizeof(u32));
+        VReport(1, " CovDump: %zd PCs written to packed file\n", vb - old_vb);
       } else {
----------------
Are you adding the leading whitespaces on purpose (here and below)? Why?

================
Comment at: lib/sanitizer_common/sanitizer_coverage.cc:183
@@ +182,3 @@
+static void OpenPackedFileForWriting() {
+  CHECK(cov_fd < 0);  // NOLINT
+  InternalScopedBuffer<char> path(1024);
----------------
CHECK(cov_fd != kInvalidFd), remove NOLINT.

================
Comment at: lib/sanitizer_common/sanitizer_internal_defs.h:98
@@ +97,3 @@
+      __sanitizer::uptr coverage_max_block_size;
+  };
+
----------------
You shouldn't need to redefine this struct.

================
Comment at: lib/sanitizer_common/scripts/sancov.py:48
@@ -46,1 +47,3 @@
 
+def UnpackOneFile(path):
+  with open(path, mode="rb") as f:
----------------
Two lines between the function declarations, please.

http://reviews.llvm.org/D3726