[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

struct BCDumpOptions {
   /// The stream.
   raw_ostream &OS;
   /// Print per-code histogram.
   bool Histogram = false;
   /// Don't emit numeric info in dump if symbolic info is available.
   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) {}
 };
 
 class BitcodeAnalyzer {
   BitstreamCursor Stream;
   BitstreamBlockInfo BlockInfo;
   CurStreamTypeType CurStreamType;

llvm/lib/Bitcode/Reader/BitcodeAnalyzer.cpp

Error BitcodeAnalyzer::parseBlock(unsigned BlockID, unsigned IndentLevel,
   // Get the statistics for this BlockID.
   PerBlockIDStats &BlockStats = BlockIDStats[BlockID];
 
   BlockStats.NumInstances++;
 
   // 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 (!MaybeNewBlockInfo)
       return MaybeNewBlockInfo.takeError();
     Optional<BitstreamBlockInfo> NewBlockInfo =
         std::move(MaybeNewBlockInfo.get());
     if (!NewBlockInfo)
       return reportError("Malformed BlockInfoBlock");
     BlockInfo = std::move(*NewBlockInfo);
     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;
   if (Error Err = Stream.EnterSubBlock(BlockID, &NumWords))
     return Err;
 
   // Keep it for later, when we see a MODULE_HASH record
   uint64_t BlockEntryPos = Stream.getCurrentByteNo();

llvm/test/Other/bcanalyzer-dump-blockinfo-option.txt

+# RUN: llvm-bcanalyzer --dump --dump-blockinfo %S/Inputs/has-block-info.bc | FileCheck %s
+
+# CHECK: <BLOCKINFO_BLOCK NumWords=13 BlockCodeSize=2>
+# CHECK:   <SETBID op0=8/>
+# CHECK:   <BLOCKNAME op0=65 op1=66 op2=67 op3=0/>
+# CHECK:   <SETRECORDNAME op0=0 op1=65 op2=65 op3=65 op4=0/>
+# CHECK:   <SETRECORDNAME op0=1 op1=66 op2=66 op3=66 op4=0/>
+# CHECK:   <SETBID op0=9/>
+# CHECK:   <BLOCKNAME op0=88 op1=89 op2=90 op3=0/>
+# CHECK:   <SETRECORDNAME op0=0 op1=88 op2=88 op3=88 op4=0/>
+# CHECK:   <SETRECORDNAME op0=1 op1=89 op2=89 op3=89 op4=0/>
+# CHECK: </BLOCKINFO_BLOCK>

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

 //===-- llvm-bcanalyzer.cpp - Bitcode Analyzer --------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This tool may be invoked in the following manner:
 //  llvm-bcanalyzer [options]      - Read LLVM bitcode from stdin
 //  llvm-bcanalyzer [options] x.bc - Read LLVM bitcode from the x.bc file
 //
 //  Options:
-//      --help      - Output information about command line switches
-//      --dump      - Dump low-level bitcode structure in readable format
+//      --help            - Output information about command line switches
+//      --dump            - Dump low-level bitcode structure in readable format
+//      --dump-blockinfo  - Dump the BLOCKINFO_BLOCK, when used with --dump
 //
 // This tool provides analytical information about a bitcode file. It is
 // intended as an aid to developers of bitcode reading and writing software. It
 // produces on std::out a summary of the bitcode file that shows various
 // statistics about the contents of the file. By default this information is
 // detailed and contains information about individual bitcode blocks and the
 // functions in the module.
 // The tool is also able to print a bitcode file in a straight forward text

 static cl::opt<std::string> InputFilename(cl::Positional,
                                           cl::desc("<input bitcode>"),
                                           cl::init("-"),
                                           cl::cat(BCAnalyzerCategory));
 
 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"),
+                                   cl::cat(BCAnalyzerCategory));
+
 //===----------------------------------------------------------------------===//
 // Bitcode specific analysis.
 //===----------------------------------------------------------------------===//
 
 static cl::opt<bool> NoHistogram("disable-histogram",
                                  cl::desc("Do not print per-code histogram"),
                                  cl::cat(BCAnalyzerCategory));
 
int main(int argc, char **argv) {
   BitcodeAnalyzer BA(MB->getBuffer(),
                      BlockInfoMB ? Optional<StringRef>(BlockInfoMB->getBuffer())
                                  : None);
 
   BCDumpOptions O(outs());
   O.Histogram = !NoHistogram;
   O.Symbolic = !NonSymbolic;
   O.ShowBinaryBlobs = ShowBinaryBlobs;
+  O.DumpBlockinfo = DumpBlockinfo;
 
   ExitOnErr(BA.analyze(
       Dump ? Optional<BCDumpOptions>(O) : Optional<BCDumpOptions>(None),
       CheckHash.empty() ? None : Optional<StringRef>(CheckHash)));
 
   if (Dump)
     outs() << "\n\n";
 
   BA.printStats(O, StringRef(InputFilename.getValue()));
   return 0;
 }