[PATCH] D14889: sancov -not-covered-functions.

[Alexey Samsonov via llvm-commits]

samsonov added a comment.

Mostly fine, several nits below (in addition to Ivan's comments).

I don't quite agree on the change to LLVMObject, that method looks somewhat weird to me. We can probably discuss that separately: for now you seem to only support x86_64-linux anyway.


================
Comment at: lib/Object/ObjectFile.cpp:118
@@ +117,3 @@
+
+Triple ObjectFile::getTriple() const {
+  llvm::Triple TheTriple("unknown-unknown-unknown");
----------------
pcc wrote:
> I'd probably prefer if this function returned a `Target *` to reflect the fact that it isn't really supposed to (nor can it) return an accurate triple but rather something that can be used to summon appropriate bits of MC etc.
Agree: you can't rely deduce triple from object file. At the very least, you should reflect that you're guessing the triple in method name.

================
Comment at: tools/sancov/sancov.cc:102
@@ +101,3 @@
+
+void FailIfNotEmpty(const std::string &E) {
+  if (E.empty())
----------------
nit: please use "static" keyword consistently: it's not really needed for functions, as they are enclosed in anonymous namespace, but if you're willing to keep it - keep it everywhere.

================
Comment at: tools/sancov/sancov.cc:136
@@ +135,3 @@
+struct FunctionLoc {
+  bool operator<(const FunctionLoc &Rhs) const {
+    return std::tie(Loc, FunctionName) < std::tie(Rhs.Loc, Rhs.FunctionName);
----------------
Nit: LLVM tends to use `RHS` variant (as it's an abbreviation).

$ grep -rnH "RHS" lib/* | wc -l
10263
$ grep -rnH "Rhs" lib/* | wc -l
4

================
Comment at: tools/sancov/sancov.cc:210
@@ +209,3 @@
+  FailIfNotEmpty("__sanitizer_cov not found");
+  return 0; // not reachable.
+}
----------------
llvm_unreachable()

================
Comment at: tools/sancov/sancov.cc:352
@@ +351,3 @@
+
+  for (const FunctionLoc &FnLoc : FnLocs) {
+    std::string FileName = FnLoc.Loc.FileName;
----------------
TBH I don't understand the rationale behind stripping (calculated) common prefix. I can construct weird examples, e.g. imagine I have built my executable from /tmp/a.cc with function foo() and /tmp2/a.cc with function foo(). Suppose I suspect that when I ran my executable only one was executed. I call llvm-cov my.exe and see
  a.cc:1 foo()

Well, thanks :)

================
Comment at: tools/sancov/sancov.cc:358
@@ +357,3 @@
+
+    OS << FileName << ":" << FnLoc.Loc.Line << " " << FnLoc.FunctionName
+        << "\n";
----------------
Just FYI: file names may contain colons, as well as spaces. It's probably not a big deal, but just note that the output of llvm-cov might be not machine-readable.

================
Comment at: tools/sancov/sancov.cc:411
@@ -145,6 +410,3 @@
 
-    auto AddrsVector = llvm::make_unique<std::vector<uint64_t>>(
-        Addrs.begin(), Addrs.end());
-    return std::unique_ptr<CoverageData>(
-        new CoverageData(std::move(AddrsVector)));
+    return std::unique_ptr<CoverageData>(new CoverageData(std::move(Addrs)));
   }
----------------
`return llvm::make_unique<CoverageData>(std::move(Addrs));`

================
Comment at: tools/sancov/sancov.cc:439
@@ +438,3 @@
+  void printCoveredFunctions(raw_ostream &OS) {
+    printFunctionLocs(computeFunctionLocs(*Addrs.get()), OS);
+  }
----------------
Just `*Addrs`

================
Comment at: tools/sancov/sancov.cc:493
@@ -266,2 +492,2 @@
   }
 }
----------------
Add llvm_unreachable() at the end?


http://reviews.llvm.org/D14889