[PATCH] [asancov] Write coverage directly to a memory-mapped file

[Evgeniy Stepanov]

================
Comment at: lib/sanitizer_common/sanitizer_coverage_libcdep.cc:94
@@ +93,3 @@
+void CoverageData::DirectInit() {
+  InternalScopedString path(1024);
+  internal_snprintf((char *)path.data(), path.size(), "%zd.sancov.raw",
----------------
Dmitry Vyukov wrote:
> same comment as for sancov.map
done

================
Comment at: lib/sanitizer_common/sanitizer_coverage_libcdep.cc:129
@@ +128,3 @@
+  uptr size =
+      atomic_fetch_add(&pc_array_size, delta, memory_order_relaxed) + delta;
+  uptr mapped_size = atomic_load(&pc_array_mapped_size, memory_order_relaxed);
----------------
Dmitry Vyukov wrote:
> You need to increment it after MapWritableFileToMemory. Otherwise the CHECK in Add does not work.
done

================
Comment at: lib/sanitizer_common/sanitizer_coverage_libcdep.cc:130
@@ +129,3 @@
+      atomic_fetch_add(&pc_array_size, delta, memory_order_relaxed) + delta;
+  uptr mapped_size = atomic_load(&pc_array_mapped_size, memory_order_relaxed);
+  if (size <= mapped_size) return;
----------------
Dmitry Vyukov wrote:
> Why do you need this to be atomic? It's accessed only in Init or under the mutex.
done

================
Comment at: lib/sanitizer_common/sanitizer_coverage_libcdep.cc:138
@@ +137,3 @@
+  uptr res = internal_ftruncate(pc_fd, new_mapped_size);
+  CHECK_EQ(res, 0);
+  void *p = MapWritableFileToMemory(pc_array + mapped_size,
----------------
Dmitry Vyukov wrote:
> please print a readable error message with errno and Die
> this is not a programming error, it can happen due to out of disk space of networking error
done

================
Comment at: lib/sanitizer_common/sanitizer_coverage_libcdep.cc:153
@@ -66,1 +152,3 @@
+  CHECK_LT(idx * sizeof(uptr),
+           atomic_load(&pc_array_size, memory_order_acquire));
   pc_array[idx] = pc;
----------------
Dmitry Vyukov wrote:
> memory_order_acquire in CHECK looks suspicious
> I think it needs to be just memory_order_relaxed
But then release-store to pc_array_size has no corresponding acquire-load.
Looks fine to me: this ordering is only needed for the CHECK to work, fetch_add one line above can be relaxed.

================
Comment at: lib/sanitizer_common/sanitizer_coverage_mapping_libcdep.cc:43
@@ +42,3 @@
+
+  InternalScopedString path(1024);
+  internal_snprintf((char *)path.data(), path.size(), "%zd.sancov.map",
----------------
Dmitry Vyukov wrote:
> It's just a "zd.sancov.map" string, right?
> 64 would be enough. 1024 makes me think that it's a full path, while it is not.
done. but I'll increase it back to 1024 with the next commit anyway.

================
Comment at: lib/sanitizer_common/sanitizer_coverage_mapping_libcdep.cc:46
@@ +45,3 @@
+                    internal_getpid());
+  uptr map_fd = OpenFile(path.data(), true);
+  if (internal_iserror(map_fd)) {
----------------
Dmitry Vyukov wrote:
> If the process is killed in the middle of this function, then we will get broken cov profile. And working in the presence of sudden kills was the main motivation for this changes.
> I would write the data to ancov.map.tmp and then rename it to ancov.map.
> 
done

================
Comment at: lib/sanitizer_common/sanitizer_flags.cc:133
@@ -130,1 +132,3 @@
+            "If set, coverage information will be dumped directly to a memory "
+            "mapped file.");
   ParseFlag(str, &f->full_address_space, "full_address_space",
----------------
Dmitry Vyukov wrote:
> please explain when it can be useful (if the process is suddenly killed, it will leave some valid coverage file)
done

http://reviews.llvm.org/D3874