[BitcodeAnalyzer] allow a motivated user to dump BLOCKINFO

[woodruffw]

Summary

This adds the --dump-blockinfo flag to llvm-bcanalyzer, allowing a sufficiently motivated user to dump (parts of) the BLOCKINFO_BLOCK block. The default behavior is unchanged, and --dump-blockinfo only takes effect in the same context as other flags that control dump behavior (i.e., requires that --dump is also passed).

[woodruffw]

Sorry for the churn; rebased the commit so that it sits on the latest main instead of an old one.

[woodruffw]

Fixed the diff tests.

[woodruffw]

Gentle ping for review on this!

[woodruffw]

Another gentle ping for review here. Please let me know if there's anything else I can do.

[woodruffw]

Another ping for review.

[xgupta]

To make it convenient to get reviewing, please upload the patches with context, see documentation - https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-command-line
git show HEAD -U999999 > mypatch.patch or arc diff HEAD~

[woodruffw]

Added context to the patch.

[woodruffw]

Thanks! I've updated the diff to include context.

[woodruffw]

Gentle ping for review on this!

[tejohnson]

Please add a test. Maybe in llvm/test/tools/llvm-bcanalyzer/ ?

llvm/lib/Bitcode/Reader/BitcodeAnalyzer.cpp

747

Why this change?

[woodruffw]

llvm/lib/Bitcode/Reader/BitcodeAnalyzer.cpp

747

Without it, the dumper will prematurely emit the end of the BLOCKINFO_BLOCK.

There are two cases:

• Not dumping the BLOCKINFO_BLOCK (the default): emit an empty XML-element-thingy
• Dumping the BLOCKINFO_BLOCK: actually visit the BLOCKINFO_BLOCK, and let its dumper handle the fully XML-element-thingy

[woodruffw]

Added a test; made some small fixes to the --help output and file comments.

llvm/test/tools/llvm-bcanalyzer/dump-blockinfo.test

1 ↗

