[llvm-dis] Add an option `print-thinlto-index-only` in llvm-dis to print index in LLVM assembly.

[mingmingl]

Summary

Context:

• In the ThinLTO minimized code [1], ThinLTOBitcodeWriter has a different format for GlobalVar [2] that's not yet supported by BitcodeReader yet. So add a flag to tell llvm-dis to parse thin-lto index only.

• From the iterative discussion and development of this patch, there isn't a clean, automatic way to choose a bitcode reader (that's as simple as adding a flag llvm-dis). An alternative option is to modify BitcodeReader to support the GlobalVar format, but that's going to be more involved change.

[1] e.g., bitcode generated by opt --thin-link-bitcode-file=<minimized-bitcode> or clang -Xclang -fthin-link-bitcode=<minimized-bitcode>
[2] https://github.com/llvm/llvm-project/blob/b6a93967d9c11e79802b5e75cec1584d6c8aa472/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp#L4741

[mingmingl]

llvm/tools/llvm-dis/llvm-dis.cpp

206–207

(Just a message before fixing the failed test)

In pre-merge test, llvm-dis parsing behavior changed for a bitcode that's invalid itself; the behavior changed from parse error to crash (test expects llvm-dis to return parse error while now llvm-dis crashes).

After a quick test, this is caused by the change of read order here;

• before this patch, getLazyModule will exit-on-err so getLTOInfo won't be executed
• after this patch, getLTOInfo is executed first, and crashes upon invalid abbrev ID around https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Bitstream/BitstreamReader.h#L528

I'll see how to solve this without changing test expectation from not to crash (i.e., llvm-dis still reports error rather than crash upon invalid bitcode).

The relevant checker line in invalid.test is /var/lib/buildkite-agent/builds/llvm-project/build/bin/llvm-dis -disable-output /var/lib/buildkite-agent/builds/llvm-project/llvm/test/Bitcode/Inputs/invalid-pointer-element-type.bc

[mingmingl]

llvm/tools/llvm-dis/llvm-dis.cpp

206–207

To add more context on getLazyModule and getLTOInfo and regarding how a bitcode is read

a) `getLazyModule` will invoke `BitcodeReader::parseModule`, which parses each subblock and exit-on-err when invalid pointer is read [1]; this is also where `invalid record` is returned when non-index-reader is used to parse minimized lto code.

