[sancov] __sanitizer_dump_coverage api

[aizatsky]

rebase

[kcc]

lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc

67

I don't think this code belongs here.

As I've mentioned before, we need to implement this using a public interface (which needs to be implemented).

We already have sanitizer_symbolize_pc which can give you the module and the offset,
but in an inefficient way (via converting to and from strings, etc).
Just need to implement a special interface, e.g.
int __sanitizer_get_module_and_offset_for_pc(uptr pc, char module_path[], uptr module_path_len, uptr *offset)

Symbolizer::FindModuleForAddress does what you need (or almost).
If you need more sorting/bsearching it belongs there or in LoadedModule::containsAddress

123

avoid comment like this, if possible.

[kcc]

I frankly don't remember your arguments and so I don't remember why (and if) I agreed.
Can you repeat the arguments?

[kcc]

[please reply in phab, it doesn't properly line up the replies done in e-mail :( ]

I already need this API in libFuzzer.
The API could be simple, as described above (sanitizer_get_module_and_offset_for_pc())

then let's design and implement it in a separate CL.

Yes. And test.

you wanted trace-pc-guard deployed as soon as possible.

Yes, but that not a good reason to duplicate the code, and the patch partially duplicates the existing code in sanitizer_common.

To do what you are asking me to do would require changing the way LoadedModules stores the information.

Probably yes.

[kcc]

Also, I suggest to implement the API w/o changing the internals, test it and submit.
Then see if it needs performance improvements.

[aizatsky]

next iteration

[aizatsky]

removed binary search.

[aizatsky]

Kostya,

Please take another look. This is now fully functional with test passing.

I can't use __sanitizer_get_module_and_offset_for_pc because it returns char* module_name, and organizing lookup by name is problematic.

[kcc]

please add a test with at least one (better, two) DSOs

lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc

10

Coverage dumper used with -fsanitize-coverage=tarce-pc-guards
(or some such)

56

I am confused.
I thought you were going to use __sanitizer_get_module_and_offset_for_pc here.

126

Hm... Why?
The function should be fully executed once for every DSO
(called once per every comp unit)

128

use CHECK_NE

141

technically speaking, this does not prevent us from a race of another sort:
we may have two threads running the same code and so they will increment pc_array_index twice for the same PC
and so you will get the same PC twice.

BTW, if something needs to be atomic, that's the *guard

I had another scheme in mind (and in the docs):

• allocate the array of PCs according to the amount of PCs deducted from __sanitizer_cov_trace_pc_guard_init.
• set guards to consecutive numbers starting from 1
• here do this uptr Idx = atomic_exchange(*guard, 0); if (Idx) pc_array[Idx] = pc;

test/sanitizer_common/TestCases/sanitizer_coverage_trace_pc_guard.cc

6 ↗

(On Diff #80832)

shouldn't you run this test in a separate scratch dir?
This way it may race with another test that creates sancov files.

10 ↗

(On Diff #80832)

Mmm. really? I thought I've fixed it.
I guess you need to fix it before submitting this test.

[kcc]

In D26758#617736, @aizatsky wrote:

Kostya,

Please take another look. This is now fully functional with test passing.

I can't use __sanitizer_get_module_and_offset_for_pc because it returns char* module_name, and organizing lookup by name is problematic.

You should not need to do any lookup by name.... See my other comments.

[aizatsky]

lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc

10

I'll have to rename the file then. I told me this will be the basis for all sanitizer coverage implementations in the future. Is this sanitizer-coverage_libcdep_tracepcguard.cc?

56

That's why I didn't want to create it in the first place. It is useless for me right now.

I suggest we change it to return module base address, not the 'pc - base'. This way I will be able to identify module by base address.

67

see above suggestion how we can modify public API to be useful here.

126

I have a check below in __sanitizer function. I'd prefer this detail to be handled over there.

141

this would create sparse pc_array. Not sure if we want this. Besides the code now is dead-simple. I like it this way.

You are right that sometimes we can get same PC twice, but I don't think we really care about this race. What's the problem of having some PC listed twice in .sancov file? Looks harmless.

[aizatsky]

fixing test, sorting pcs, using api only.

[aizatsky]

PTAL. Sancov test works, using only api functions now.

test/sanitizer_common/TestCases/sanitizer_coverage_trace_pc_guard.cc

10 ↗

(On Diff #80832)

Ahh. Missing -1 :)) Works now.

[kcc]

lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc

10

file name is fine.

12

remove comment

65

allow a special case of a zero pc (just ignore it)

85

is this one syscall per offset?
This actually may become a perf problem.
We need a single WriteToFile per file.

97

did you see my previous comment about DSOs?

test/sanitizer_common/TestCases/sanitizer_coverage_trace_pc_guard.cc

23 ↗

(On Diff #80847)

I thought I added a commend asking to extend the test to have DSOs, but I can't find it now.

[kcc]

lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc

141

what's wrong with sparse array?
Its size will be limited by the total number of PCs, i.e. known at the module load time.
And it will make the code even simpler and more correct.

[aizatsky]

multi-dso test; writing in a single call.

[aizatsky]

Added a dso test, writing in a single call.

[kcc]

lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc

111

We used exactly this approach in the previous implementation.
But I continue to think that instead we need to use an array indexed by the guard (from 1 to NumPCs), see my other unresolved comment.

[aizatsky]

using InternalMmapVector rather than fixed memory set.

[aizatsky]

format

[aizatsky]

Using InternalMmapVector for pc storage. PTAL.

[kcc]

lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc

119

hm... Why i + end - start?

The invariant here is that pc_vector.size() is max_guard_value + 1,
i.e. pc_vector[max_guard_value] is the last element.

123

Let's do proper atomics here.
I suggest

idx = atomic_exchange(guard, 0, memory_order_acquire);
if (!idx) return;
....

[kcc]

lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc

123

memory_order_relaxed, of course

[aizatsky]

atomic

lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc

119

obviously. The line was before the loop, I moved it below and forgot to fix it. Done.

[kcc]

lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc

119

Still hmmmm.
if the vector's size is 'i' we can't use pc_vector[i]

and just doing pc_vector.resize(i+1) won't work too, because next time you call it you will do

u32 i = pc_vector.size();

and so new 'i' will be old_i+1, which is not what you want.

I would decouple the counter and the vector size and just have another data member in TracePcGuardController

for (u32* p = start; p < end; p++) *p = ++num_guards;
pc_vector.resize(num_guards + 1);

[aizatsky]

lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc

119

I think this is exactly what I need. Let's say we call it twice. First time len=2, second time len=3.

1. i = 0, we write 1 & 2 to *p, size = 2 afterwards

2. i = 2, we write 3, 4, 5 to *p, size = 5
3. i = 5 we'll write 6 to guards.

[kcc]

lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc

119

i = 0, we write 1 & 2 to *p, size = 2 afterwards

But that's wrong! :)
size of pc_vector to be 3, because few lines later you do

pc_vector[idx] = pc;

where idx is either 1 or 2.

[aizatsky]

using idx - 1

lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc

119

I see what you mean. It's actually correct to do

pc_vector[idx - 1] = pc, since we start idx from 1. Fixed. I'll try to trigger it in the test.

[kcc]

LGTM

include/sanitizer/coverage_interface.h

 #ifdef __cplusplus
 extern "C" {
 #endif
 
   // Initialize coverage.
   void __sanitizer_cov_init();
   // Record and dump coverage info.
   void __sanitizer_cov_dump();
+
+  //  Dump collected coverage info. Sorts pcs by module into individual
+  //  .sancov files.
+  void __sanitizer_dump_coverage(const uintptr_t *pcs, uintptr_t len);
+
   // Open <name>.sancov.packed in the coverage directory and return the file
   // descriptor. Returns -1 on failure, or if coverage dumping is disabled.
   // This is intended for use by sandboxing code.
   intptr_t __sanitizer_maybe_open_cov_file(const char *name);
   // Get the number of unique covered blocks (or edges).
   // This can be useful for coverage-directed in-process fuzzers.
   uintptr_t __sanitizer_get_total_unique_coverage();
   // Get the number of unique indirect caller-callee pairs.

lib/sanitizer_common/CMakeLists.txt

 # SANITIZER_LIBCDEP_SOURCES when sanitizer_common library must not depend on
 # libc.
 set(SANITIZER_NOLIBC_SOURCES
   sanitizer_common_nolibc.cc)
 
 set(SANITIZER_LIBCDEP_SOURCES
   sanitizer_common_libcdep.cc
   sanitizer_coverage_libcdep.cc
+  sanitizer_coverage_libcdep_new.cc
   sanitizer_coverage_mapping_libcdep.cc
   sanitizer_linux_libcdep.cc
   sanitizer_posix_libcdep.cc
   sanitizer_stacktrace_libcdep.cc
   sanitizer_stoptheworld_linux_libcdep.cc
   sanitizer_symbolizer_libcdep.cc
   sanitizer_symbolizer_posix_libcdep.cc
   sanitizer_unwind_linux_libcdep.cc)

lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc

+//===-- sanitizer_coverage.cc ---------------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+// Sanitizer Coverage.
+
+#include "sanitizer_addrhashmap.h"
+#include "sanitizer_common.h"
+#include "sanitizer_symbolizer.h"
+
+using namespace __sanitizer;
+
+using AddressRange = LoadedModule::AddressRange;
+
+namespace {
+
+static const u64 Magic64 = 0xC0BFFFFFFFFFFF64ULL;
+static const u64 Magic32 = 0xC0BFFFFFFFFFFF32ULL;
+static const u64 Magic = SANITIZER_WORDSIZE == 64 ? Magic64 : Magic32;
+
+struct ModuleAndRange {
+  const LoadedModule* module;
+  const AddressRange* range;
+};
+
+struct CovFile {
+  CovFile() : fd(-1), num_offsets(0), file_path(0) {}
+
+  fd_t fd;
+  uptr num_offsets;
+  char* file_path;
+};
+
+};  // namespace
+
+static fd_t OpenFile(const char* path) {
+  error_t err;
+  fd_t fd = OpenFile(path, WrOnly, &err);
+  if (fd == kInvalidFd)
+    Report("SanitizerCoverage: failed to open %s for writing (reason: %d)\n",
+           path, err);
+  return fd;
+}
+
+static void GetCoverageFilename(char* path, const char* name,
+                                const char* extension) {
+  CHECK(name);
+  internal_snprintf(path, kMaxPathLength, "%s/%s.%zd.%s",
+                    common_flags()->coverage_dir, name, internal_getpid(),
+                    extension);
+}
+
+static void SanitizerDumpCoverage(const uptr* pcs, uptr len) {
+  auto symbolizer = Symbolizer::GetOrInit();
+  if (!symbolizer) return;
+
+  InternalMmapVector<fd_t> open_files(0);
+  using CovFilesMap = AddrHashMap<CovFile, 17>;
+  CovFilesMap cov_files;
+
+  ListOfModules modules;
+  modules.init();
+  InternalMmapVector<ModuleAndRange> ranges(modules.size());
+  for (const LoadedModule& module : modules) {
+    for (const AddressRange& r : module.ranges()) {
+      ranges.push_back(ModuleAndRange {&module, &r});
+    }
+  }
+  InternalSort(&ranges, ranges.size(),
+               [](const ModuleAndRange& m1, const ModuleAndRange& m2) {
+                 return m1.range->end < m2.range->end;
+               });
+
+  for (uptr i = 0; i < len; ++i) {
+    const uptr pc = pcs[i];
+    if (!pc)
+      continue;
+
+    uptr idx = InternalBinarySearch(
+        ranges, 0, ranges.size(), pc,
+        [](const ModuleAndRange& m, uptr pc) { return pc >= m.range->end; });
+    if (idx >= ranges.size()) {
+      Printf("ERROR: bad pc %x\n", pc);
+      continue;
+    }
+
+    const ModuleAndRange& module_and_range = ranges[idx];
+    auto module = module_and_range.module;
+    auto range = module_and_range.range;
+
+    if (range->beg > pc || range->end <= pc) {
+      Printf("ERROR: bad pc %x\n", pc);
+      continue;
+    }
+
+    CovFilesMap::Handle h(&cov_files, (uptr)&module);
+    if (h.created()) {
+      h->file_path = static_cast<char*>(InternalAlloc(kMaxPathLength));
+      GetCoverageFilename(h->file_path, StripModuleName(module->full_name()),
+                          "sancov");
+      h->fd = OpenFile(h->file_path);
+      WriteToFile(h->fd, &Magic, sizeof(Magic));
+    }
+
+    const uptr offset = pc - module->base_address();
+    h->num_offsets++;
+    WriteToFile(h->fd, &offset, sizeof(offset));
+  }
+
+  for (const LoadedModule& module : modules) {
+    CovFilesMap::Handle h(&cov_files, (uptr)&module);
+    if (h.created()) continue;
+    CloseFile(h->fd);
+    VReport(1, " CovDump: %s: %zd PCs written\n", h->file_path, h->num_offsets);
+    InternalFree(h->file_path);
+  }
+}
+
+// clang-format off
+SANITIZER_INTERFACE_ATTRIBUTE void __sanitizer_dump_coverage(  // NOLINT
+    const uptr* pcs, uptr len) {
+  return SanitizerDumpCoverage(pcs, len);
+}
+// clang-format on

lib/sanitizer_common/sanitizer_interface_internal.h

SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE void
       __sanitizer_sandbox_on_notify(__sanitizer_sandbox_arguments *args);
 
   // This function is called by the tool when it has just finished reporting
   // an error. 'error_summary' is a one-line string that summarizes
   // the error message. This function can be overridden by the client.
   SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE
   void __sanitizer_report_error_summary(const char *error_summary);
 
-  SANITIZER_INTERFACE_ATTRIBUTE void __sanitizer_cov_dump();
   SANITIZER_INTERFACE_ATTRIBUTE void __sanitizer_cov_init();
+  SANITIZER_INTERFACE_ATTRIBUTE void __sanitizer_cov_dump();
+  SANITIZER_INTERFACE_ATTRIBUTE void __sanitizer_dump_coverage(
+      const __sanitizer::uptr *pcs, const __sanitizer::uptr len);
+
   SANITIZER_INTERFACE_ATTRIBUTE void __sanitizer_cov(__sanitizer::u32 *guard);
   SANITIZER_INTERFACE_ATTRIBUTE
   void __sanitizer_annotate_contiguous_container(const void *beg,
                                                  const void *end,
                                                  const void *old_mid,
                                                  const void *new_mid);
   SANITIZER_INTERFACE_ATTRIBUTE
   int __sanitizer_verify_contiguous_container(const void *beg, const void *mid,
                                               const void *end);
   SANITIZER_INTERFACE_ATTRIBUTE
   const void *__sanitizer_contiguous_container_find_bad_address(
       const void *beg, const void *mid, const void *end);
   } // extern "C"
 
 #endif  // SANITIZER_INTERFACE_INTERNAL_H