(On Diff #376797)

N.B.: Please let me know if using llvm-stress like this isn't advisable; I can check in a test input instead if that's preferred.

[xgupta]

Yes, we don't use llvm-stress in testcases. I think you take input .bc file from llvm/test/Other/Inputs. Other than that testcase, changes LGTM but @mehdi_amini might give it final approval.

Screenshot of the output of --dump-blockinfo flag.

[tejohnson]

llvm/test/tools/llvm-bcanalyzer/dump-blockinfo.test

1 ↗

(On Diff #376797)

Can you make this a small .ll test instead (see examples in other test directories) and just run it through opt to get a bitcode and then run llvm-bcanalyzer on that?

And then is it possible to check some of the expected contents of the blockinfo section?

[mehdi_amini]

In D107536#3052989, @xgupta wrote:

Yes, we don't use llvm-stress in testcases. I think you take input .bc file from llvm/test/Other/Inputs. Other than that testcase, changes LGTM but @mehdi_amini might give it final approval.

Thanks for pulling me in, but I really haven't worked in this area for a long time :)
If you have a hard time finding a reviewer you can always ping the code owner for bitcode and ask them to help find one.

Happy to help thought: so LGTM with the inline comments addressed!

llvm/lib/Bitcode/Reader/BitcodeAnalyzer.cpp

762

llvm/test/tools/llvm-bcanalyzer/dump-blockinfo.test

1 ↗

(On Diff #376797)

What Theresa suggests makes sense to me.

[woodruffw]

Avoid llvm-stress in test, more detailed checks.

llvm/lib/Bitcode/Reader/BitcodeAnalyzer.cpp

762

O is Optional<BCDumpOptions> here, so I think we need the O && ... check here.

When I remove it, running llvm-bcanalyzer with no options (i.e. just llvm-bcanalyzer foo.bc) causes an abort in the Optional.

llvm/test/tools/llvm-bcanalyzer/dump-blockinfo.test

1 ↗

(On Diff #376797)

Done!

[tejohnson]

llvm/test/tools/llvm-bcanalyzer/dump-blockinfo.test

1 ↗

(On Diff #376797)

Generally we avoid using bitcode directly as a test input, because the format can change. What I would suggest is to llvm-dis that bitcode into a .ll file. Then just make that the test file with the command and tests in it. I.e. rather than a .test extension like this, have something like a llvm/test/tools/llvm-bcanalyzer/dump-blockinfo.ll test that contains your llvm-dis'ed bitcode file, then add commands in it like:

; RUN: llvm-as < %s | llvm-bcanalyzer --dump --dump-blockinfo | FileCheck %s

; CHECK: ...

[woodruffw]

llvm/test/tools/llvm-bcanalyzer/dump-blockinfo.test

1 ↗

(On Diff #376797)

Unfortunately, I think we need the direct bitcode input here: LLVM's bitcode writer is allowed to make arbitrary abbreviation decisions in the BLOCKINFO_BLOCK, so round-tripping through the textual form isn't guaranteed to produce the same records in BLOCKINFO_BLOCK (or even a BLOCKINFO_BLOCK at all).

(Separately, the test input I'm using here isn't valid LLVM IR, since it's just a bitstream fragment. I copied it from llvm/test/Other/Inputs per @xgupta's example, where it was already separately checked in.)

[mehdi_amini]

llvm/lib/Bitcode/Reader/BitcodeAnalyzer.cpp

762

I don't quite get why, because the block is guarded by:

if (O && !O->DumpBlockinfo) ; how can we get here and O && have any effect?

762

Ah my bad: there is a if (O && !O->DumpBlockinfo) but it only guard a single line, not the entire block.

[tejohnson]

llvm/test/tools/llvm-bcanalyzer/dump-blockinfo.test

1 ↗

(On Diff #376797)

Ah ic. Hmm, perhaps you can just do the new testing directly in llvm/test/Other/bcanalyzer-block-info.txt? I didn't realize there were other llvm-bcanalyzer tests in that test directory - makes sense to put it there and probably consolidate it into that existing test which also tests how that info gets dumped.

[woodruffw]

Moved the new test to llvm/test/Other/bcanalyzer-dump-blockinfo-option.txt per @tejohnson's suggestion.

(I kept it in its own file for consistency with the other llvm-bcanalyzer tests in that dir.)

[tejohnson]

lgtm

llvm/include/llvm/Bitcode/BitcodeAnalyzer.h

   bool Symbolic = false;
   /// Print binary blobs using hex escapes.
   bool ShowBinaryBlobs = false;
+  /// Print BLOCKINFO block details.
+  bool DumpBlockinfo = false;
 
   BCDumpOptions(raw_ostream &OS) : OS(OS) {}
 };

llvm/lib/Bitcode/Reader/BitcodeAnalyzer.cpp

   // BLOCKINFO is a special part of the stream.
   bool DumpRecords = O.hasValue();
   if (BlockID == bitc::BLOCKINFO_BLOCK_ID) {
-    if (O)
+    if (O && !O->DumpBlockinfo)
       O->OS << Indent << "<BLOCKINFO_BLOCK/>\n";
     Expected<Optional<BitstreamBlockInfo>> MaybeNewBlockInfo =
         Stream.ReadBlockInfoBlock(/*ReadBlockInfoNames=*/true);

     if (Error Err = Stream.JumpToBit(BlockBitStart))
       return Err;
     // It's not really interesting to dump the contents of the blockinfo
-    // block.
-    DumpRecords = false;
+    // block, so only do it if the user explicitly requests it.
+    DumpRecords = O && O->DumpBlockinfo;
   }
 
   unsigned NumWords = 0;

llvm/tools/llvm-bcanalyzer/llvm-bcanalyzer.cpp

 static cl::opt<bool> Dump("dump", cl::desc("Dump low level bitcode trace"),
                           cl::cat(BCAnalyzerCategory));
 
+static cl::opt<bool> DumpBlockinfo("dump-blockinfo",
+                                   cl::desc("Include BLOCKINFO details in low"
+                                            " level dump"));
+
 //===----------------------------------------------------------------------===//
 // Bitcode specific analysis.
 //===----------------------------------------------------------------------===//

   O.Histogram = !NoHistogram;
   O.Symbolic = !NonSymbolic;
   O.ShowBinaryBlobs = ShowBinaryBlobs;
+  O.DumpBlockinfo = DumpBlockinfo;
 
   ExitOnErr(BA.analyze(
       Dump ? Optional<BCDumpOptions>(O) : Optional<BCDumpOptions>(None),