[PATCH] D108942: [compiler-rt] Add more diagnostic to InstrProfError

[Petr Hosek via Phabricator via llvm-commits]

phosek added a comment.

I think this is a great change but the messages could be improved. Saying that value is corrupted in every case doesn't really say much plus it's not clear if the value has really been corrupted, there are other ways in which the data can become invalid (like using incompatible runtime version or compiler error). Rather I'd try to explain what's the issue in concise terms. I left a few suggestions, hopefully you'll get the point.



================
Comment at: llvm/include/llvm/ProfileData/InstrProf.h:325
   instrprof_error get() const { return Err; }
+  std::string getErrMsg() const { return ErrMsg; }
 
----------------
I'd call this just `getMessage` which is what `StringError` does as well.


================
Comment at: llvm/include/llvm/ProfileData/InstrProf.h:342
   instrprof_error Err;
+  std::string ErrMsg;
 };
----------------
I'd call this just `Msg` which is what `StringError` does as well.


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp:572
+            instrprof_error::malformed,
+            "Function name is empty in CovMapFuncRecordReader");
       ++CovMapNumUsedRecords;
----------------
I'd omit the `CovMapFuncRecordReader` which may not be useful for most users.


================
Comment at: llvm/lib/ProfileData/InstrProf.cpp:79
+                                         const std::string &ErrMsg = "") {
+  std::string ErrStr;
   switch (Err) {
----------------
I'd use `raw_string_ostream` to assemble the message. This would like like this:

```
std::string Msg;
raw_string_ostream OS(Msg);
switch (Err) {
case instrprof_error::success:
  OS << "success";
  break;
...
}
...
return OS.str();
```


================
Comment at: llvm/lib/ProfileData/InstrProf.cpp:150
+  if (!ErrMsg.empty())
+    ErrStr = ErrStr + " (" + ErrMsg + ")";
+  return ErrStr;
----------------
Some of the error messages above already include details in parenthesis and having multiple parenthesis might look awkward so I'd use `:` as a separator instead.


================
Comment at: llvm/lib/ProfileData/InstrProf.cpp:911
+        instrprof_error::malformed,
+        "Integrity check failure: Number of value profile kinds is corrupted");
   // Total size needs to be mulltiple of quadword size.
----------------
These are not full sentences so they shouldn't be capitalized, the same for all the other messages below.. I'd also consider moving the `Integrity check failure` at the end in parenthesis as an additional qualifier.

Regarding this particular error message, I think you can make it even more specific (corrupted is too generic) by saying something like "number of value profile kinds is invalid". 


================
Comment at: llvm/lib/ProfileData/InstrProf.cpp:916
+        instrprof_error::malformed,
+        "Integrity check failure: Value profile data total size is corrupted");
 
----------------
Rather than corrupted, I'd say something like "total size is not congruent modulo quardword size".


================
Comment at: llvm/lib/ProfileData/InstrProf.cpp:923
+          instrprof_error::malformed,
+          "Integrity check failure: Value kind is corrupted");
     VR = getValueProfRecordNext(VR);
----------------
I'd use "value kind is invalid" not corrupted.


================
Comment at: llvm/lib/ProfileData/InstrProf.cpp:928
+          instrprof_error::malformed,
+          "Integrity check failure: Value profile record is corrupted");
   }
----------------
Maybe something like "address of the next value profile record is greater than the total size".


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108942/new/

https://reviews.llvm.org/D108942