b) `getLTOInfo` [2] has the equivalent loop to iterate blocks and subblocks, but skips uninteresting subblocks. As a result the invalid block `TYPE_BLOCK_ID_NEW` is read in a skipping fashion (while it's detected if parsed), so the read action right after `TYPE_BLOCK_ID_NEW` is in a sense undefined and triggered `invalid abbrev id` [3] in this case.

This means, llvm-dis gives more clear error log by parsing first and tell which section is not correct; if llvm-dis tries to read LTO info before parsing, and reading LTO info doesn't parse, the error log is less helpful than error log from parsing.

On the other hand, if a wrong reader is used during parsing, llvm-dis gives false alarms (e.g., llvm-dis cannot read minimized thinlto bitcode now)

From this perspective, it seems the right for llvm-dis to first parse and detect error; two options at the top of my head to solve this

-In `getLazyModule`, use the read-and-fail approach (try common reader first, fallback to the other reader, and returns error if the fallback doesn't work); the index-flag in module block is probably not necessary in this option to solve `llvm-dis` issue.
 
- Add another flavor of `getLTOInfo` that does parsing; but the problem of choosing a right reader still exists.

Maybe I should book a chat to discuss this further, but this patch is not in a hurry and not in the critical path anyway.

btw verified the loop to read module block is the same between getLTOInfo and getLazyModule implementation by printing names out (getLazyModule returns error on the 3rd entry when parsing it, getLTOInfo skips 3rd entry and fails on the 4th one).

[1] https://github.com/llvm/llvm-project/blob/d2cc6c2d0c2f8a6e272110416a3fd579ed5a3ac1/llvm/lib/Bitcode/Reader/BitcodeReader.cpp#L3657
[2] https://github.com/llvm/llvm-project/blob/d2cc6c2d0c2f8a6e272110416a3fd579ed5a3ac1/llvm/lib/Bitcode/Reader/BitcodeReader.cpp#L6982
[3] https://github.com/llvm/llvm-project/blob/6a1f50b84ae8f8a8087fcdbe5f27dae8c76878f1/llvm/include/llvm/Bitstream/BitstreamReader.h#L528

[tejohnson]

llvm/tools/llvm-dis/llvm-dis.cpp

206–207

Hmm, I see the issue. It is unfortunate to lose the more specific error message. Let me think about this some more.

[mingmingl]

Add a flag in llvm-dis to print index, and it's up to command runner to decide if the input is a fit (e.g., a ThinLTO bitcode).

Add a regression test for it.

[tejohnson]

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

3269

FYI looks like FUNCTION, ALIAS and IFUNC have the same issue. I'm not sure if there is a good common place to leave a comment about handling all of those?

llvm/test/Bitcode/thinlto-index-disassembled-by-llvm-dis.ll

15

Check that the option also works on %t.o

llvm/tools/llvm-dis/llvm-dis.cpp

79

"dump the index as a bitcode file" Do you mean dump/print the LLVM assembly? Suggest replacing "dump" with "print" in the option also.

[mingmingl]

Revise based on feedback.

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

3269

Good point!

Moved this to switch statements of BitcodeReader::parseModule, mentioned other simplified records, and added a github pointer to ThinLinkBitcodeWriter::writeSimplifiedModuleInfo (in case the method itself was renamed in the future).

[mingmingl]

Remove outdated verbose comment in BitcodeReader::parseGlobalVarRecord since there is a comment to cover more types of records.

[tejohnson]

llvm/test/Bitcode/thinlto-index-disassembled-by-llvm-dis.ll

16

If you change the suffix part to a wildcard then I believe the same checking lines could be shared by both llvm-dis invocations (e.g. remove FULL below and use DIS). Or just stop matching after thinlto-index-disassembled-by-llvm-dis.ll.tmp instead of using a wildcard.

llvm/tools/llvm-dis/llvm-dis.cpp

79

I would remove "as a bitcode file" since we aren't printing as bitcode (bitcode is the encoded input format), or use my initial suggestion to replace this with "LLVM assembly".

[mingmingl]

Revise based on feedback.

llvm/test/Bitcode/thinlto-index-disassembled-by-llvm-dis.ll

16

Done by stop matching after thinlto-index-disassembled-by-llvm-dis.ll.tmp

(Was initially thinking of keeping the suffix explicit but there is no ambiguity in the context anyway, so sharing is reasonable to me).

[mingmingl]

llvm/tools/llvm-dis/llvm-dis.cpp

79

thanks for pointing this out! Update to LLVM assembly now.

[tejohnson]

lgtm

[mingmingl]

pre-merge tests all passed. Going to submit the patch.

[mingmingl]

This is submitted in https://github.com/llvm/llvm-project/commit/e95ad93e6ef874ad208028c18b4cb4890320f25c

It seems that revision link isn't appended in commit message (maybe this time I didn't use arc), but mention this revision in https://github.com/llvm/llvm-project/commit/e95ad93e6ef874ad208028c18b4cb4890320f25c#commitcomment-64209086 for future reference.

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

 
 Error BitcodeReader::parseGlobalVarRecord(ArrayRef<uint64_t> Record) {
   // v1: [pointer type, isconst, initid, linkage, alignment, section,
   // visibility, threadlocal, unnamed_addr, externally_initialized,
   // dllstorageclass, comdat, attributes, preemption specifier,
   // partition strtab offset, partition strtab size] (name in VST)
   // v2: [strtab_offset, strtab_size, v1]
   StringRef Name;
   std::tie(Name, Record) = readNameFromStrtab(Record);
 
   if (Record.size() < 6)
     return error("Invalid record");
   Type *Ty = getTypeByID(Record[0]);
   if (!Ty)
     return error("Invalid record");
   bool isConstant = Record[1] & 1;
   bool explicitType = Record[1] & 2;
case bitc::MODULE_CODE_GCNAME: {  // SECTIONNAME: [strchr x N]
         return error("Invalid record");
       GCTable.push_back(S);
       break;
     }
     case bitc::MODULE_CODE_COMDAT:
       if (Error Err = parseComdatRecord(Record))
         return Err;
       break;
+    // FIXME: BitcodeReader should handle {GLOBALVAR, FUNCTION, ALIAS, IFUNC}
+    // written by ThinLinkBitcodeWriter. See
+    // `ThinLinkBitcodeWriter::writeSimplifiedModuleInfo` for the format of each
+    // record
+    // (https://github.com/llvm/llvm-project/blob/b6a93967d9c11e79802b5e75cec1584d6c8aa472/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp#L4714)
     case bitc::MODULE_CODE_GLOBALVAR:
       if (Error Err = parseGlobalVarRecord(Record))
         return Err;
       break;
     case bitc::MODULE_CODE_FUNCTION:
       ResolveDataLayout();
       if (Error Err = parseFunctionRecord(Record))
         return Err;

llvm/test/Bitcode/thinlto-index-disassembled-by-llvm-dis.ll

+; Tests that `llvm-dis` could disassemble ThinLTO minimized bitcode
+; (e.g., bitcode generated by `opt --thin-link-bitcode-file=<minimized-bitcode>`,
+;  or `clang -Xclang -fthin-link-bitcode=<minimized-bitcode>`)
+
+; %t.o is the thin-lto ready bitcode, and %t.thinlink.bc is the minimized bitcode. 
+; RUN: opt -thinlto-bc %s -thin-link-bitcode-file=%t.thinlink.bc -o %t.o
+
+; With default options, `llvm-dis` returns invalid record for ThinLTO minimized bitcode.
+; RUN: not llvm-dis %t.thinlink.bc 2>&1 | FileCheck %s --check-prefix=INVALID-RECORD
+
+; INVALID-RECORD: Invalid record
+
+; Tests that `llvm-dis` could disassemble minimized bitcode with `--print-thinlto-index-only`, and verify its content.
+; RUN: llvm-dis --print-thinlto-index-only %t.thinlink.bc -o - | FileCheck %s --check-prefix=DIS
+
+; Tests that `llvm-dis` could disassemble ThinLTO-ready bitcode with `--print-thinlto-index-only`,
+; and verify its content.
+; RUN: llvm-dis --print-thinlto-index-only %t.o -o - | FileCheck %s --check-prefix=DIS
+
+; DIS: ^0 = module: (path: "{{.*}}thinlto-index-disassembled-by-llvm-dis.ll.tmp
+; DIS: ^1 = gv: (name: "aplusb", summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0), insts: 2))) ; guid = 
+; DIS: ^2 = blockcount: 1
+
+source_filename = "add.cpp"
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+define i32 @aplusb(i32 %a, i32 %b) {
+  %add = add i32 %b, %a
+  ret i32 %add
+}

