Page MenuHomePhabricator

[BitcodeAnalyzer] allow a motivated user to dump BLOCKINFO
ClosedPublic

Authored by woodruffw on Aug 4 2021, 10:15 PM.

Details

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).

Diff Detail

Event Timeline

woodruffw created this revision.Aug 4 2021, 10:15 PM
woodruffw requested review of this revision.Aug 4 2021, 10:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2021, 10:15 PM
woodruffw updated this revision to Diff 364348.Aug 4 2021, 10:22 PM

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

woodruffw updated this revision to Diff 364557.Aug 5 2021, 11:10 AM

Fixed the diff tests.

Gentle ping for review on this!

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

Another ping for review.

xgupta added reviewers: dexonsmith, tejohnson.EditedSep 14 2021, 11:21 AM
xgupta added a subscriber: 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~

Added context to the patch.

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

Gentle ping for review on this!

steakhal removed a subscriber: steakhal.Sep 27 2021, 5:34 AM

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

llvm/lib/Bitcode/Reader/BitcodeAnalyzer.cpp
747

Why this change?

woodruffw added inline comments.Sep 29 2021, 1:21 PM
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 updated this revision to Diff 376797.Oct 3 2021, 7:51 PM

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.

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 added inline comments.Oct 8 2021, 9:49 PM
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?

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 updated this revision to Diff 378473.Oct 9 2021, 1:54 PM

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 added inline comments.Oct 9 2021, 2:02 PM
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 added inline comments.Oct 9 2021, 2:17 PM
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 added inline comments.Oct 9 2021, 5:15 PM
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 added inline comments.Oct 9 2021, 7:32 PM
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 updated this revision to Diff 378489.Oct 9 2021, 8:56 PM

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.)

This revision is now accepted and ready to land.Oct 9 2021, 9:06 PM
This revision was automatically updated to reflect the committed changes.