[PATCH] D138846: MC/DC in LLVM Source-Based Code Coverage: LLVM back-end and compiler-rt

[Ellis Hoag via Phabricator via llvm-commits]

ellis added a comment.

Just did another quick pass of this diff and I'll take a look at the other diffs soon.



================
Comment at: compiler-rt/lib/profile/InstrProfiling.h:106-108
  * Read profile data form buffer \p Profile  and merge with in-process profile
- * counters. The client is expected to have checked or already knows the profile
- * data in the buffer matches the in-process counter structure before calling
- * it. Returns 0 (success) if the profile data is valid. Upon reading
- * invalid/corrupted profile data, returns 1 (failure).
+ * counters and bitmaps. The client is expected to have checked or already
+ * knows the profile data in the buffer matches the in-process counter
----------------



================
Comment at: llvm/include/llvm/IR/IntrinsicInst.h:1400
+/// A base class for instrprof mcdc intrinsics that require bitmask bytes.
+class InstrProfMCDCInstBase : public InstrProfInstBase {
+public:
----------------
I believe `dyn_cast<InstrProfMCDCInstBase>(I)` will return `nullptr` when `I` is `InstrProfMCDCParameters`, `InstrProfMCDCTVBitmapUpdate`, or `InstrProfMCDCCondBitmapUpdate` as it is right now. To fix this we need to implement `classof()` for `InstrProfMCDCInstBase` like in D141579. Just to avoid confusion in the future.


================
Comment at: llvm/include/llvm/IR/Intrinsics.td:620-622
+def int_instrprof_mcdc_parameters : Intrinsic<[],
+                                        [llvm_ptr_ty, llvm_i64_ty,
+                                         llvm_i32_ty]>;
----------------
NIT: Inconsistent formatting.


================
Comment at: llvm/lib/ProfileData/InstrProfReader.cpp:314
+    uint64_t NumBitmapBytes;
+    if ((Line++)->drop_front(1).getAsInteger(10, NumBitmapBytes))
+      return error(instrprof_error::malformed,
----------------
Why not set `Radix=0` so it can automatically infer the base? Same with the other `getAsInteger()` below. Then the user can specify these values in the most convenient form.

Also, I'm not sure if `getAsInteger()` will fail if there are spaces. If it does, should we call `.trim()` just before so that we can accept more readable inputs? For example, the tests could look something like this:

```
# Num Bitmask Bytes:
$ 2
# Bitmask Byte Values:
0x1d
0x0
```


================
Comment at: llvm/lib/ProfileData/InstrProfWriter.cpp:566
+    for (uint8_t Byte : Func.BitmapBytes) {
+      OS.write_hex(Byte);
+      OS << "\n";
----------------



================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:994-998
+  // Use internal rather than private linkage so the counter variable shows up   
+  // in the symbol table when using debug info for correlation.                  
+  if (DebugInfoCorrelate && TT.isOSBinFormatMachO() &&                           
+      Linkage == GlobalValue::PrivateLinkage)                                    
+    Linkage = GlobalValue::InternalLinkage; 
----------------
There seems to be extra whitespace changes here.


================
Comment at: llvm/test/tools/llvm-profdata/mcdc-bitmap.test:1
+# Test MC/DC bitmap reading and merging.
+
----------------
I just recently discovered `split-file` (https://llvm.org/docs/TestingGuide.html#extra-files). Since we only use the `.proftext` files in this test we can put their contents here so that everything is in one place. This will make this file much larger, so it's up to you if you think it's worth it.


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

https://reviews.llvm.org/D138846