[PATCH] D16174: Introduce sanstats tool and llvm::CreateSanitizerStatReport function.

[Peter Collingbourne via llvm-commits]

pcc added inline comments.

================
Comment at: lib/Transforms/Utils/SanitizerStats.cpp:54
@@ +53,3 @@
+  case Triple::UnknownObjectFormat:
+    break;
+  }
----------------
krasin1 wrote:
> Is that a correct error handling for the unknown format? Should we warn the user? May be even fail hard?
It would probably be appropriate to call `report_fatal_error` here.

================
Comment at: tools/sanstats/sanstats.cpp:48
@@ +47,3 @@
+  const char *FilenameBegin = Begin;
+  while (Begin != End && *Begin)
+    ++Begin;
----------------
krasin1 wrote:
> isn't a reimplementation of strnlen?
Yes, but strnlen is not part of the C standard, and I think it's clearer to be explicit.

================
Comment at: tools/sanstats/sanstats.cpp:66
@@ +65,3 @@
+    Begin += SizeofPtr;
+    uint64_t Data = ReadLE(SizeofPtr, Begin, End);
+    Begin += SizeofPtr;
----------------
krasin1 wrote:
> At this point Begin could be greater than End. May be check in the previous line?
> Alternatively, make ReadLE to accept a pointer to Begin and let it to update it.
> At this point Begin could be greater than End. May be check in the previous line?

We defend against this in ReadLE. 

> Alternatively, make ReadLE to accept a pointer to Begin and let it to update it.

I considered this, but it's harder to distinguish between short reads and EOF this way.

================
Comment at: tools/sanstats/sanstats.cpp:127
@@ +126,3 @@
+  char SizeofPtr = *Begin++;
+  while (Begin != End) {
+    Begin = ReadModule(SizeofPtr, Begin, End);
----------------
krasin1 wrote:
> I highly suggest to check Begin < End and optionally check that Begin == End after the loop.
> 
> Otherwise, it's not hard to imaging that a tiny mistake somewhere would just Begin slightly past End and then we'll have to read after the end of the buffer until SEGFAULT stops this heroic attempt to overflow uintptr.
I added an assertion that should catch this.


http://reviews.llvm.org/D16174