[bcanalyzer] Recognize more stream types

[modocache]

Summary

llvm-bcanalyzer prints out the stream type of the file it is
analyzing. If the file begins with the LLVM IR magic number, it reports
a stream type of "LLVM IR". However, any other bitstream format is
reported as "unknown".

Add some checks for two other common bitstream formats: Clang AST
files, which begin with 'CPCH', and Clang serialized diagnostics, which
begin with 'DIAG'.

Test Plan: check-llvm

[davide]

I don't mind the direction but this needs a test.

[modocache]

@davide I mentioned this in the "Test Plan" above, but I'm not sure how to test this without relying upon Clang. I do have a diff up for review that adds tests for this to the Clang repository, though: https://reviews.llvm.org/D41980. Maybe I'm missing something, though -- do you have any tips on how to test this entirely in the LLVM repo, so that I can include tests in this diff?

[davide]

Have you considered checking in the final artifacts (bitcode), maybe with a short description on how to craft them in case we need to regenerate?

[aprantl]

tools/llvm-bcanalyzer/llvm-bcanalyzer.cpp

799 ↗

(On Diff #129578)

Half-serious: How many implementations of this check do we have now (including clang)? It might be nice to we could add a checkMagic(stdd::array<char, 4>) function to BitStreamReader.

[modocache]

Very smart idea, @davide! I'll do that.

tools/llvm-bcanalyzer/llvm-bcanalyzer.cpp

799 ↗

(On Diff #129578)

Oh, great point!

I think @davide's suggestion to add a test for this is a good one. What do you think of me landing this with a test first, and then in another commit I can consolidate the logic?

[modocache]

Add tests directly to the LLVM test suite. Thanks, @davide!

[modocache]

Friendly ping! Is this looking OK?

[aprantl]

The two binaries are almost .5M large. Is that the smallest we can do, or would two 4-byte files with just the magic number be sufficient as well?

[aprantl]

tools/llvm-bcanalyzer/llvm-bcanalyzer.cpp

799 ↗

(On Diff #129578)

Could you add a helper function for comparing a magic number read from a bitstream?

[bruno]

test/Bitcode/stream-types.c

6 ↗

(On Diff #129642)

I see tests in clang using llvm-bcanalyzer, maybe you can also add tests there when this is done?

[modocache]

Sorry for letting this diff languish for so long. I updated the tests to be simple files with just the bytes necessary to be picked up as a specific stream type. I also moved the magic number reading over to a static helper function. I considered adding this function as a member of llvm::BirstreamReader, but chose not to for now, because doing so (as far as I can tell) would mean encoding Clang's magic numbers into LLVM's machinery. Let me know if you have different thoughts. Thanks!

[JDevlieghere]

Would you mind adding one more test that ensure the tool doesn’t crash with incomplete input, e.g. a file containing just CP or DI? Other than that this LGTM.

[modocache]

Thanks, @JDevlieghere! I added two tests like the ones you described. Thankfully it doesn't crash because llvm-bcanalyzer verifies the bitcode invariant that input sizes be a multiple of 4.

[modocache]

Strange, the revision appeared to have been "Accepted" by @JDevlieghere. I wonder why the message says it wasn't accepted...? Apologies in advance if I did something incorrectly when landing this.

[JDevlieghere]

In D41979#1074569, @modocache wrote:

Thanks, @JDevlieghere! I added two tests like the ones you described. Thankfully it doesn't crash because llvm-bcanalyzer verifies the bitcode invariant that input sizes be a multiple of 4.

Thanks, Brian!

In D41979#1074575, @modocache wrote:

Strange, the revision appeared to have been "Accepted" by @JDevlieghere. I wonder why the message says it wasn't accepted...? Apologies in advance if I did something incorrectly when landing this.

If someone request changes, they have to explicitly accept for Phabricator to consider the whole diff as accepted. I think the UI could do a better job at showing this, because I've seen this confuse people on several occasions. Anyway, I'm sure there's nothing to worry about here since you had already addressed @davide's comments.

llvm/trunk/test/Bitcode/stream-types.c

+// Tests that llvm-bcanalyzer recognizes the correct "stream type" for various
+// common bitstream formats.
+
+// RUN: llvm-bcanalyzer -dump %s.ast | FileCheck %s -check-prefix=CHECK-AST
+// CHECK-AST: Stream type: Clang Serialized AST
+
+// RUN: llvm-bcanalyzer -dump %s.dia | FileCheck %s -check-prefix=CHECK-DIAG
+// CHECK-DIAG: Stream type: Clang Serialized Diagnostics
+
+// RUN: not llvm-bcanalyzer -dump %s.ast.incomplete 2>&1 | FileCheck %s -check-prefix=CHECK-INCOMPLETE
+// RUN: not llvm-bcanalyzer -dump %s.dia.incomplete 2>&1 | FileCheck %s -check-prefix=CHECK-INCOMPLETE
+// CHECK-INCOMPLETE: Bitcode stream should be a multiple of 4 bytes in length

llvm/trunk/test/Bitcode/stream-types.c.ast

+CPCH
 No newline at end of file

llvm/trunk/test/Bitcode/stream-types.c.ast.incomplete

+CP
 No newline at end of file

llvm/trunk/test/Bitcode/stream-types.c.dia

+DIAG
 No newline at end of file

llvm/trunk/test/Bitcode/stream-types.c.dia.incomplete

+DIA
 No newline at end of file

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

static cl::opt<std::string> CheckHash(
     "check-hash",
     cl::desc("Check module hash using the argument as a string table"));
 
 namespace {
 
 /// CurStreamTypeType - A type for CurStreamType
 enum CurStreamTypeType {
   UnknownBitstream,
-  LLVMIRBitstream
+  LLVMIRBitstream,
+  ClangSerializedASTBitstream,
+  ClangSerializedDiagnosticsBitstream,
 };
 
 }
 
 /// GetBlockName - Return a symbolic block name if known, otherwise return
 /// null.
 static const char *GetBlockName(unsigned BlockID,
                                 const BitstreamBlockInfo &BlockInfo,

 static void PrintSize(double Bits) {
   outs() << format("%.2f/%.2fB/%luW", Bits, Bits/8,(unsigned long)(Bits/32));
 }
 static void PrintSize(uint64_t Bits) {
   outs() << format("%lub/%.2fB/%luW", (unsigned long)Bits,
                    (double)Bits/8, (unsigned long)(Bits/32));
 }
 
+static CurStreamTypeType ReadSignature(BitstreamCursor &Stream) {
+  char Signature[6];
+  Signature[0] = Stream.Read(8);
+  Signature[1] = Stream.Read(8);
+
+  // Autodetect the file contents, if it is one we know.
+  if (Signature[0] == 'C' && Signature[1] == 'P') {
+    Signature[2] = Stream.Read(8);
+    Signature[3] = Stream.Read(8);
+    if (Signature[2] == 'C' && Signature[3] == 'H')
+      return ClangSerializedASTBitstream;
+  } else if (Signature[0] == 'D' && Signature[1] == 'I') {
+    Signature[2] = Stream.Read(8);
+    Signature[3] = Stream.Read(8);
+    if (Signature[2] == 'A' && Signature[3] == 'G')
+      return ClangSerializedDiagnosticsBitstream;
+  } else {
+    Signature[2] = Stream.Read(4);
+    Signature[3] = Stream.Read(4);
+    Signature[4] = Stream.Read(4);
+    Signature[5] = Stream.Read(4);
+    if (Signature[0] == 'B' && Signature[1] == 'C' &&
+        Signature[2] == 0x0 && Signature[3] == 0xC &&
+        Signature[4] == 0xE && Signature[5] == 0xD)
+      return LLVMIRBitstream;
+  }
+  return UnknownBitstream;
+}
+
 static bool openBitcodeFile(StringRef Path,
                             std::unique_ptr<MemoryBuffer> &MemBuf,
                             BitstreamCursor &Stream,
                             CurStreamTypeType &CurStreamType) {
   // Read the input file.
   ErrorOr<std::unique_ptr<MemoryBuffer>> MemBufOrErr =
       MemoryBuffer::getFileOrSTDIN(Path);
   if (std::error_code EC = MemBufOrErr.getError())
if (Dump) {
              << " CPUType=" << format_hex(CPUType, 10) << "/>\n";
     }
 
     if (SkipBitcodeWrapperHeader(BufPtr, EndBufPtr, true))
       return ReportError("Invalid bitcode wrapper header");
   }
 
   Stream = BitstreamCursor(ArrayRef<uint8_t>(BufPtr, EndBufPtr));
-
-  // Read the stream signature.
-  char Signature[6];
-  Signature[0] = Stream.Read(8);
-  Signature[1] = Stream.Read(8);
-  Signature[2] = Stream.Read(4);
-  Signature[3] = Stream.Read(4);
-  Signature[4] = Stream.Read(4);
-  Signature[5] = Stream.Read(4);
-
-  // Autodetect the file contents, if it is one we know.
-  CurStreamType = UnknownBitstream;
-  if (Signature[0] == 'B' && Signature[1] == 'C' &&
-      Signature[2] == 0x0 && Signature[3] == 0xC &&
-      Signature[4] == 0xE && Signature[5] == 0xD)
-    CurStreamType = LLVMIRBitstream;
+  CurStreamType = ReadSignature(Stream);
 
   return false;
 }
 
 /// AnalyzeBitcode - Analyze the bitcode file specified by InputFilename.
 static int AnalyzeBitcode() {
   std::unique_ptr<MemoryBuffer> StreamBuffer;
   BitstreamCursor Stream;
static int AnalyzeBitcode() {
   uint64_t BufferSizeBits = Stream.getBitcodeBytes().size() * CHAR_BIT;
   // Print a summary of the read file.
   outs() << "Summary of " << InputFilename << ":\n";
   outs() << "         Total size: ";
   PrintSize(BufferSizeBits);
   outs() << "\n";
   outs() << "        Stream type: ";
   switch (CurStreamType) {
-  case UnknownBitstream: outs() << "unknown\n"; break;
-  case LLVMIRBitstream:  outs() << "LLVM IR\n"; break;
+  case UnknownBitstream:
+    outs() << "unknown\n";
+    break;
+  case LLVMIRBitstream:
+    outs() << "LLVM IR\n";
+    break;
+  case ClangSerializedASTBitstream:
+    outs() << "Clang Serialized AST\n";
+    break;
+  case ClangSerializedDiagnosticsBitstream:
+    outs() << "Clang Serialized Diagnostics\n";
+    break;
   }
   outs() << "  # Toplevel Blocks: " << NumTopBlocks << "\n";
   outs() << "\n";
 
   // Emit per-block stats.
   outs() << "Per-block Summary:\n";
   for (std::map<unsigned, PerBlockIDStats>::iterator I = BlockIDStats.begin(),
        E = BlockIDStats.end(); I != E; ++I) {