[PATCH] D104827: [DebugInfo] Enforce implicit constraints on `distinct` MDNodes

Thu Jul 8 08:23:48 PDT 2021

scott.linder added a comment.

In D104827#2854825 <https://reviews.llvm.org/D104827#2854825>, @uabelho wrote:

> Hi,
>
> We've noticed that this patch changes things so bcfiles created with the patch cannot be parsed with binaries built before it.
> E.g if I create foo.bc with a clang binary built from commit 8cd35ad854ab <https://reviews.llvm.org/rG8cd35ad854ab4458fd509447359066ea3578b494> (this patch) with:
>
>   clang -save-temps foo.c -S -g
>
> and then try to run opt built from aad87328fab <https://reviews.llvm.org/rGaad87328fabff9382bac0b452c83934515e6d0c8> (the commit before this patch) with
>
>   opt foo.bc -S > /dev/null
>
> it results in
>
>   opt: ../lib/Bitcode/Reader/MetadataLoader.cpp:366: (anonymous namespace)::(anonymous namespace)::PlaceholderQueue::~PlaceholderQueue(): Assertion `empty() && "PlaceholderQueue hasn't been flushed before being destroyed"' failed.
>   PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
>   Stack dump:
>   0.	Program arguments: build-all-builtins/bin/opt foo.bc -S
>    #0 0x0000000002a11623 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (build-all-builtins/bin/opt+0x2a11623)
>    #1 0x0000000002a0f2de llvm::sys::RunSignalHandlers() (build-all-builtins/bin/opt+0x2a0f2de)
>    #2 0x0000000002a119a6 SignalHandler(int) Signals.cpp:0:0
>    #3 0x00007f2c97079630 __restore_rt sigaction.c:0:0
>    #4 0x00007f2c947ac387 raise (/lib64/libc.so.6+0x36387)
>    #5 0x00007f2c947ada78 abort (/lib64/libc.so.6+0x37a78)
>    #6 0x00007f2c947a51a6 __assert_fail_base (/lib64/libc.so.6+0x2f1a6)
>    #7 0x00007f2c947a5252 (/lib64/libc.so.6+0x2f252)
>    #8 0x0000000003398a64 llvm::MetadataLoader::MetadataLoaderImpl::parseMetadata(bool) (build-all-builtins/bin/opt+0x3398a64)
>    #9 0x00000000033a19ac llvm::MetadataLoader::parseMetadata(bool) (build-all-builtins/bin/opt+0x33a19ac)
>   #10 0x00000000033730a8 (anonymous namespace)::BitcodeReader::parseFunctionBody(llvm::Function*) BitcodeReader.cpp:0:0
>   #11 0x0000000003370f7c (anonymous namespace)::BitcodeReader::materialize(llvm::GlobalValue*) BitcodeReader.cpp:0:0
>   #12 0x00000000033720aa (anonymous namespace)::BitcodeReader::materializeModule() BitcodeReader.cpp:0:0
>   #13 0x000000000221fc8a llvm::Module::materializeAll() (build-all-builtins/bin/opt+0x221fc8a)
>   #14 0x000000000336acb1 llvm::BitcodeModule::getModuleImpl(llvm::LLVMContext&, bool, bool, bool, llvm::function_ref<llvm::Optional<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > (llvm::StringRef)>) (build-all-builtins/bin/opt+0x336acb1)
>   #15 0x000000000336ef60 llvm::parseBitcodeFile(llvm::MemoryBufferRef, llvm::LLVMContext&, llvm::function_ref<llvm::Optional<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > (llvm::StringRef)>) (build-all-builtins/bin/opt+0x336ef60)
>   #16 0x00000000024521a0 llvm::parseIR(llvm::MemoryBufferRef, llvm::SMDiagnostic&, llvm::LLVMContext&, llvm::function_ref<llvm::Optional<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > (llvm::StringRef)>) (build-all-builtins/bin/opt+0x24521a0)
>   #17 0x000000000245262e llvm::parseIRFile(llvm::StringRef, llvm::SMDiagnostic&, llvm::LLVMContext&, llvm::function_ref<llvm::Optional<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > (llvm::StringRef)>) (build-all-builtins/bin/opt+0x245262e)
>   #18 0x0000000000782acb main (build-all-builtins/bin/opt+0x782acb)
>   #19 0x00007f2c94798555 __libc_start_main (/lib64/libc.so.6+0x22555)
>   #20 0x000000000076f800 _start (build-all-builtins/bin/opt+0x76f800)
>   Abort
>
> I suppose this is not expected?
>
> F17715352: foo.bc <https://reviews.llvm.org/F17715352>

This seems to be a long-standing bug with the bitcode reader, where every `return error("Invalid record")` in `llvm::MetadataLoader::MetadataLoaderImpl::parseOneMetadata` is untested code that actually just triggers an `assert` in `PlaceholderQueue::~PlaceholderQueue`. For example, the last version bump for DIExpression has the same issue:

  user at host:~/llvm-project/main:main$ git switch --detach ffc498dfcc8d67e6e9acaebb690377f674457ab4
  Updating files: 100% (101031/101031), done.
  HEAD is now at ffc498dfcc8d Align definition of DW_OP_plus with DWARF spec [3/3]
  user at host:~/llvm-project/main:(ffc498dfcc8d...)$ ninja -C release/ clang
  ninja: Entering directory `release/'
  ...
  [2569/2569] Creating executable symlink bin/clang
  user at host:~/llvm-project/main:(ffc498dfcc8d...)$ echo 'void f(int x) {}' | release/bin/clang -x c - -c -emit-llvm -o- -g >
  ../f.bc
  user at host:~/llvm-project/main:(ffc498dfcc8d...)$ git switch --detach HEAD^
  Previous HEAD position was ffc498dfcc8d Align definition of DW_OP_plus with DWARF spec [3/3]
  HEAD is now at 3f250380d4bd Corrected some comment typos; NFC.
  user at host:~/llvm-project/main:(3f250380d4bd...)$ ninja -C release/ opt
  ninja: Entering directory `release/'
  ...
  [149/149] Linking CXX executable bin/opt
  user at host:~/llvm-project/main:(3f250380d4bd...)$ release/bin/opt ../f.bc -S
  opt: /home/slinder1/llvm-project/main/llvm/lib/Bitcode/Reader/MetadataLoader.cpp:363: (anonymous namespace)::(anonymous namespace):
  :PlaceholderQueue::~PlaceholderQueue(): Assertion `empty() && "PlaceholderQueue hasn't been flushed before being destroyed"' failed
  .
  #0 0x0000000002aab1b4 PrintStackTraceSignalHandler(void*) Signals.cpp:0:0
  #1 0x0000000002aab4f6 SignalHandler(int) Signals.cpp:0:0
  #2 0x00007f56a9e753c0 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x153c0)
  #3 0x00007f56a970718b raise /build/glibc-eX1tMB/glibc-2.31/signal/../sysdeps/unix/sysv/linux/raise.c:51:1
  #4 0x00007f56a96e6859 abort /build/glibc-eX1tMB/glibc-2.31/stdlib/abort.c:81:7
  #5 0x00007f56a96e6729 get_sysdep_segment_value /build/glibc-eX1tMB/glibc-2.31/intl/loadmsgcat.c:509:8
  #6 0x00007f56a96e6729 _nl_load_domain /build/glibc-eX1tMB/glibc-2.31/intl/loadmsgcat.c:970:34
  #7 0x00007f56a96f7f36 (/lib/x86_64-linux-gnu/libc.so.6+0x36f36)
  #8 0x0000000002f8e298 llvm::MetadataLoader::MetadataLoaderImpl::parseMetadata(bool) (release/bin/opt+0x2f8e298)
  #9 0x0000000002f94dcc llvm::MetadataLoader::parseMetadata(bool) (release/bin/opt+0x2f94dcc)
  #10 0x0000000002f6ecc6 (anonymous namespace)::BitcodeReader::parseFunctionBody(llvm::Function*) BitcodeReader.cpp:0:0
  #11 0x0000000002f6cf29 (anonymous namespace)::BitcodeReader::materialize(llvm::GlobalValue*) BitcodeReader.cpp:0:0
  #12 0x0000000002f6dc1a (anonymous namespace)::BitcodeReader::materializeModule() BitcodeReader.cpp:0:0
  #13 0x00000000025a7dda llvm::Module::materializeAll() (release/bin/opt+0x25a7dda)
  #14 0x0000000002f66dd3 llvm::BitcodeModule::getModuleImpl(llvm::LLVMContext&, bool, bool, bool) (release/bin/opt+0x2f66dd3)
  #15 0x0000000002f6a519 llvm::parseBitcodeFile(llvm::MemoryBufferRef, llvm::LLVMContext&) (release/bin/opt+0x2f6a519)
  #16 0x00000000026c7cf1 llvm::parseIR(llvm::MemoryBufferRef, llvm::SMDiagnostic&, llvm::LLVMContext&) (release/bin/opt+0x26c7cf1)
  #17 0x00000000026c81e0 llvm::parseIRFile(llvm::StringRef, llvm::SMDiagnostic&, llvm::LLVMContext&) (release/bin/opt+0x26c81e0)
  #18 0x00000000014d65fc main (release/bin/opt+0x14d65fc)
  #19 0x00007f56a96e80b3 __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:342:3
  #20 0x00000000014cc5ae _start (release/bin/opt+0x14cc5ae)
  Stack dump:
  0.      Program arguments: release/bin/opt ../f.bc -S
  Aborted (core dumped)
  user at host:~/llvm-project/main:(3f250380d4bd...)$

Again, my guess is that we just want to not assert in this case, and "clear" the `PlaceholderQueue` as none of the `Metadata` we have parsed will be used.

The issue of testing newly-encoded bitcode against previous versions of the loader seems like a trickier issue, though. In fact, it may be tricky to test the fix for this assert at all, as I assume it requires manipulating bitcode files to make them invalid in some way that would trigger these `return("Invalid record")`s.

Any thoughts on where to go from here, and whether this patch should be blocked on these fixes?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104827/new/

https://reviews.llvm.org/D104827