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

Ivan Krasin via llvm-commits llvm-commits at lists.llvm.org
Mon Nov 23 18:23:39 PST 2015


krasin added inline comments.

================
Comment at: tools/sancov/sancov.cc:227
@@ +226,3 @@
+  if (!STI) {
+    errs() << "error: no subtarget info for target " << TripleName << "\n";
+    return;
----------------
Why is lookupTarget error critical, while an error in createMCSubtargetInfo is not?

================
Comment at: tools/sancov/sancov.cc:249
@@ +248,3 @@
+      TheTarget->createMCDisassembler(*STI, Ctx));
+
+  if (!DisAsm) {
----------------
Please, remove this empty line. There's no empty line in the similar places above.

================
Comment at: tools/sancov/sancov.cc:262
@@ +261,3 @@
+  std::unique_ptr<const MCInstrAnalysis> MIA(
+      TheTarget->createMCInstrAnalysis(MII.get()));
+
----------------
Could createMCInstrAnalysis fail? If yes, please, add error handling as you did above.


================
Comment at: tools/sancov/sancov.cc:267
@@ +266,3 @@
+  for (const auto Section : O.sections()) {
+    if (Section.isVirtual() || !Section.isText())
+      continue;
----------------
A comment why do we skip virtual .text sections is appreated.

================
Comment at: tools/sancov/sancov.cc:275
@@ +274,3 @@
+    StringRef SectionName;
+    FailIfError(Section.getName(SectionName));
+
----------------
The more I think, the more it looks to me that an assert or CHECK should be used here. The problem is that asserts are disabled in some environments, and it would be unwise to proceed here.

================
Comment at: tools/sancov/sancov.cc:157
@@ +156,3 @@
+    auto InliningInfo = Symbolizer.symbolizeInlinedCode(ClBinaryName, Addr);
+    FailIfError(InliningInfo);
+    for (uint32_t i = 0; i < InliningInfo->getNumberOfFrames(); ++i) {
----------------
You may also consider to have a macro instead of a function, as it would allow to preserve the code of line. See for example: https://github.com/llvm-mirror/llvm/blob/358918e8dd375e41b4f51e7570a57e123b2951c6/unittests/Support/Path.cpp#L31

The macro in the example is gtest-specific, but it still gives an idea.

================
Comment at: tools/sancov/sancov.cc:295
@@ +294,3 @@
+        if (Target == SanCovAddr) {
+          // todo
+          Addrs->insert(Index + SectionAddr + Size - 1);
----------------
Please, add a better TODO description (if you wish to address some issue here later)

================
Comment at: tools/sancov/sancov.cc:296
@@ +295,3 @@
+          // todo
+          Addrs->insert(Index + SectionAddr + Size - 1);
+        }
----------------
A comment about the way you compute this address is appreciated.

================
Comment at: tools/sancov/sancov.cc:347
@@ +346,3 @@
+  for (const auto &FnLoc : FnLocs)
+    FilePrefix = CommonPrefix(FilePrefix, FnLoc.Loc.FileName);
+
----------------
What happens if you only have a single file? I have a feeling that the file name will be printed as an empty string.


http://reviews.llvm.org/D14889





More information about the llvm-commits mailing list