llvm/tools/llvm-dis/llvm-dis.cpp

static cl::opt<bool> PreserveAssemblyUseListOrder(
     cl::init(false), cl::Hidden, cl::cat(DisCategory));
 
 static cl::opt<bool>
     MaterializeMetadata("materialize-metadata",
                         cl::desc("Load module without materializing metadata, "
                                  "then materialize only the metadata"),
                         cl::cat(DisCategory));
 
+static cl::opt<bool> PrintThinLTOIndexOnly(
+    "print-thinlto-index-only",
+    cl::desc("Only read thinlto index and print the index as LLVM assembly."),
+    cl::init(false), cl::Hidden, cl::cat(DisCategory));
+
 namespace {
 
 static void printDebugLoc(const DebugLoc &DL, formatted_raw_ostream &OS) {
   OS << DL.getLine() << ":" << DL.getCol();
   if (DILocation *IDL = DL.getInlinedAt()) {
     OS << "@";
     printDebugLoc(IDL, OS);
   }
for (std::string InputFilename : InputFilenames) {
 
     const size_t N = IF.Mods.size();
 
     if (OutputFilename == "-" && N > 1)
       errs() << "only single module bitcode files can be written to stdout\n";
 
     for (size_t I = 0; I < N; ++I) {
       BitcodeModule MB = IF.Mods[I];
-      std::unique_ptr<Module> M = ExitOnErr(
+
+      std::unique_ptr<Module> M;
+
+      if (!PrintThinLTOIndexOnly) {
+        M = ExitOnErr(
             MB.getLazyModule(Context, MaterializeMetadata, SetImporting));
         if (MaterializeMetadata)
           ExitOnErr(M->materializeMetadata());
         else
           ExitOnErr(M->materializeAll());
+      }
 
       BitcodeLTOInfo LTOInfo = ExitOnErr(MB.getLTOInfo());
       std::unique_ptr<ModuleSummaryIndex> Index;
       if (LTOInfo.HasSummary)
         Index = ExitOnErr(MB.getSummary());
 
       std::string FinalFilename(OutputFilename);
 
       // Just use stdout.  We won't actually print anything on it.
       if (DontPrint)
         FinalFilename = "-";
for (size_t I = 0; I < N; ++I) {
       }
 
       std::unique_ptr<AssemblyAnnotationWriter> Annotator;
       if (ShowAnnotations)
         Annotator.reset(new CommentWriter());
 
       // All that llvm-dis does is write the assembly to a file.
       if (!DontPrint) {
+        if (M)
           M->print(Out->os(), Annotator.get(), PreserveAssemblyUseListOrder);
         if (Index)
           Index->print(Out->os());
       }
 
       // Declare success.
       Out->keep();
     }
   }
 
   return 0;
 }