[sancov] Accommodate sancov and coverage report server for use under Windows

[dgg5503]

Summary

This patch makes the following changes to SanCov and its complementary Python script in order to resolve issues pertaining to non-UNIX file paths in JSON symbolization information:

• Convert all paths to use forward slash.
• Update coverage-report-server.py to correctly handle paths to sources which contain spaces.
• Remove Linux platform restriction for all SanCov unit tests. All SanCov tests passed when ran on my local Windows machine.

Patch by Douglas Gliner.

[dgg5503]

Rebase over mono-repo.

[dgg5503]

Ping.

[dgg5503]

Ping.

[dgg5503]

Ping.

[dgg5503]

Ping.

[dgg5503]

Ping.

[dgg5503]

Ping.

[dgg5503]

Ping.

[dgg5503]

Ping.

[dgg5503]

Ping.

[vsk]

@Dor1s - any chance you know more folks actively working on sancov who have the bandwidth to review?

llvm/tools/sancov/sancov.cpp

351 ↗

(On Diff #213398)

Perhaps this whole function should be simplified to OS << json::Value(S) (see 'llvm/Support/JSON.h')?

[dgg5503]

@vsk thanks for the review! It looks like the JSON support library implements what JSONWriter does in this tool. To reduce maintenance, I've updated sancov to use the JSON support library implementation instead. The only downside to this change is that the JSON text format differs compared to the original implementation. I'm open to reverting this diff and simply adding your suggested change which also worked. Let me know what you think.

EDIT:
I've also updated the title and description to better describe the changes in this diff.

[Dor1s]

@Dor1s - any chance you know more folks actively working on sancov who have the bandwidth to review?

Added Matt and Vitaly from "sanitizers" team, + Jonathan who has Windows expertise.

[vitalybuka]

In D51018#1700811, @dgg5503 wrote:

@vsk thanks for the review! It looks like the JSON support library implements what JSONWriter does in this tool. To reduce maintenance, I've updated sancov to use the JSON support library implementation instead. The only downside to this change is that the JSON text format differs compared to the original implementation. I'm open to reverting this diff and simply adding your suggested change which also worked. Let me know what you think.

EDIT:
I've also updated the title and description to better describe the changes in this diff.

I like the change.

Could you move JSONWriter -> JSON refactoring into separate patch and rebase win stuff ontop?
If you don't have commiter access, someone will need to commit it for you
So I don't mind to split the patch and commit myself.

[dgg5503]

In D51018#1702330, @vitalybuka wrote:

In D51018#1700811, @dgg5503 wrote:

@vsk thanks for the review! It looks like the JSON support library implements what JSONWriter does in this tool. To reduce maintenance, I've updated sancov to use the JSON support library implementation instead. The only downside to this change is that the JSON text format differs compared to the original implementation. I'm open to reverting this diff and simply adding your suggested change which also worked. Let me know what you think.

EDIT:
I've also updated the title and description to better describe the changes in this diff.

I like the change.

Could you move JSONWriter -> JSON refactoring into separate patch and rebase win stuff ontop?
If you don't have commiter access, someone will need to commit it for you
So I don't mind to split the patch and commit myself.

Sure thing! Would I be creating a separate diff for review or would this be taken from the history? Also, I do not have commiter access so I will need someone to commit it for me.

[vitalybuka]

In D51018#1702476, @dgg5503 wrote:

In D51018#1702330, @vitalybuka wrote:

In D51018#1700811, @dgg5503 wrote:

@vsk thanks for the review! It looks like the JSON support library implements what JSONWriter does in this tool. To reduce maintenance, I've updated sancov to use the JSON support library implementation instead. The only downside to this change is that the JSON text format differs compared to the original implementation. I'm open to reverting this diff and simply adding your suggested change which also worked. Let me know what you think.

EDIT:
I've also updated the title and description to better describe the changes in this diff.

I like the change.

Could you move JSONWriter -> JSON refactoring into separate patch and rebase win stuff ontop?
If you don't have commiter access, someone will need to commit it for you
So I don't mind to split the patch and commit myself.

Sure thing! Would I be creating a separate diff for review or would this be taken from the history? Also, I do not have commiter access so I will need someone to commit it for me.

yes, you need to upload new patch with "arc diff"
then you can chain them with "Edit Related Revisions..." on this site

[dgg5503]

Split out JSON changes to D68752.

[dgg5503]

@vitalybuka thanks for the explanation. I believe I did it correctly, please let me know otherwise. It is my first time submitting a change to the LLVM project!

In other news, I've slightly modified the test symbolize.test to actually test the case I present in the initial description.

[metzman]

I don't consider myself a Windows expert but I don't see anything problematic from a Windows point of view.

[vitalybuka]

Reverted https://reviews.llvm.org/rL374636

[dgg5503]

@vitalybuka I plan to debug this failure as soon as I can. I imagine I'd have to use QEmu to emulate ppc64/s390x since I don't have such a machine on hand. Thanks for committing my other diff though!

[vitalybuka]

In D51018#1709815, @dgg5503 wrote:

@vitalybuka I plan to debug this failure as soon as I can. I imagine I'd have to use QEmu to emulate ppc64/s390x since I don't have such a machine on hand. Thanks for committing my other diff though!

I suspect it never worked on these platform and we just enabled the test with x86-registered-target

[dgg5503]

So I've finally taken the time to figure out why these tests fail on s390x using QEMU. When .sancov files are loaded into memory and the header is checked in isCoverageFile, the check fails presumably due to endianess. For example, the sancov header magic for test-linux_x86_64.0.sancov is expected to be 0xC0BFFFFF but is read as ‭0xFFFFBFC0‬ on big endian systems. I assume the same issue is causing the tests to fail when ran on ppc64be. I am not at all familiar with how other parts of LLVM handle endian differences when reading files from memory using MemoryBuffer::getFile or equivalent. I'd guess there'd be a portable method since llvm-objdump -d /test/tools/sancov/Inputs/test-linux_x86_64 returns the identical output when ran on both s390x and x86_64, but perhaps there are other factors in play that I'm overlooking. What are the expectations regarding cross platform compatibility with sancov? Is it reasonable to expect sancov to be able to parse and provide coverage information for any .sancov and binary compiled for a target supported by LLVM on any platform? Or should users be expected to use .sancov files on the same host that generated them?

For reference, all of these tests passed on Windows when parsing the Linux sancov test file. So I guess what I'm really asking is, should we expect "cross-endian" support? If not, I wouldn't mind adding some .sancov binary files created on a big endian system just so these tests pass accordingly.

[dgg5503]

Require little-endian byte order when executing tests which read in byte order sensitive .sancov files. This should prevent LIT test failure on big-endian systems such as s390x which is what caused this commit to be reverted as per https://reviews.llvm.org/rL374636.

[vitalybuka]

llvm/test/tools/sancov/blacklist.test

1 ↗

(On Diff #261526)

In cases like this we usually put spaces around &&

[dgg5503]

Added spaces around "&&".

[dgg5503]

@vitalybuka thanks for the approval! Can someone with commit access please push these changes?

[dgg5503]

Ping.

test/tools/sancov/blacklist.test

-REQUIRES: x86_64-linux
+REQUIRES: x86-registered-target
 RUN: sancov -covered-functions %p/Inputs/test-linux_x86_64 %p/Inputs/test-linux_x86_64.0.sancov | FileCheck %s --check-prefix=ALL
 RUN: sancov -covered-functions -blacklist %p/Inputs/fun_blacklist.txt %p/Inputs/test-linux_x86_64 %p/Inputs/test-linux_x86_64.0.sancov | FileCheck %s
 RUN: sancov -covered-functions -blacklist %p/Inputs/src_blacklist.txt %p/Inputs/test-linux_x86_64 %p/Inputs/test-linux_x86_64.1.sancov | FileCheck --check-prefix=CHECK1 %s
 RUN: sancov -print-coverage-stats %p/Inputs/test-linux_x86_64 %p/Inputs/test-linux_x86_64.1.sancov | FileCheck --check-prefix=STATS %s
 RUN: sancov -print-coverage-stats -blacklist %p/Inputs/fun_blacklist.txt %p/Inputs/test-linux_x86_64 %p/Inputs/test-linux_x86_64.1.sancov | FileCheck --check-prefix=STATS-BLIST %s
 
 ALL: test.cpp:12 bar(std::string)
 ALL: test.cpp:14 main

test/tools/sancov/covered_functions.test

-REQUIRES: x86_64-linux
+REQUIRES: x86-registered-target
 RUN: sancov -covered-functions %p/Inputs/test-linux_x86_64 %p/Inputs/test-linux_x86_64.0.sancov | FileCheck %s
 RUN: sancov -covered-functions -strip_path_prefix=Inputs/ %p/Inputs/test-linux_x86_64 %p/Inputs/test-linux_x86_64.0.sancov | FileCheck --check-prefix=STRIP_PATH %s
 RUN: sancov -demangle=0 -covered-functions %p/Inputs/test-linux_x86_64 %p/Inputs/test-linux_x86_64.0.sancov | FileCheck --check-prefix=NO_DEMANGLE %s
 
 CHECK: Inputs{{[/\\]}}test.cpp:12 bar(std::string)
 CHECK: Inputs{{[/\\]}}test.cpp:14 main
 
 STRIP_PATH: {{^}}test.cpp:12 bar(std::string)
 STRIP_PATH: {{^}}test.cpp:14 main
 
 NO_DEMANGLE: test.cpp:12 _Z3barSs
 NO_DEMANGLE: test.cpp:14 main

test/tools/sancov/merge.test

-REQUIRES: x86_64-linux
+REQUIRES: x86-registered-target
 RUN: sancov -merge %p/Inputs/test-linux_x86_64.0.symcov| FileCheck --check-prefix=MERGE1 %s
 RUN: sancov -merge %p/Inputs/test-linux_x86_64.0.symcov %p/Inputs/test-linux_x86_64.1.symcov| FileCheck --check-prefix=MERGE2 %s
 
 MERGE1: {
 MERGE1-NEXT:   "covered-points" : ["4e132b", "4e1472", "4e1520", "4e1553", "4e1586"],
 MERGE1-NEXT:   "binary-hash" : "BB3CDD5045AED83906F6ADCC1C4DAF7E2596A6B5",
 MERGE1-NEXT:   "point-symbol-info" : {
 MERGE1-NEXT:     "test/tools/sancov/Inputs/foo.cpp" : {

test/tools/sancov/not_covered_functions.test

-REQUIRES: x86_64-linux
+REQUIRES: x86-registered-target
 RUN: sancov -skip-dead-files=0 -not-covered-functions %p/Inputs/test-linux_x86_64 %p/Inputs/test-linux_x86_64.0.sancov | FileCheck %s
 RUN: sancov -not-covered-functions %p/Inputs/test-linux_x86_64 %p/Inputs/test-linux_x86_64.1.sancov | FileCheck --check-prefix=CHECK1 --allow-empty %s
 
 CHECK: Inputs{{[/\\]}}foo.cpp:5 foo()
 CHECK-NOT: {{.*__sanitizer.*}}
 CHECK1-NOT: {{.+}}
 

test/tools/sancov/print.test

-REQUIRES: x86_64-linux
+REQUIRES: x86-registered-target
 RUN: sancov -print %p/Inputs/test-linux_x86_64.0.sancov | FileCheck %s
 
 CHECK: 0x4e132b
 CHECK: 0x4e1472
 CHECK: 0x4e1520
 CHECK: 0x4e1553
 CHECK: 0x4e1586
 

test/tools/sancov/stats.test

-REQUIRES: x86_64-linux
+REQUIRES: x86-registered-target
 RUN: sancov -print-coverage-stats %p/Inputs/test-linux_x86_64 %p/Inputs/test-linux_x86_64.0.sancov | FileCheck %s
 
 CHECK: all-edges: 8
 CHECK: cov-edges: 5
 CHECK: all-functions: 2
 CHECK: cov-functions: 2
 
 

test/tools/sancov/symbolize.test

-REQUIRES: x86_64-linux
+REQUIRES: x86-registered-target
 RUN: sancov -symbolize -strip_path_prefix="llvm/" %p/Inputs/test-linux_x86_64 %p/Inputs/test-linux_x86_64.0.sancov | FileCheck %s
 
 CHECK: {
 CHECK-NEXT:  "covered-points" : ["4e132b", "4e1472", "4e1520", "4e1553", "4e1586"],
 CHECK-NEXT:  "binary-hash" : "BB3CDD5045AED83906F6ADCC1C4DAF7E2596A6B5",
 CHECK-NEXT:  "point-symbol-info" : {
 CHECK-NEXT:   "test/tools/sancov/Inputs/test.cpp" : {
 CHECK-NEXT:     "bar(std::string)" : {

test/tools/sancov/symbolize_noskip_dead_files.test

-REQUIRES: x86_64-linux
+REQUIRES: x86-registered-target
 RUN: sancov -symbolize -skip-dead-files=0 -strip_path_prefix="llvm/" %p/Inputs/test-linux_x86_64 %p/Inputs/test-linux_x86_64.0.sancov | FileCheck %s
 
 CHECK: {
 CHECK-NEXT:  "covered-points" : ["4e132b", "4e1472", "4e1520", "4e1553", "4e1586"],
 CHECK-NEXT:  "binary-hash" : "BB3CDD5045AED83906F6ADCC1C4DAF7E2596A6B5",
 CHECK-NEXT:  "point-symbol-info" : {
 CHECK-NEXT:    "test/tools/sancov/Inputs/foo.cpp" : {
 CHECK-NEXT:      "foo()" : {

test/tools/sancov/validation.test

-REQUIRES: x86_64-linux
+REQUIRES: x86-registered-target
 RUN: not sancov -covered-functions %p/Inputs/test-linux_x86_64 2>&1 | FileCheck --check-prefix=NOCFILE %s
 
 NOCFILE: WARNING: No coverage file for {{.*}}test-linux_x86_64
 NOCFILE: ERROR: No valid coverage files given.
 

tools/sancov/coverage-report-server.py

 import http.server
 import json
 import socketserver
 import time
 import html
 import os
 import string
 import math
+import urllib
 
 INDEX_PAGE_TMPL = """
 <html>
 <head>
   <title>Coverage Report</title>
   <style>
     .lz { color: lightgray; }
   </style>
if zeroes:
         zeroes = '<span class="lz">{0}</span>'.format(zeroes)
     return zeroes + pct_str
 
 class ServerHandler(http.server.BaseHTTPRequestHandler):
     symcov_data = None
     src_path = None
 
     def do_GET(self):
+        norm_path = os.path.normpath(urllib.parse.unquote(self.path[1:]))
         if self.path == '/':
             self.send_response(200)
             self.send_header("Content-type", "text/html; charset=utf-8")
             self.end_headers()
 
             filelist = []
             for filename in sorted(self.symcov_data.filenames()):
                 file_coverage = self.symcov_data.file_coverage[filename]
                 if not file_coverage:
                     continue
                 filelist.append(
                         "<tr><td><a href=\"./{name}\">{name}</a></td>"
                         "<td>{coverage}%</td></tr>".format(
                             name=html.escape(filename, quote=True), 
                             coverage=format_pct(file_coverage)))
 
             response = string.Template(INDEX_PAGE_TMPL).safe_substitute(
                 filenames='\n'.join(filelist))
             self.wfile.write(response.encode('UTF-8', 'replace'))
-        elif self.symcov_data.has_file(self.path[1:]):
-            filename = self.path[1:]
+        elif self.symcov_data.has_file(norm_path):
+            filename = norm_path
             filepath = os.path.join(self.src_path, filename) 
             if not os.path.exists(filepath):
                 self.send_response(404)
                 self.end_headers()
                 return
 
             self.send_response(200)
             self.send_header("Content-type", "text/html; charset=utf-8")

tools/sancov/sancov.cpp

public:
   };
 
   std::unique_ptr<Array> array() { return make_unique<Array>(OS); }
 
 private:
   void indent() { OS.indent(Indent * 2); }
 
   static void printJSONStringLiteral(StringRef S, raw_ostream &OS) {
-    if (S.find('"') == std::string::npos) {
+    if (S.find('"') == std::string::npos &&
+        S.find('\\') == std::string::npos) {
       OS << "\"" << S << "\"";
       return;
     }
     OS << "\"";
     for (char Ch : S.bytes()) {
       if (Ch == '"')
         OS << "\\";
+      else if (Ch == '\\')
+        OS << "\\";
       OS << Ch;
     }
     OS << "\"";
   }
 
   raw_ostream &OS;
   int Indent = 0;
 };
static std::unique_ptr<symbolize::LLVMSymbolizer> createSymbolizer() {
   SymbolizerOptions.UseSymbolTable = true;
   return std::unique_ptr<symbolize::LLVMSymbolizer>(
       new symbolize::LLVMSymbolizer(SymbolizerOptions));
 }
 
 static std::string normalizeFilename(const std::string &FileName) {
   SmallString<256> S(FileName);
   sys::path::remove_dots(S, /* remove_dot_dot */ true);
-  return stripPathPrefix(S.str().str());
+  return stripPathPrefix(sys::path::convert_to_slash(S.str()));
 }
 
 class Blacklists {
 public:
   Blacklists()
       : DefaultBlacklist(createDefaultBlacklist()),
         UserBlacklist(createUserBlacklist()) {}