[DebugInfo] Enforce implicit constraints on `distinct` MDNodes

[scott.linder]

Summary

Add UNIQUED and DISTINCT properties in Metadata.def and use them to
implement restrictions on the distinct property of MDNodes:

• DIExpression can currently be parsed from IR or read from bitcode as distinct, but this property is silently dropped when printing to IR. This causes accepted IR to fail to round-trip. As DIExpression appears inline at each use in the canonical form of IR, it cannot actually be distinct anyway, as there is no syntax to describe it.
• DICompileUnit is already restricted to always being distinct, but along with adding general support for the inverse restriction I went ahead and described this in Metadata.def and updated the parser to be general. Future nodes which have this restriction can share this support.

The new UNIQUED property applies to DIExpression, and forbids it to be
distinct. It also implies it are canonically printed inline at each
use, rather than via MDNode ID.

The new DISTINCT property applies to DICompileUnit, and requires it to
be distinct.

A potential alternative change is to forbid the non-inline syntax for
DIExpression entirely, as is done with DIArgList implicitly by requiring
it appear in the context of a function. For example, we would forbid:

!named = !{!0}
!0 = !DIExpression()

Instead we would only accept the equivalent inlined version:

!named = !{!DIExpression()}

This essentially removes the ability to create a distinct DIExpression
by construction, as there is no syntax for distinct inline. If this
patch is accepted as-is, the result would be that the non-canonical
version is accepted, but the following would be an error and produce a diagnostic:

!named = !{!0}
; error: 'distinct' not allowed for !DIExpression()
!0 = distinct !DIExpression()

Also update some documentation to consistently use the inline syntax for
DIExpression, and to describe the restrictions on distinct for nodes
where applicable.

[t-tye]

LGTM Thanks for documenting the differences.

[scott.linder]

Implement inline case in AssemblyWriter::printNamedMDNode based on UNIQUED-ness.

I'm not sure my overall approach to this patch is actually the best way to
proceed. The biggest issue is that DIArgList is more particular about where it
can appear, so most of this generic support for UNIQUED nodes is just dead code
for DIArgList. It still seems like having a lot of if (isa<Foo>) ...;
littered around the IR parser/printer, bitcode reader/writer, MIR
reader/writer, etc. to implement inline nodes is more error prone, and we still
need to fix the bug with distinct either way.

Does anyone have thoughts on what approach to take?

[StephenTozer]

Good patch, this is a good way of generifying the logic for inline metadata - and improving the documentation at the same time, which is a big plus!

In D104827#2839615, @scott.linder wrote:

I'm not sure my overall approach to this patch is actually the best way to
proceed. The biggest issue is that DIArgList is more particular about where it
can appear, so most of this generic support for UNIQUED nodes is just dead code
for DIArgList.

I think this is mostly unavoidable as-is; this patch does nicely take care of the inline printing/parsing logic, trying to also simplify the function-local aspects would be out of scope. In that regard, the class that DIArgList shares its behaviour with is ValueAsMetadata/LocalAsMetadata. Unfortunately there is no easy way to generify that logic, as ValueAsMetadata is a unique case since it is printed as i32 %value rather than !ClassName(...). And I don't think I'd quite call the generic support "dead code" for DIArgList - it may be an error in all cases except when parsed as a function operand, but it still does need to be handled, and if some change in future affects how we print/parse inline metadata then that change must still apply equally to both DIExpression and DIArgList, which this patch facilitates.

A potential alternative change is to forbid the non-inline syntax for
DIExpression entirely, as is done with DIArgList implicitly by requiring
it appear in the context of a function.

Personally, I could go either way with this. Currently, DIExpression does not actually use function state in any way, but will still only ever be used in the context of a function. In particular, I'm not sure what value there could ever be in using a DIExpression in a named metadata node. It seems that in principle it would be better suited to being explicitly function-local metadata, but I'm not sure if it's worth putting in the effort to make the change when doing so doesn't grant us much benefit and also breaks parsing for old IR that contains ID'd DIExpressions.

[scott.linder]

Assert based on UNIQUED/DISTINCT-ness in ModuleBitcodeWriter

[uabelho]

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 (this patch) with:

clang -save-temps foo.c -S -g

and then try to run opt built from aad87328fab (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?

foo.bc2 KBDownload

[dexonsmith]

That looks bad — best to revert while that's investigated.

[JDevlieghere]

In D104827#2855970, @dexonsmith wrote:

That looks bad — best to revert while that's investigated.

This also breaks lang/cpp/member-and-local-vars-with-same-name/TestMembersAndLocalsWithSameName.py in the LLDB test suite:

Assertion failed: ((FromValue || !(isa<LocalAsMetadata>(MD) || isa<DIArgList>(MD))) && "Unexpected function-local metadata outside of value argument"), function WriteAsOperandInternal, file AsmWriter.cpp, line 2519.

Given that it's Friday, I'm going to go ahead and revert this to get our bot back to green.

[scott.linder]

In 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 (this patch) with:

clang -save-temps foo.c -S -g

and then try to run opt built from aad87328fab (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?

foo.bc2 KBDownload

Thank you for letting me know, and for the reproducer! Curiously this is just on the return error(...) path in upgradeDIExpression, which I just didn't test (as I'm not sure how to in the current LLVM infrastructure). I assume this is because normally bitcode error returns occur after most of the bitcode file has already been read, so there are no pending placeholders, but I need to learn some more about the bitcode parser in general to be sure I understand the issue. My first thought is that we should just flush placeholders on an error, as we are about to just bail anyway.

In D104827#2856347, @JDevlieghere wrote:

In D104827#2855970, @dexonsmith wrote:

That looks bad — best to revert while that's investigated.

This also breaks lang/cpp/member-and-local-vars-with-same-name/TestMembersAndLocalsWithSameName.py in the LLDB test suite:

Assertion failed: ((FromValue || !(isa<LocalAsMetadata>(MD) || isa<DIArgList>(MD))) && "Unexpected function-local metadata outside of value argument"), function WriteAsOperandInternal, file AsmWriter.cpp, line 2519.

Given that it's Friday, I'm going to go ahead and revert this to get our bot back to green.

Thank you for reverting, I was out Friday+Monday. I thought I was safe to move the assert that is triggering here up further in the function body to make it stronger, but evidently I overlooked some cases. I'll identify them and either fix them elsewhere or restore the assert (possibly with a comment describing the rationale). Thanks again!

[scott.linder]

In 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 (this patch) with:

clang -save-temps foo.c -S -g

and then try to run opt built from aad87328fab (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?

foo.bc2 KBDownload

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@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@host:~/llvm-project/main:(ffc498dfcc8d...)$ ninja -C release/ clang
ninja: Entering directory `release/'
...
[2569/2569] Creating executable symlink bin/clang
user@host:~/llvm-project/main:(ffc498dfcc8d...)$ echo 'void f(int x) {}' | release/bin/clang -x c - -c -emit-llvm -o- -g >
../f.bc
user@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@host:~/llvm-project/main:(3f250380d4bd...)$ ninja -C release/ opt
ninja: Entering directory `release/'
...
[149/149] Linking CXX executable bin/opt
user@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@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?

[dexonsmith]

In D104827#2855970, @dexonsmith wrote:

That looks bad — best to revert while that's investigated.

Oops... I had misread this comment:

In D104827#2854825, @uabelho wrote:

We've noticed that this patch changes things so bcfiles created with the patch cannot be parsed with binaries built before it.

I don't think we have any guarantees about reading new bitcode with old readers. There's some machinery for a high-level version check to get a nice error (checking MODULE_CODE_VERSION) but I think that's it. (I thought this was saying the new reader couldn't read the old bitcode, in which case there'd be a missing bitcode upgrade.)

In D104827#2864582, @scott.linder wrote:

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

Assuming IIUC now, the patch can probably land as-is... but probably makes sense to investigate the LLDB issue first. @JDevlieghere, could the failing LLDB test somehow be reading a new bitcode file with an old reader, or is that something different?

[uabelho]

In D104827#2865292, @dexonsmith wrote:

In D104827#2855970, @dexonsmith wrote:

That looks bad — best to revert while that's investigated.

Oops... I had misread this comment:

In D104827#2854825, @uabelho wrote:

We've noticed that this patch changes things so bcfiles created with the patch cannot be parsed with binaries built before it.

I don't think we have any guarantees about reading new bitcode with old readers. There's some machinery for a high-level version check to get a nice error (checking MODULE_CODE_VERSION) but I think that's it. (I thought this was saying the new reader couldn't read the old bitcode, in which case there'd be a missing bitcode upgrade.)

Hm, I often do this and I can't remember it failing before. E.g. if I run into a crash in opt, I usually save the bc file and bisect into old versions to see with what opt version the crash starts appearing. This usually works very well so I was surprised and considered it a bug now when it didn't work.

In D104827#2864582, @scott.linder wrote:

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

Assuming IIUC now, the patch can probably land as-is... but probably makes sense to investigate the LLDB issue first. @JDevlieghere, could the failing LLDB test somehow be reading a new bitcode file with an old reader, or is that something different?

I've seen the same crash in our testing and then there was no "new bitcode with old reader" stuff involved, it was just a normal run crashing in somewhere llc (for our out of tree target).

[scott.linder]

llvm/lib/IR/AsmWriter.cpp

4787

I think I've identified the source of the bug causing the assert on GreenDragon: I didn't intend to remove this FromValue=true argument. I believe the assert can still be hoisted up, so I'll post an updated patch which just restores this line.

[scott.linder]

Fix bug in printMetadataImpl

[scott.linder]

Eliminate the getDistinct method entirely from DIExpression and DIArgList. I
went about this by splitting up the DEFINE_MDNODE_* macros so each MDNode
class can use an appropriate version based on their distinct/uniqued
properties.

Also fix up the end of DebugInfoMetadata.h to #undef all of the macros.

[scott.linder]

Rebase

I believe the only pending issue is resolved, so I plan to land this tomorrow
if nobody has any objections.

[aeubanks]

hi, this is causing crashes

$ build/rel/bin/opt -passes='thinlto-pre-link<O2>' /tmp/a.ll -o /dev/null
opt: ../../llvm/include/llvm/IR/Metadata.def:180: void (anonymous namespace)::ModuleBitcodeWriter::writeMetadataRecords(ArrayRef<const llvm::Metadata *>, SmallVectorImpl<uint64_t> &, std::vector<
unsigned int> *, std::vector<uint64_t> *): Assertion `!N->isDistinct() && "Expected non-distinct " "DIArgList"' failed.                                                                            
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.                                                                                                              
Stack dump:                                                                                                                                                                                        
0.      Program arguments: build/rel/bin/opt -passes=thinlto-pre-link<O2> /tmp/a.ll -o /dev/null                                                                                                   
 #0 0x0000000001f37a13 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/lib/Support/Unix/Signals.inc:565:13             
 #1 0x0000000001f3588e llvm::sys::RunSignalHandlers() /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/lib/Support/Signals.cpp:98:18                                        
 #2 0x0000000001f37d7f SignalHandler(int) /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/lib/Support/Unix/Signals.inc:407:1                                               
 #3 0x00007f2003b8e8e0 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x138e0)                                                                                                                
 #4 0x00007f2003668e71 raise ./signal/../sysdeps/unix/sysv/linux/raise.c:50:1                                                                                                                      
 #5 0x00007f2003652536 abort ./stdlib/abort.c:81:7                                                                                                                                                 
 #6 0x00007f200365241f get_sysdep_segment_value ./intl/loadmsgcat.c:509:8                                                                                                                          
 #7 0x00007f200365241f _nl_load_domain ./intl/loadmsgcat.c:970:34                                                                                                                                  
 #8 0x00007f20036617f2 (/lib/x86_64-linux-gnu/libc.so.6+0x357f2)                                                                                                                                   
 #9 0x000000000168e506 (anonymous namespace)::ModuleBitcodeWriter::writeMetadataRecords(llvm::ArrayRef<llvm::Metadata const*>, llvm::SmallVectorImpl<unsigned long>&, std::vector<unsigned int, std::allocator<unsigned int> >*, std::vector<unsigned long, std::allocator<unsigned long> >*) /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/include/llvm/IR/Metadata.def:0:0
#10 0x000000000167df15 writeFunctionMetadata /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:2303:3
#11 0x000000000167df15 writeFunction /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:3351:3
#12 0x000000000167df15 (anonymous namespace)::ModuleBitcodeWriter::write() /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:4448:7
#13 0x0000000001677d32 ~ModuleBitcodeWriterBase /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:155:7
#14 0x0000000001677d32 llvm::BitcodeWriter::writeModule(llvm::Module const&, bool, llvm::ModuleSummaryIndex const*, bool, std::array<unsigned int, 5ul>*) /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:4626:1
#15 0x00000000016813cb llvm::WriteBitcodeToFile(llvm::Module const&, llvm::raw_ostream&, bool, llvm::ModuleSummaryIndex const*, bool, std::array<unsigned int, 5ul>*) /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:4653:10

a.ll60 KBDownload

(I tried reducing as much as possible but there's still a lot of debug info)

[scott.linder]

In D104827#3119839, @aeubanks wrote:

hi, this is causing crashes

$ build/rel/bin/opt -passes='thinlto-pre-link<O2>' /tmp/a.ll -o /dev/null
opt: ../../llvm/include/llvm/IR/Metadata.def:180: void (anonymous namespace)::ModuleBitcodeWriter::writeMetadataRecords(ArrayRef<const llvm::Metadata *>, SmallVectorImpl<uint64_t> &, std::vector<
unsigned int> *, std::vector<uint64_t> *): Assertion `!N->isDistinct() && "Expected non-distinct " "DIArgList"' failed.                                                                            
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.                                                                                                              
Stack dump:                                                                                                                                                                                        
0.      Program arguments: build/rel/bin/opt -passes=thinlto-pre-link<O2> /tmp/a.ll -o /dev/null                                                                                                   
 #0 0x0000000001f37a13 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/lib/Support/Unix/Signals.inc:565:13             
 #1 0x0000000001f3588e llvm::sys::RunSignalHandlers() /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/lib/Support/Signals.cpp:98:18                                        
 #2 0x0000000001f37d7f SignalHandler(int) /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/lib/Support/Unix/Signals.inc:407:1                                               
 #3 0x00007f2003b8e8e0 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x138e0)                                                                                                                
 #4 0x00007f2003668e71 raise ./signal/../sysdeps/unix/sysv/linux/raise.c:50:1                                                                                                                      
 #5 0x00007f2003652536 abort ./stdlib/abort.c:81:7                                                                                                                                                 
 #6 0x00007f200365241f get_sysdep_segment_value ./intl/loadmsgcat.c:509:8                                                                                                                          
 #7 0x00007f200365241f _nl_load_domain ./intl/loadmsgcat.c:970:34                                                                                                                                  
 #8 0x00007f20036617f2 (/lib/x86_64-linux-gnu/libc.so.6+0x357f2)                                                                                                                                   
 #9 0x000000000168e506 (anonymous namespace)::ModuleBitcodeWriter::writeMetadataRecords(llvm::ArrayRef<llvm::Metadata const*>, llvm::SmallVectorImpl<unsigned long>&, std::vector<unsigned int, std::allocator<unsigned int> >*, std::vector<unsigned long, std::allocator<unsigned long> >*) /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/include/llvm/IR/Metadata.def:0:0
#10 0x000000000167df15 writeFunctionMetadata /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:2303:3
#11 0x000000000167df15 writeFunction /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:3351:3
#12 0x000000000167df15 (anonymous namespace)::ModuleBitcodeWriter::write() /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:4448:7
#13 0x0000000001677d32 ~ModuleBitcodeWriterBase /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:155:7
#14 0x0000000001677d32 llvm::BitcodeWriter::writeModule(llvm::Module const&, bool, llvm::ModuleSummaryIndex const*, bool, std::array<unsigned int, 5ul>*) /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:4626:1
#15 0x00000000016813cb llvm::WriteBitcodeToFile(llvm::Module const&, llvm::raw_ostream&, bool, llvm::ModuleSummaryIndex const*, bool, std::array<unsigned int, 5ul>*) /usr/local/google/home/aeubanks/repos/llvm-project/build/rel/../../llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:4653:10

a.ll60 KBDownload

(I tried reducing as much as possible but there's still a lot of debug info)

Thank you for the reproducer! It seems like one of my fundamental assumptions about how LLVM handles metadata was faulty.

I assumed that uniqued nodes couldn't "become distinct", but they evidently can due to things like RAUW. I'll try to get a better grasp on what the rules actually are and then update the review.

[scott.linder]

Update DIArgList::handleChangedOperand to propagate in-place changes which
would require the DIArgList become distinct up to the owning MetadataAsValue.

This feels a bit odd, but so does allowing DIArgList to be distinct as an
implementation detail, while never serializing that fact (and so making a
round-trip lossy). Semantically this may be OK, but unless we have a good
reason not to I feel like finding a way to ensure these "inline" nodes are
always uniqued will make the relationship between IR/bitcode and the in-memory
representation more obvious.

[scott.linder]

In D104827#3128572, @scott.linder wrote:

Update DIArgList::handleChangedOperand to propagate in-place changes which
would require the DIArgList become distinct up to the owning MetadataAsValue.

This feels a bit odd, but so does allowing DIArgList to be distinct as an
implementation detail, while never serializing that fact (and so making a
round-trip lossy). Semantically this may be OK, but unless we have a good
reason not to I feel like finding a way to ensure these "inline" nodes are
always uniqued will make the relationship between IR/bitcode and the in-memory
representation more obvious.

I'm adding some more reviewers from those involved in https://reviews.llvm.org/D108968, as I'm essentially extending the changes made there.

[dexonsmith]

In D104827#3128572, @scott.linder wrote:

Update DIArgList::handleChangedOperand to propagate in-place changes which
would require the DIArgList become distinct up to the owning MetadataAsValue.

This feels a bit odd, but so does allowing DIArgList to be distinct as an
implementation detail, while never serializing that fact (and so making a
round-trip lossy).

Seems reasonable to try to fix this.

Semantically this may be OK, but unless we have a good
reason not to I feel like finding a way to ensure these "inline" nodes are
always uniqued will make the relationship between IR/bitcode and the in-memory
representation more obvious.

The tricky constraint is that (most) references to Metadata are not in any use-list.

llvm/lib/IR/DebugInfoMetadata.cpp

1584–1585

I'm sorry; I'm lacking some context here; I'd appreciate a bit of hand-holding.

I see how this updates the MetadataAsValue reference.

Can you walk me through in way way it's enforced that DIArgList is only referenced by MetadataAsValue? It sounds like textual IR has something, but can you confirm? What about the C++ API? The verifier? And what about historical bitcode... i.e., what can be found there?

• If there are holes in enforcement somehow, what's going to happen to the dangling references to this node?
• Can/should we delete this node now? And/or somehow mark it as dead, so that ValueAsMetadata will look through it?

[scott.linder]

llvm/lib/IR/DebugInfoMetadata.cpp

1584–1585

Can you walk me through in way way it's enforced that DIArgList is only referenced by MetadataAsValue?

I don't know that it is enforced currently, but I believe it was an intended restriction. The LLVM IR langref entry for DIArgList currently says (emphasis mine):

Because a DIArgList may refer to local values within a function, it must only be used as a function argument, must always be inlined, and cannot appear in named metadata.

The only way I can interpret the bold portion is to make it equivalent to "it must only be used through a MetadataAsValue", as that is the only means of using metadata as a function argument.

It sounds like textual IR has something, but can you confirm? What about the C++ API? The verifier? And what about historical bitcode... i.e., what can be found there?

Would it be appropriate to just add as a verifier check? No matter how the IR is produced it would then fail to verify, so we would cover any current or future APIs. For bitcode I've implemented an upgrade path to force IsDistinct=false when loading old bitcode.

If there are holes in enforcement somehow, what's going to happen to the dangling references to this node?
Can/should we delete this node now? And/or somehow mark it as dead, so that ValueAsMetadata will look through it?

I need to look closer at the lifetime of metadata nodes, it seems clear there is more to do here I'm just not sure exactly what that is.

[StephenTozer]

llvm/lib/IR/DebugInfoMetadata.cpp

1584–1585

Can you walk me through in way way it's enforced that DIArgList is only referenced by MetadataAsValue? It sounds like textual IR has something, but can you confirm? What about the C++ API? The verifier? And what about historical bitcode... i.e., what can be found there?

In textual IR parsing, a DIArgList cannot be parsed without a reference to PerFunctionState, which is provided for metadata exclusively when parsing the contents of MetadataAsValue. There is no explicit assertion to this effect, and there is also no assertion in Bitcode parsing or in the verifier either - this should probably be added! Although I don't think there should be any existing cases of this, as the Bitcode writer can also only write DIArgLists as part of a MetadataAsValue and this has been true since the metadata was added, so the only possible scenario would be someone manually editing a bitcode file I think.

[StephenTozer]

llvm/lib/IR/DebugInfoMetadata.cpp

1584–1585

Just to clarify, because it's slightly unclear in my last comment: I believe that the right move is to just add a verifier check. It doesn't really make sense for a DIArgList to exist outside of a MetadataAsValue in any circumstance, as it may refer to local function values and so must exist as a Value within a function, and the only way for that to happen is by being wrapped in a MetadataAsValue. As such, I don't think we need to put assertions all over the place to catch this, only to express it in the verifier as a rule of IR that will likely never be hit.

The equivalent assertion for LocalAsMetadata, which must be wrapped by MetadataAsValue for the same reason, is in Verifier::visitMDNode. I've also noticed while checking this that Verifier::visitMetadataAsValue does not call Verifier::visitDIArgList if it contains one, which should also be adjusted (this doesn't need to be a part of this patch).

[dexonsmith]

llvm/lib/IR/DebugInfoMetadata.cpp

1584–1585

Relying on the verifier is probably fine. I suspect you could add verifier coverage in a unit test (i.e., DIArgList in an invalid spot and then run the verifier), maybe in llvm/unittests/IR/MetadataTest.cpp or DebugInfoTest.cpp. It looks like that's missing for LocalAsMetadata (my fault I'm sure).

Still, IMO an assertion in MDOperand that the argument is valid to reference (i.e., neither LocalAsMetadata nor DIArgList) would be nice to have. Maybe Metadata could grow:

bool isValidOperand() const { return Kind != ... };

and then MDOperand::reset could check:

assert((!MD || MD->isValidMDOperand()) && "...");

If there are holes in enforcement somehow, what's going to happen to the dangling references to this node?
Can/should we delete this node now? And/or somehow mark it as dead, so that ValueAsMetadata will look through it?

I need to look closer at the lifetime of metadata nodes, it seems clear there is more to do here I'm just not sure exactly what that is.

Looking at the example of LocalAsMetadata, since DIArgList seems similar, ValueAsMetadata::handleRAUW() deletes the node after RAUW'ing it. It looks like DIArgList doesn't inherit from ReplaceableMetadataImpl so it can't RAUW itself, but maybe it should?

[scott.linder]

Add enforcement of the always-owned-by-MetadataAsValue property of DIArgList to
the metadata verifier. I'm not sure this is sufficient, but at least it is a
start.

Fixed the handling of the uniquify()!=this case in
DIArgList::handleChangedOperand to assert if the above property doesn't hold.

Add unittests for both changes above; the reproducers for this in IR rely on
specific sequences of optimizations, and would be much more fragile.

Is this moving in the right direction? Does the restriction on DIArgList seem
reasonable, and if so does it need more explicit documentation or more thorough
enforcement?

[scott.linder]

In D104827#3136642, @scott.linder wrote:

Add enforcement of the always-owned-by-MetadataAsValue property of DIArgList to
the metadata verifier. I'm not sure this is sufficient, but at least it is a
start.

Fixed the handling of the uniquify()!=this case in
DIArgList::handleChangedOperand to assert if the above property doesn't hold.

Add unittests for both changes above; the reproducers for this in IR rely on
specific sequences of optimizations, and would be much more fragile.

Is this moving in the right direction? Does the restriction on DIArgList seem
reasonable, and if so does it need more explicit documentation or more thorough
enforcement?

I pushed this before looking at the intervening comments, so the last bit is a bit redundant; it sounds like the restriction on DIArgList was intended and makes sense, so I think the remaining question is whether the Verifier check is sufficient.

[dexonsmith]

In D104827#3137807, @scott.linder wrote:

Is this moving in the right direction? Does the restriction on DIArgList seem
reasonable, and if so does it need more explicit documentation or more thorough
enforcement?

I pushed this before looking at the intervening comments, so the last bit is a bit redundant; it sounds like the restriction on DIArgList was intended and makes sense, so I think the remaining question is whether the Verifier check is sufficient.

It seems like DIArgList is very similar to LocalAsMetadata. I feel like it'd be less error-prone for it to use/inherit-from ReplaceableMetadataImpl, just like ValueAsMetadata does. Then it has the machinery to RAUW/delete itself when necessary.

(Moreover, given that it doesn't have any operands, I find it suspicious that it inherits (albeit indirectly) from MDNode. It should probably be a direct subclass of Metadata. This would mean it couldn't inherit from DINode which might complicate some debug info code; if you're going to clean that up I suggest doing it later as a separate patch.)

[scott.linder]

In D104827#3138627, @dexonsmith wrote:

In D104827#3137807, @scott.linder wrote:

Is this moving in the right direction? Does the restriction on DIArgList seem
reasonable, and if so does it need more explicit documentation or more thorough
enforcement?

I pushed this before looking at the intervening comments, so the last bit is a bit redundant; it sounds like the restriction on DIArgList was intended and makes sense, so I think the remaining question is whether the Verifier check is sufficient.

It seems like DIArgList is very similar to LocalAsMetadata. I feel like it'd be less error-prone for it to use/inherit-from ReplaceableMetadataImpl, just like ValueAsMetadata does. Then it has the machinery to RAUW/delete itself when necessary.

(Moreover, given that it doesn't have any operands, I find it suspicious that it inherits (albeit indirectly) from MDNode. It should probably be a direct subclass of Metadata. This would mean it couldn't inherit from DINode which might complicate some debug info code; if you're going to clean that up I suggest doing it later as a separate patch.)

I looked into deriving DIArgList from ReplaceableMetadataImpl, and it does get awkward quickly that it is an MDNode. It seems like there is a fair amount of code around tracking/RAUW which relies on MDNodes being eventually "resolveable". The result is that I don't think I can inherit from ReplaceableMetadataImpl without also changing the inheritence from MDNode to Metadata as you suggest.

I'll look at what effect that has on any debug-info related code, but I do worry that no matter what there will be a fair amount of special-casing.

[scott.linder]

I'm pushing the (rough) patch which tries to make the switch from inheriting
DIArglist from MDNode to ReplaceableMetadataImpl.

The biggest issue I see with a change along these lines is that the
!DIArgList syntax becomes misleading. If we really want this, I think it
implies a bit more work, namely generalizing this to something closer to a
ValuesAsMetadata (plural), with a syntax closer to:

metadata {i32 1, i64 %x, float %f}

I think this doesn't add any ambiguity to the syntax, but this is just my
initial guess at what might make sense.

I'm not sure I can commit to this work, but I would still like to propose making
the UNIQUED/DISTINCT change for DIExpression and DICompileUnit. Would it be
reasonable for me to just remove DIArgList from my original patch, as it has
proven to be special enough not to generalize how I had hoped?

[scott.linder]

I went ahead with just leaving DIArgList alone, let me know what you think

[dexonsmith]

In D104827#3189622, @scott.linder wrote:

I'm pushing the (rough) patch which tries to make the switch from inheriting
DIArglist from MDNode to ReplaceableMetadataImpl.

The biggest issue I see with a change along these lines is that the
!DIArgList syntax becomes misleading.

I don't see what's misleading about it. ! is the indicator that "metadata follows", and DIArgList is the name of this special kind of metadata.

TBH, I find it odd that there'd be something inheriting from MDNode that is as special as this....

If we really want this, I think it
implies a bit more work, namely generalizing this to something closer to a
ValuesAsMetadata (plural), with a syntax closer to:

metadata {i32 1, i64 %x, float %f}

I think this doesn't add any ambiguity to the syntax, but this is just my
initial guess at what might make sense.

This looks awkwardly similar to generic MDNode syntax (just with the distinction that generic metadata is not allowed to reference local variables). I don't think generic syntax is better than naming the type here. If you still disagree, can you explain why?

I'm not sure I can commit to this work, but I would still like to propose making
the UNIQUED/DISTINCT change for DIExpression and DICompileUnit. Would it be
reasonable for me to just remove DIArgList from my original patch, as it has
proven to be special enough not to generalize how I had hoped?

IIUC, DIExpression is data-driven, so it's reasonable for it always to be uniqued. Originally it was not going to inherit from MDNode, but IIRC it was useful as an incremental step for the transition from all generic metadata. At some point there was a FIXME in the code to reparent directly under Metadata but probably that was dropped eventually. I still think that'd be a reasonable thing to do.

If DICompileUnit already has this property, seems to me like that part could be separated into an NFC change to make it easier to review what's actually changing.

Seems reasonable to handle DIArgList separately from the other two. It'd be a shame to drop it entirely; it seems like there are some fundamental modelling problems with DIArgList that would be valuable to iron out.

llvm/lib/Bitcode/Reader/MetadataLoader.cpp

558

I don't think you need to change upgradeDIExpression.

1143

Seems odd that this is declared here. Would be nice at some point to make it local to each case below that needs it in an NFC change.

1982

I think you should replace this line with a comment, maybe something like:

// DIExpression is never distinct. Old bitcode uses the 0-bit for that
// but it's now ignored.

You can drop the assignment to IsDistinct entirely; it's not interesting here anymore.

1987–1992

I don't think you need this change.

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

2012

I'm not sure we need a new version.

llvm/lib/CodeGen/MIRParser/MIParser.cpp

1162

I'm not sure what you mean by this comment. What is "this", and why should a metadata kind being uniqued-by-content influence it?

2179

I'm not sure what you mean by this comment. What is "this", and why should a metadata kind being uniqued-by-content influence it?

llvm/lib/IR/AsmWriter.cpp

1304–1308

I don't think nodes being uniqued-by-content should force them to be inlined everywhere. Those seem like independent properties to me.

llvm/lib/IR/DebugInfoMetadata.cpp

1056

Can this be Storage == Uniqued?

llvm/lib/IR/Verifier.cpp

936 ↗

(On Diff #394023)

Nit: if we add a newline here, should be in a separate NFC commit since there's no other change around.

[scott.linder]

In D104827#3140973, @scott.linder wrote:

In D104827#3138627, @dexonsmith wrote:

In D104827#3137807, @scott.linder wrote:

Is this moving in the right direction? Does the restriction on DIArgList seem
reasonable, and if so does it need more explicit documentation or more thorough
enforcement?

I pushed this before looking at the intervening comments, so the last bit is a bit redundant; it sounds like the restriction on DIArgList was intended and makes sense, so I think the remaining question is whether the Verifier check is sufficient.

It seems like DIArgList is very similar to LocalAsMetadata. I feel like it'd be less error-prone for it to use/inherit-from ReplaceableMetadataImpl, just like ValueAsMetadata does. Then it has the machinery to RAUW/delete itself when necessary.

(Moreover, given that it doesn't have any operands, I find it suspicious that it inherits (albeit indirectly) from MDNode. It should probably be a direct subclass of Metadata. This would mean it couldn't inherit from DINode which might complicate some debug info code; if you're going to clean that up I suggest doing it later as a separate patch.)

I looked into deriving DIArgList from ReplaceableMetadataImpl, and it does get awkward quickly that it is an MDNode. It seems like there is a fair amount of code around tracking/RAUW which relies on MDNodes being eventually "resolveable". The result is that I don't think I can inherit from ReplaceableMetadataImpl without also changing the inheritence from MDNode to Metadata as you suggest.

I'll look at what effect that has on any debug-info related code, but I do worry that no matter what there will be a fair amount of special-casing.

In D104827#3192965, @dexonsmith wrote:

In D104827#3189622, @scott.linder wrote:

I'm pushing the (rough) patch which tries to make the switch from inheriting
DIArglist from MDNode to ReplaceableMetadataImpl.

The biggest issue I see with a change along these lines is that the
!DIArgList syntax becomes misleading.

I don't see what's misleading about it. ! is the indicator that "metadata follows", and DIArgList is the name of this special kind of metadata.

TBH, I find it odd that there'd be something inheriting from MDNode that is as special as this....

It seemed like a useful invariant that the syntax !<id> always implied an MDNode, in the same way that !"..." means MDString, although I guess this is just an implementation detail.

It may just be a naming thing, though; we say in the langref that there are only metadata "strings" and metadata "nodes", but we already have metadata "ValueAsMetadata"s too. If we were to hoist up DIArgList as well how would we want to describe it? By saying we have: strings, nodes, ValueAsMetadatas, and DIArgLists?

If we really want this, I think it
implies a bit more work, namely generalizing this to something closer to a
ValuesAsMetadata (plural), with a syntax closer to:

metadata {i32 1, i64 %x, float %f}

I think this doesn't add any ambiguity to the syntax, but this is just my
initial guess at what might make sense.

This looks awkwardly similar to generic MDNode syntax (just with the distinction that generic metadata is not allowed to reference local variables). I don't think generic syntax is better than naming the type here. If you still disagree, can you explain why?

That was my intention, because I don't see why this needs to be DI-centric. My guess was wrong either way, though, because metadata { already signifies the start of a structure type IIUC.

I was just trying to suggest that all DIArgList currently does is stand in for a function-local metadata tuple, so just making one of those and using that seems more straightforward.

I'm not sure I can commit to this work, but I would still like to propose making
the UNIQUED/DISTINCT change for DIExpression and DICompileUnit. Would it be
reasonable for me to just remove DIArgList from my original patch, as it has
proven to be special enough not to generalize how I had hoped?

IIUC, DIExpression is data-driven, so it's reasonable for it always to be uniqued. Originally it was not going to inherit from MDNode, but IIRC it was useful as an incremental step for the transition from all generic metadata. At some point there was a FIXME in the code to reparent directly under Metadata but probably that was dropped eventually. I still think that'd be a reasonable thing to do.

That FIXME is still present, and if that is true then maybe my whole conception of the class hierarchy and what to call various parts is just not lining up with reality. As a smoke test for whether I'm on the same page, would you expect DIExpression to no longer be referred to as a "node" in documentation, as it would no longer derive from MDNode?

If DICompileUnit already has this property, seems to me like that part could be separated into an NFC change to make it easier to review what's actually changing.

Sure; just to confirm, you mean split this into two patches, one NFC patch adding HANDLE_MDNODE_LEAF_DISTINCT, and another adding HANDLE_MDNODE_LEAF_UNIQUED?

Seems reasonable to handle DIArgList separately from the other two. It'd be a shame to drop it entirely; it seems like there are some fundamental modelling problems with DIArgList that would be valuable to iron out.

Yes, I agree that at the very least the fact that DIArgList can transiently become distinct, but then not round-trip this property through IR, probably deserves attention. I am not particularly confident that the way I first though about remedying it really lines up with how metadata is currently designed, but I would be interested in learning enough to contribute a fix.

I'll wait to hear if I follow how you would prefer the patch be split up before I incorporate your other feedback.

llvm/lib/Bitcode/Reader/MetadataLoader.cpp

1982

OK, fair enough; so the bit just becomes "don't care" going forward, which naturally means old bitcode is forward compatible

llvm/lib/CodeGen/MIRParser/MIParser.cpp

1162

"this" is trying to get at "this code which supports DIExpression appearing inline in MIR"; I can try to fix up the wording some to be more clear

The property I was aiming for with the whole patch was that "inlined" metadata is never distinct. This seemed like a logical "rule" that would explain the lack of syntax for distinct nodes inline.

2179

ditto

llvm/lib/IR/DebugInfoMetadata.cpp

1056

Yes, I'll make this change.

llvm/lib/IR/Verifier.cpp

936 ↗

(On Diff #394023)

Woops, I just didn't notice this before pushing the diff. I'll just remove this change from the patch.

[dexonsmith]

In D104827#3193518, @scott.linder wrote:

I was just trying to suggest that all DIArgList currently does is stand in for a function-local metadata tuple, so just making one of those and using that seems more straightforward.

Ah, that's an interesting point; thanks for clarifying!

Stepping back a bit...

Before the big metadata / debug info refactor, there was support for metadata nodes owned by a function. These nodes were allowed to point at function-local values. The only uses of these metadata nodes were for direct reference from debug info intrinsics. I removed them entirely to simplify the model, and left behind LocalAsMetadata.

I've since realized that much of this "global" metadata is really tied to a specific function. Three examples:

• Inlined DILocations were made distinct in order to defeat uniquing across different function definitions.
• DISubprogram have become function attachments; and it's illegal to have the same one attached in multiple places. It's effectively owned by the function; and it needs to be cloned when a function is cloned.
• Outside of debug info, there are optimization-related metadata that describe function-local constructs, and that defeat uniquing by having a "root" node that self-references and/or is marked distinct. (IIRC, loop metadata is an example?)

I think we should bring back local metadata; moreover, there really seem to be three scopes for metadata:

• Context: pure / immutable constants; cannot change / cannot be distinct. Only reference other "context" constants (any referenced LLVM values are also pure constants (i32 0 okay, but i32* @global is not)). Should be shared across all users of an LLVMContext and owned by the LLVMContext. Since they are immutable, never "cloned" by cloneFunctionInto / cloneModule / related APIs.
• Module: module-specific. May have identity and be mutable (distinct). If it is uniqued / "constant", then it is at "module" scope because it (transitively) references something else with "module" scope (e.g., a global value or distinct metadata). Can reference LLVM global values but not local values. These are effectively owned by a specific Module. Cloning a module would require making new copies of these, but cloning a function would not.
• Local: function-specific. May have identity and be mutable (distinct). If it is uniqued / "constant", then it is at "local" scope because it (transitively) references something else with "local" scope. Cloning a function would require making new copies of these. Effectively owned by the function/global value that it's associated with.

In this model, it seems like DIArgList falls under "local"; and you're suggesting it's just a stand-in for not having a local "MDNode", and not really semantically tied to debug info or argument lists. If that's the case, then it seems they're only required to be inline right now because there's no syntax to describe them otherwise. Syntax could look like:

define void @f(i32 %a, i32 %b) {
  call @llvm.some.intrinsic(!%1)
  ; Some local metadata. Can reference LocalAsMetadata.
  ; Cloned when the function is cloned.
  !%1 = metadata !{i32 %a, i32 %b, !%2}
  !%2 = distinct metadata !{!1}
}

; Global metadata. Cloned when a module is cloned.
!1 = distinct metadata !{!2, ptr @f}
!2 = metadata !{!1, !3}

; Context metadata. Pure constants.
!3 = metadata !{!4, !5}
!4 = metadata !{i32 1, ptr null}
!5 = metadata !{}

Stepping back again, I think there are three kinds of identity for metadata:

• Distinct: not in any uniquing table (e.g., MDNode marked distinct)
• Uniqued (always): in a uniquing table; either immutable (e.g., "Context" scope / DIExpression) or has permanent RAUW support to handle changes (e.g., ValueAsMetadata)
• Uniqued (best-effort): findable via uniquing tables, but operands might change and lacks permanent RAUW support; degrades to "distinct" if an operand changes (e.g., MDNode without distinct)

The current design for metadata optimizes for having most metadata either "distinct" or "best-effort-uniqued". Permanent RAUW support is expensive (in this design).

Our recent discussions about DIArgList point toward making it always-uniqued; since the operands can change, this requires permanent RAUW support. I wonder if, instead, best-effort-uniqued would be sufficient... if only there were syntax for it. WDYT?

Sure; just to confirm, you mean split this into two patches, one NFC patch adding HANDLE_MDNODE_LEAF_DISTINCT, and another adding HANDLE_MDNODE_LEAF_UNIQUED?

Yes.

For the former, maybe "ALWAYS_DISTINCT" would be more clear? Either way, this seems like a well-defined concept.

For the latter, I'm not quite convinced yet. The name doesn't seem right, since most nodes are stored/de-duped in uniquing tables (maybe "ALWAYS_UNIQUED" would work?). But:

• I don't think "always-uniqued" should imply things like "printed inline in textual IR"
• I'm not sure should have MDNodes that have permanent RAUW support (since it's expensive)
  • Can we get away with a "local" MDNode that is best-effort-uniqued?

llvm/lib/CodeGen/MIRParser/MIParser.cpp

1162

We could support all nodes here for parsing, if we think that's useful... I'm not sure if there's a good reason to only have DIExpression here...

that would explain the lack of syntax for distinct nodes inline.

IIRC, the historical lack of syntax (in .ll at least, not sure about .mir) is because it didn't seem useful/important. When MDNodes are distinct, it is because they either need identity *a priori* (in which case, probably there are multiple references to them and they MUST be out-of-line) or there was a uniquing table collision for best-effort-uniqued metadata (expected to be rare). Anything that was always-uniqued had first class syntax.

llvm/docs/LangRef.rst

 These aren't inherently debug info centric, but currently all the specialized
 metadata nodes are related to debug info.
 
 .. _DICompileUnit:
 
 DICompileUnit
 """""""""""""
 
-``DICompileUnit`` nodes represent a compile unit. The ``enums:``,
-``retainedTypes:``, ``globals:``, ``imports:`` and ``macros:`` fields are tuples
-containing the debug info to be emitted along with the compile unit, regardless
-of code optimizations (some nodes are only emitted if there are references to
-them from instructions). The ``debugInfoForProfiling:`` field is a boolean
-indicating whether or not line-table discriminators are updated to provide
-more-accurate debug info for profiling results.
+``DICompileUnit`` nodes represent a compile unit. ``DICompileUnit`` nodes must
+be ``distinct``. The ``enums:``, ``retainedTypes:``, ``globals:``, ``imports:``
+and ``macros:`` fields are tuples containing the debug info to be emitted along
+with the compile unit, regardless of code optimizations (some nodes are only
+emitted if there are references to them from instructions). The
+``debugInfoForProfiling:`` field is a boolean indicating whether or not
+line-table discriminators are updated to provide more-accurate debug info for
+profiling results.
 
 .. code-block:: text
 
-    !0 = !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang",
+    !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang",
                                  isOptimized: true, flags: "-O2", runtimeVersion: 2,
                                  splitDebugFilename: "abc.debug", emissionKind: FullDebug,
                                  enums: !2, retainedTypes: !3, globals: !4, imports: !5,
                                  macros: !6, dwoId: 0x0abcd)
 
 Compile unit descriptors provide the root scope for objects declared in a
 specific compilation unit. File descriptors are defined using this scope.  These
 descriptors are collected by a named metadata node ``!llvm.dbg.cu``. They keep
 track of global variables, type information, and imported entities (declarations
 and namespaces).
 
 .. _DIFile:
.. code-block:: text
     !2 = !DILocalVariable(name: "y", scope: !5, file: !2, line: 7, type: !3)
 
 .. _DIExpression:
 
 DIExpression
 """"""""""""
 
 ``DIExpression`` nodes represent expressions that are inspired by the DWARF
-expression language. They are used in :ref:`debug intrinsics<dbg_intrinsics>`
-(such as ``llvm.dbg.declare`` and ``llvm.dbg.value``) to describe how the
-referenced LLVM variable relates to the source language variable. Debug
-intrinsics are interpreted left-to-right: start by pushing the value/address
-operand of the intrinsic onto a stack, then repeatedly push and evaluate
-opcodes from the DIExpression until the final variable description is produced.
+expression language. ``DIExpression`` nodes must not be ``distinct``, and are
+canonically printed inline at each use. They are used in :ref:`debug
+intrinsics<dbg_intrinsics>` (such as ``llvm.dbg.declare`` and
+``llvm.dbg.value``) to describe how the referenced LLVM variable relates to the
+source language variable. Debug intrinsics are interpreted left-to-right: start
+by pushing the value/address operand of the intrinsic onto a stack, then
+repeatedly push and evaluate opcodes from the DIExpression until the final
+variable description is produced.
 
 The current supported opcode vocabulary is limited:
 
 - ``DW_OP_deref`` dereferences the top of the expression stack.
 - ``DW_OP_plus`` pops the last two entries from the expression stack, adds
   them together and appends the result to the expression stack.
 - ``DW_OP_minus`` pops the last two entries from the expression stack, subtracts
   the last entry from the second last entry and appends the result to the
- ``DW_OP_LLVM_implicit_pointer`` It specifies the dereferenced value. It can
   it points to is known. This operator is required as it is different than DWARF
   operator DW_OP_implicit_pointer in representation and specification (number
   and types of operands) and later can not be used as multiple level.
 
 .. code-block:: text
 
     IR for "*ptr = 4;"
     --------------
-    call void @llvm.dbg.value(metadata i32 4, metadata !17, metadata !20)
+    call void @llvm.dbg.value(metadata i32 4, metadata !17,
+                              metadata !DIExpression(DW_OP_LLVM_implicit_pointer)))
     !17 = !DILocalVariable(name: "ptr1", scope: !12, file: !3, line: 5,
                            type: !18)
     !18 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !19, size: 64)
     !19 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
-    !20 = !DIExpression(DW_OP_LLVM_implicit_pointer))
 
     IR for "**ptr = 4;"
     --------------
-    call void @llvm.dbg.value(metadata i32 4, metadata !17, metadata !21)
+    call void @llvm.dbg.value(metadata i32 4, metadata !17,
+                              metadata !DIExpression(DW_OP_LLVM_implicit_pointer,
+                                                     DW_OP_LLVM_implicit_pointer)))
     !17 = !DILocalVariable(name: "ptr1", scope: !12, file: !3, line: 5,
                            type: !18)
     !18 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !19, size: 64)
     !19 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !20, size: 64)
     !20 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
-    !21 = !DIExpression(DW_OP_LLVM_implicit_pointer,
-                        DW_OP_LLVM_implicit_pointer))
 
 DWARF specifies three kinds of simple location descriptions: Register, memory,
 and implicit location descriptions.  Note that a location description is
 defined over certain ranges of a program, i.e the location of a variable may
 change over the course of the program. Register and memory location
 descriptions describe the *concrete location* of a source variable (in the
 sense that a debugger might modify its value), whereas *implicit locations*
 describe merely the actual *value* of a source variable which might not exist
.. code-block:: text
     !2 = !DIExpression(DW_OP_bit_piece, 3, 7)
     !3 = !DIExpression(DW_OP_deref, DW_OP_constu, 3, DW_OP_plus, DW_OP_LLVM_fragment, 3, 7)
     !4 = !DIExpression(DW_OP_constu, 2, DW_OP_swap, DW_OP_xderef)
     !5 = !DIExpression(DW_OP_constu, 42, DW_OP_stack_value)
 
 DIArgList
 """"""""""""
 
-``DIArgList`` nodes hold a list of constant or SSA value references. These are
-used in :ref:`debug intrinsics<dbg_intrinsics>` (currently only in
+``DIArgList`` nodes hold a list of constant or SSA value references.
+``DIArgList`` must not be ``distinct``, must only be used as an argument to a
+function call, and must appear inline at each use. ``DIArgList`` may refer to
+function-local values of the containing function. ``DIArgList`` nodes are used
+in :ref:`debug intrinsics<dbg_intrinsics>` (currently only in
 ``llvm.dbg.value``) in combination with a ``DIExpression`` that uses the
-``DW_OP_LLVM_arg`` operator. Because a DIArgList may refer to local values
-within a function, it must only be used as a function argument, must always be
-inlined, and cannot appear in named metadata.
+``DW_OP_LLVM_arg`` operator.
 
 .. code-block:: text
 
     llvm.dbg.value(metadata !DIArgList(i32 %a, i32 %b),
                    metadata !16,
                    metadata !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_plus))
 
 DIFlags

llvm/docs/SourceLevelDebugging.rst

 .. code-block:: text
 
   ; Function Attrs: nounwind ssp uwtable
   define void @foo() #0 !dbg !4 {
   entry:
     %X = alloca i32, align 4
     %Y = alloca i32, align 4
     %Z = alloca i32, align 4
-    call void @llvm.dbg.declare(metadata i32* %X, metadata !11, metadata !13), !dbg !14
-    store i32 21, i32* %X, align 4, !dbg !14
-    call void @llvm.dbg.declare(metadata i32* %Y, metadata !15, metadata !13), !dbg !16
-    store i32 22, i32* %Y, align 4, !dbg !16
-    call void @llvm.dbg.declare(metadata i32* %Z, metadata !17, metadata !13), !dbg !19
-    store i32 23, i32* %Z, align 4, !dbg !19
-    %0 = load i32, i32* %X, align 4, !dbg !20
-    store i32 %0, i32* %Z, align 4, !dbg !21
-    %1 = load i32, i32* %Y, align 4, !dbg !22
-    store i32 %1, i32* %X, align 4, !dbg !23
-    ret void, !dbg !24
+    call void @llvm.dbg.declare(metadata i32* %X, metadata !11, metadata !DIExpression()), !dbg !13
+    store i32 21, i32* %X, align 4, !dbg !13
+    call void @llvm.dbg.declare(metadata i32* %Y, metadata !14, metadata !DIExpression()), !dbg !15
+    store i32 22, i32* %Y, align 4, !dbg !15
+    call void @llvm.dbg.declare(metadata i32* %Z, metadata !16, metadata !DIExpression()), !dbg !18
+    store i32 23, i32* %Z, align 4, !dbg !18
+    %0 = load i32, i32* %X, align 4, !dbg !19
+    store i32 %0, i32* %Z, align 4, !dbg !20
+    %1 = load i32, i32* %Y, align 4, !dbg !21
+    store i32 %1, i32* %X, align 4, !dbg !22
+    ret void, !dbg !23
   }
 
   ; Function Attrs: nounwind readnone
   declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
 
   attributes #0 = { nounwind ssp uwtable "less-precise-fpmad"="false" "frame-pointer"="all" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
   attributes #1 = { nounwind readnone }
 
.. code-block:: text
   !5 = !DISubroutineType(types: !6)
   !6 = !{null}
   !7 = !{i32 2, !"Dwarf Version", i32 2}
   !8 = !{i32 2, !"Debug Info Version", i32 3}
   !9 = !{i32 1, !"PIC Level", i32 2}
   !10 = !{!"clang version 3.7.0 (trunk 231150) (llvm/trunk 231154)"}
   !11 = !DILocalVariable(name: "X", scope: !4, file: !1, line: 2, type: !12)
   !12 = !DIBasicType(name: "int", size: 32, align: 32, encoding: DW_ATE_signed)
-  !13 = !DIExpression()
-  !14 = !DILocation(line: 2, column: 9, scope: !4)
-  !15 = !DILocalVariable(name: "Y", scope: !4, file: !1, line: 3, type: !12)
-  !16 = !DILocation(line: 3, column: 9, scope: !4)
-  !17 = !DILocalVariable(name: "Z", scope: !18, file: !1, line: 5, type: !12)
-  !18 = distinct !DILexicalBlock(scope: !4, file: !1, line: 4, column: 5)
-  !19 = !DILocation(line: 5, column: 11, scope: !18)
-  !20 = !DILocation(line: 6, column: 11, scope: !18)
-  !21 = !DILocation(line: 6, column: 9, scope: !18)
-  !22 = !DILocation(line: 8, column: 9, scope: !4)
-  !23 = !DILocation(line: 8, column: 7, scope: !4)
-  !24 = !DILocation(line: 9, column: 3, scope: !4)
+  !13 = !DILocation(line: 2, column: 9, scope: !4)
+  !14 = !DILocalVariable(name: "Y", scope: !4, file: !1, line: 3, type: !12)
+  !15 = !DILocation(line: 3, column: 9, scope: !4)
+  !16 = !DILocalVariable(name: "Z", scope: !17, file: !1, line: 5, type: !12)
+  !17 = distinct !DILexicalBlock(scope: !4, file: !1, line: 4, column: 5)
+  !18 = !DILocation(line: 5, column: 11, scope: !17)
+  !19 = !DILocation(line: 6, column: 11, scope: !17)
+  !20 = !DILocation(line: 6, column: 9, scope: !17)
+  !21 = !DILocation(line: 8, column: 9, scope: !4)
+  !22 = !DILocation(line: 8, column: 7, scope: !4)
+  !23 = !DILocation(line: 9, column: 3, scope: !4)
 
 
 This example illustrates a few important details about LLVM debugging
 information.  In particular, it shows how the ``llvm.dbg.declare`` intrinsic and
 location information, which are attached to an instruction, are applied
 together to allow a debugger to analyze the relationship between statements,
 variable definitions, and the code used to implement the function.
 
 .. code-block:: llvm
 
-  call void @llvm.dbg.declare(metadata i32* %X, metadata !11, metadata !13), !dbg !14
+  call void @llvm.dbg.declare(metadata i32* %X, metadata !11, metadata !DIExpression()), !dbg !13
     ; [debug line = 2:7] [debug variable = X]
 
 The first intrinsic ``%llvm.dbg.declare`` encodes debugging information for the
-variable ``X``.  The metadata ``!dbg !14`` attached to the intrinsic provides
+variable ``X``.  The metadata ``!dbg !13`` attached to the intrinsic provides
 scope information for the variable ``X``.
 
 .. code-block:: text
 
-  !14 = !DILocation(line: 2, column: 9, scope: !4)
+  !13 = !DILocation(line: 2, column: 9, scope: !4)
   !4 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 1, type: !5,
                               isLocal: false, isDefinition: true, scopeLine: 1,
                               isOptimized: false, variables: !2)
 
-Here ``!14`` is metadata providing `location information
+Here ``!13`` is metadata providing `location information
 <LangRef.html#dilocation>`_.  In this example, scope is encoded by ``!4``, a
 `subprogram descriptor <LangRef.html#disubprogram>`_.  This way the location
 information attached to the intrinsics indicates that the variable ``X`` is
 declared at line number 2 at a function level scope in function ``foo``.
 
 Now lets take another example.
 
 .. code-block:: llvm
 
-  call void @llvm.dbg.declare(metadata i32* %Z, metadata !17, metadata !13), !dbg !19
+  call void @llvm.dbg.declare(metadata i32* %Z, metadata !16, metadata !DIExpression()), !dbg !18
     ; [debug line = 5:9] [debug variable = Z]
 
 The third intrinsic ``%llvm.dbg.declare`` encodes debugging information for
-variable ``Z``.  The metadata ``!dbg !19`` attached to the intrinsic provides
+variable ``Z``.  The metadata ``!dbg !18`` attached to the intrinsic provides
 scope information for the variable ``Z``.
 
 .. code-block:: text
 
-  !18 = distinct !DILexicalBlock(scope: !4, file: !1, line: 4, column: 5)
-  !19 = !DILocation(line: 5, column: 11, scope: !18)
+  !17 = distinct !DILexicalBlock(scope: !4, file: !1, line: 4, column: 5)
+  !18 = !DILocation(line: 5, column: 11, scope: !17)
 
-Here ``!19`` indicates that ``Z`` is declared at line number 5 and column
-number 11 inside of lexical scope ``!18``.  The lexical scope itself resides
+Here ``!18`` indicates that ``Z`` is declared at line number 5 and column
+number 11 inside of lexical scope ``!17``.  The lexical scope itself resides
 inside of subprogram ``!4`` described above.
 
 The scope information attached with each instruction provides a straightforward
 way to find instructions covered by a scope.
 
 Object lifetime in optimized code
 =================================
 
.. code-block:: text
   entry:
     br i1 %cond, label %truebr, label %falsebr
 
   bb1: 
     %value = phi i32 [ %value1, %truebr ], [ %value2, %falsebr ]
     br label %exit, !dbg !26
 
   truebr:
-    call void @llvm.dbg.value(metadata i32 %input, metadata !30, metadata !DIExpression()), !dbg !24
-    call void @llvm.dbg.value(metadata i32 1, metadata !23, metadata !DIExpression()), !dbg !24
+    call void @llvm.dbg.value(metadata i32 %input, metadata !30, metadata !DIExpression()), !dbg !23
+    call void @llvm.dbg.value(metadata i32 1, metadata !22, metadata !DIExpression()), !dbg !23
     %value1 = add i32 %input, 1
     br label %bb1
 
   falsebr:
-    call void @llvm.dbg.value(metadata i32 %input, metadata !30, metadata !DIExpression()), !dbg !24
-    call void @llvm.dbg.value(metadata i32 2, metadata !23, metadata !DIExpression()), !dbg !24
+    call void @llvm.dbg.value(metadata i32 %input, metadata !30, metadata !DIExpression()), !dbg !23
+    call void @llvm.dbg.value(metadata i32 2, metadata !22, metadata !DIExpression()), !dbg !23
     %value = add i32 %input, 2
     br label %bb1
 
   exit: 
     ret i32 %value, !dbg !30
   }
 
 Here the difficulties are:
 
 * The control flow is roughly the opposite of basic block order
-* The value of the ``!23`` variable merges into ``%bb1``, but there is no PHI
+* The value of the ``!22`` variable merges into ``%bb1``, but there is no PHI
   node
 
 As mentioned above, the ``llvm.dbg.value`` intrinsics essentially form an
 imperative program embedded in the IR, with each intrinsic defining a variable
 location. This *could* be converted to an SSA form by mem2reg, in the same way
 that it uses use-def chains to identify control flow merges and insert phi
 nodes for IR Values. However, because debug variable locations are defined for
 every machine instruction, in effect every IR instruction uses every variable
 location, which would lead to a large number of debugging intrinsics being
 generated.
 
 Examining the example above, variable ``!30`` is assigned ``%input`` on both
-conditional paths through the function, while ``!23`` is assigned differing
+conditional paths through the function, while ``!22`` is assigned differing
 constant values on either path. Where control flow merges in ``%bb1`` we would
-want ``!30`` to keep its location (``%input``), but ``!23`` to become undefined
+want ``!30`` to keep its location (``%input``), but ``!22`` to become undefined
 as we cannot determine at runtime what value it should have in %bb1 without
 inserting a PHI node. mem2reg does not insert the PHI node to avoid changing
 codegen when debugging is enabled, and does not insert the other dbg.values
 to avoid adding very large numbers of intrinsics.
 
 Instead, LiveDebugValues determines variable locations when control
 flow merges. A dataflow analysis is used to propagate locations between blocks:
 when control flow merges, if a variable has the same location in all
 predecessors then that location is propagated into the successor. If the
 predecessor locations disagree, the location becomes undefined.
 
 Once LiveDebugValues has run, every block should have all valid variable
 locations described by DBG_VALUE instructions within the block. Very little
 effort is then required by supporting classes (such as
 DbgEntityHistoryCalculator) to build a map of each instruction to every
 valid variable location, without the need to consider control flow. From
 the example above, it is otherwise difficult to determine that the location
 of variable ``!30`` should flow "up" into block ``%bb1``, but that the location
-of variable ``!23`` should not flow "down" into the ``%exit`` block.
+of variable ``!22`` should not flow "down" into the ``%exit`` block.
 
 .. _ccxx_frontend:
 
 C/C++ front-end specific debug information
 ==========================================
 
 The C and C++ front-ends represent information about the program in a
 format that is effectively identical to `DWARF <http://www.dwarfstd.org/>`_

llvm/include/llvm/AsmParser/LLParser.h

private:
     bool parseOptionalFunctionMetadata(Function &F);
 
     template <class FieldTy>
     bool parseMDField(LocTy Loc, StringRef Name, FieldTy &Result);
     template <class FieldTy> bool parseMDField(StringRef Name, FieldTy &Result);
     template <class ParserTy> bool parseMDFieldsImplBody(ParserTy ParseField);
     template <class ParserTy>
     bool parseMDFieldsImpl(ParserTy ParseField, LocTy &ClosingLoc);
-    bool parseSpecializedMDNode(MDNode *&N, bool IsDistinct = false);
+    bool parseSpecializedMDNode(MDNode *&N, bool IsDistinct = false,
+                                LocTy DistinctLoc = LocTy());
 
 #define HANDLE_SPECIALIZED_MDNODE_LEAF(CLASS)                                  \
   bool parse##CLASS(MDNode *&Result, bool IsDistinct);
 #include "llvm/IR/Metadata.def"
     bool parseDIArgList(MDNode *&Result, bool IsDistinct,
                         PerFunctionState *PFS);
 
     // Function Parsing.

llvm/include/llvm/IR/Metadata.def

 //===- llvm/IR/Metadata.def - Metadata definitions --------------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 //
 // Macros for running through all types of metadata.
 //
+// Definitions for terms used to describe metadata include:
+//
+// * BRANCH: refers to an "abstract" metadata kind, which exists only in the
+//   C++ class hierarchy. These cannot appear directly in IR/bitcode.
+// * LEAF: refers to a "concrete" metadata kind. These can appear directly in
+//   IR/bitcode.
+// * SPECIALIZED: refers to non-MDTuple MDNodes, i.e. those that use the
+//   syntax "!CLASS(...)" in IR.
+// * UNIQUABLE: refers to nodes which can use uniqued, distinct, or temporary
+//   storage without any restrictions.
+// * UNIQUED: refers to nodes which must use uniqued or temporary storage.
+// * DISTINCT: refers to nodes which must use distinct or temporary storage.
+//
+// In LLVM IR, UNIQUABLE and DISTINCT nodes must be referred to by MDNode ID,
+// as in `!0`, whereas UNIQUED nodes canonically appear inline at each use, as
+// in `DIExpression(...)`. This is because `distinct` nodes maintain their
+// identity irrespective of contents, making the inline syntax ambiguous in
+// some cases.
+//
+// Note: UNIQUABLE, UNIQUED, and DISTINCT are mutually exclusive. For example,
+// code which intends to consider all nodes which can use uniqued storage must
+// consider both UNIQUABLE and UNIQUED nodes.
+//
 //===----------------------------------------------------------------------===//
 
 #if !(defined HANDLE_METADATA || defined HANDLE_METADATA_LEAF ||               \
       defined HANDLE_METADATA_BRANCH || defined HANDLE_MDNODE_LEAF ||          \
-      defined HANDLE_MDNODE_LEAF_UNIQUABLE || defined HANDLE_MDNODE_BRANCH ||  \
+      defined HANDLE_MDNODE_LEAF_UNIQUABLE ||                                  \
+      defined HANDLE_MDNODE_LEAF_UNIQUED ||                                    \
+      defined HANDLE_MDNODE_LEAF_DISTINCT || defined HANDLE_MDNODE_BRANCH ||   \
       defined HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE ||                      \
+      defined HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUED ||                        \
+      defined HANDLE_SPECIALIZED_MDNODE_LEAF_DISTINCT ||                       \
       defined HANDLE_SPECIALIZED_MDNODE_LEAF ||                                \
       defined HANDLE_SPECIALIZED_MDNODE_BRANCH)
 #error "Missing macro definition of HANDLE_METADATA*"
 #endif
 
 // Handler for all types of metadata.
 #ifndef HANDLE_METADATA
 #define HANDLE_METADATA(CLASS)
 #endif
 
 // Handler for leaf nodes in the class hierarchy.
 #ifndef HANDLE_METADATA_LEAF
 #define HANDLE_METADATA_LEAF(CLASS) HANDLE_METADATA(CLASS)
 #endif
 
 // Handler for non-leaf nodes in the class hierarchy.
 #ifndef HANDLE_METADATA_BRANCH
 #define HANDLE_METADATA_BRANCH(CLASS) HANDLE_METADATA(CLASS)
 #endif
 
-// Handler for specialized and uniquable leaf nodes under MDNode.  Defers to
+// Handler for specialized and uniquable leaf nodes under MDNode. Defers to
 // HANDLE_MDNODE_LEAF_UNIQUABLE if it's defined, otherwise to
 // HANDLE_SPECIALIZED_MDNODE_LEAF.
 #ifndef HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE
 #ifdef HANDLE_MDNODE_LEAF_UNIQUABLE
 #define HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(CLASS)                        \
   HANDLE_MDNODE_LEAF_UNIQUABLE(CLASS)
 #else
 #define HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(CLASS)                        \
   HANDLE_SPECIALIZED_MDNODE_LEAF(CLASS)
 #endif
 #endif
 
-// Handler for leaf nodes under MDNode.
+// Handler for specialized and always-uniqued leaf nodes under MDNode. Defers to
+// HANDLE_MDNODE_LEAF_UNIQUED if it's defined, otherwise to
+// HANDLE_SPECIALIZED_MDNODE_LEAF.
+#ifndef HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUED
+#ifdef HANDLE_MDNODE_LEAF_UNIQUED
+#define HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUED(CLASS)                        \
+  HANDLE_MDNODE_LEAF_UNIQUED(CLASS)
+#else
+#define HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUED(CLASS)                        \
+  HANDLE_SPECIALIZED_MDNODE_LEAF(CLASS)
+#endif
+#endif
+
+// Handler for specialized and always-distinct leaf nodes under MDNode. Defers
+// to HANDLE_MDNODE_LEAF_DISTINCT if it's defined, otherwise to
+// HANDLE_SPECIALIZED_MDNODE_LEAF.
+#ifndef HANDLE_SPECIALIZED_MDNODE_LEAF_DISTINCT
+#ifdef HANDLE_MDNODE_LEAF_DISTINCT
+#define HANDLE_SPECIALIZED_MDNODE_LEAF_DISTINCT(CLASS)                        \
+  HANDLE_MDNODE_LEAF_DISTINCT(CLASS)
+#else
+#define HANDLE_SPECIALIZED_MDNODE_LEAF_DISTINCT(CLASS)                        \
+  HANDLE_SPECIALIZED_MDNODE_LEAF(CLASS)
+#endif
+#endif
+
+// Handler for uniquable leaf nodes under MDNode.
 #ifndef HANDLE_MDNODE_LEAF_UNIQUABLE
 #define HANDLE_MDNODE_LEAF_UNIQUABLE(CLASS) HANDLE_MDNODE_LEAF(CLASS)
 #endif
 
+// Handler for uniqued leaf nodes under MDNode.
+#ifndef HANDLE_MDNODE_LEAF_UNIQUED
+#define HANDLE_MDNODE_LEAF_UNIQUED(CLASS) HANDLE_MDNODE_LEAF(CLASS)
+#endif
+
+// Handler for distinct leaf nodes under MDNode.
+#ifndef HANDLE_MDNODE_LEAF_DISTINCT
+#define HANDLE_MDNODE_LEAF_DISTINCT(CLASS) HANDLE_MDNODE_LEAF(CLASS)
+#endif
+
 // Handler for leaf nodes under MDNode.
 #ifndef HANDLE_MDNODE_LEAF
 #define HANDLE_MDNODE_LEAF(CLASS) HANDLE_METADATA_LEAF(CLASS)
 #endif
 
 // Handler for non-leaf nodes under MDNode.
 #ifndef HANDLE_MDNODE_BRANCH
 #define HANDLE_MDNODE_BRANCH(CLASS) HANDLE_METADATA_BRANCH(CLASS)

 HANDLE_METADATA_LEAF(MDString)
 HANDLE_METADATA_BRANCH(ValueAsMetadata)
 HANDLE_METADATA_LEAF(ConstantAsMetadata)
 HANDLE_METADATA_LEAF(LocalAsMetadata)
 HANDLE_METADATA_LEAF(DistinctMDOperandPlaceholder)
 HANDLE_MDNODE_BRANCH(MDNode)
 HANDLE_MDNODE_LEAF_UNIQUABLE(MDTuple)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DILocation)
-HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIExpression)
+HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUED(DIExpression)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIGlobalVariableExpression)
 HANDLE_SPECIALIZED_MDNODE_BRANCH(DINode)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(GenericDINode)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DISubrange)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIEnumerator)
 HANDLE_SPECIALIZED_MDNODE_BRANCH(DIScope)
 HANDLE_SPECIALIZED_MDNODE_BRANCH(DIType)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIBasicType)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIDerivedType)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DICompositeType)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DISubroutineType)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIFile)
-HANDLE_SPECIALIZED_MDNODE_LEAF(DICompileUnit)
+HANDLE_SPECIALIZED_MDNODE_LEAF_DISTINCT(DICompileUnit)
 HANDLE_SPECIALIZED_MDNODE_BRANCH(DILocalScope)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DISubprogram)
 HANDLE_SPECIALIZED_MDNODE_BRANCH(DILexicalBlockBase)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DILexicalBlock)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DILexicalBlockFile)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DINamespace)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIModule)
 HANDLE_SPECIALIZED_MDNODE_BRANCH(DITemplateParameter)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DITemplateTypeParameter)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DITemplateValueParameter)
 HANDLE_SPECIALIZED_MDNODE_BRANCH(DIVariable)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIGlobalVariable)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DILocalVariable)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DILabel)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIObjCProperty)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIImportedEntity)
 HANDLE_SPECIALIZED_MDNODE_BRANCH(DIMacroNode)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIMacro)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIMacroFile)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DICommonBlock)
-HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIArgList)
+HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUED(DIArgList)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIStringType)
 HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIGenericSubrange)
 
 #undef HANDLE_METADATA
 #undef HANDLE_METADATA_LEAF
 #undef HANDLE_METADATA_BRANCH
 #undef HANDLE_MDNODE_LEAF
 #undef HANDLE_MDNODE_LEAF_UNIQUABLE
+#undef HANDLE_MDNODE_LEAF_UNIQUED
+#undef HANDLE_MDNODE_LEAF_DISTINCT
 #undef HANDLE_MDNODE_BRANCH
 #undef HANDLE_SPECIALIZED_MDNODE_LEAF
 #undef HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE
+#undef HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUED
+#undef HANDLE_SPECIALIZED_MDNODE_LEAF_DISTINCT
 #undef HANDLE_SPECIALIZED_MDNODE_BRANCH

llvm/lib/AsmParser/LLParser.cpp

bool LLParser::parseNamedMetadata() {
   Lex.Lex();
 
   if (parseToken(lltok::equal, "expected '=' here") ||
       parseToken(lltok::exclaim, "Expected '!' here") ||
       parseToken(lltok::lbrace, "Expected '{' here"))
     return true;
 
   NamedMDNode *NMD = M->getOrInsertNamedMetadata(Name);
-  if (Lex.getKind() != lltok::rbrace)
-    do {
-      MDNode *N = nullptr;
-      // parse DIExpressions inline as a special case. They are still MDNodes,
-      // so they can still appear in named metadata. Remove this logic if they
-      // become plain Metadata.
-      if (Lex.getKind() == lltok::MetadataVar &&
-          Lex.getStrVal() == "DIExpression") {
-        if (parseDIExpression(N, /*IsDistinct=*/false))
-          return true;
-        // DIArgLists should only appear inline in a function, as they may
-        // contain LocalAsMetadata arguments which require a function context.
-      } else if (Lex.getKind() == lltok::MetadataVar &&
-                 Lex.getStrVal() == "DIArgList") {
-        return tokError("found DIArgList outside of function");
-      } else if (parseToken(lltok::exclaim, "Expected '!' here") ||
-                 parseMDNodeID(N)) {
-        return true;
-      }
-      NMD->addOperand(N);
-    } while (EatIfPresent(lltok::comma));
+
+  if (Lex.getKind() == lltok::rbrace) {
+    Lex.Lex();
+    return false;
+  }
+
+    // Parse uniqued MDNodes inline as a special case.
+#define HANDLE_MDNODE_LEAF_UNIQUED(CLASS)                                      \
+  if (Lex.getKind() == lltok::MetadataVar && Lex.getStrVal() == #CLASS) {      \
+    if (parse##CLASS(N, /*IsDistinct=*/false))                                 \
+      return true;                                                             \
+    NMD->addOperand(N);                                                        \
+    continue;                                                                  \
+  }
+#include "llvm/IR/Metadata.def"
+    // Parse all other MDNodes as an MDNodeID.
+    if (parseToken(lltok::exclaim, "Expected '!' here") || parseMDNodeID(N)) {
 
   return parseToken(lltok::rbrace, "expected end of metadata node");
 }
 
 /// parseStandaloneMetadata:
 ///   !42 = !{...}
 bool LLParser::parseStandaloneMetadata() {
   assert(Lex.getKind() == lltok::exclaim);
   Lex.Lex();
   unsigned MetadataID = 0;
 
   MDNode *Init;
   if (parseUInt32(MetadataID) || parseToken(lltok::equal, "expected '=' here"))
     return true;
 
   // Detect common error, from old metadata syntax.
   if (Lex.getKind() == lltok::Type)
     return tokError("unexpected type in metadata definition");
 
+  auto DistinctLoc = Lex.getLoc();
   bool IsDistinct = EatIfPresent(lltok::kw_distinct);
   if (Lex.getKind() == lltok::MetadataVar) {
-    if (parseSpecializedMDNode(Init, IsDistinct))
+    if (parseSpecializedMDNode(Init, IsDistinct, DistinctLoc))
       return true;
   } else if (parseToken(lltok::exclaim, "Expected '!' here") ||
              parseMDTuple(Init, IsDistinct))
     return true;
 
   // See if this was forward referenced, if so, handle it.
   auto FI = ForwardRefMDNodes.find(MetadataID);
   if (FI != ForwardRefMDNodes.end()) {
bool LLParser::parseMDField(StringRef Name, FieldTy &Result) {
   if (Result.Seen)
     return tokError("field '" + Name + "' cannot be specified more than once");
 
   LocTy Loc = Lex.getLoc();
   Lex.Lex();
   return parseMDField(Loc, Name, Result);
 }
 
-bool LLParser::parseSpecializedMDNode(MDNode *&N, bool IsDistinct) {
+bool LLParser::parseSpecializedMDNode(MDNode *&N, bool IsDistinct,
+                                      LocTy DistinctLoc) {
   assert(Lex.getKind() == lltok::MetadataVar && "Expected metadata type name");
 
-#define HANDLE_SPECIALIZED_MDNODE_LEAF(CLASS)                                  \
+#define HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(CLASS)                        \
   if (Lex.getStrVal() == #CLASS)                                               \
     return parse##CLASS(N, IsDistinct);
+#define HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUED(CLASS)                          \
+  if (Lex.getStrVal() == #CLASS) {                                             \
+    if (IsDistinct)                                                            \
+      return error(DistinctLoc, "'distinct' not allowed for !" #CLASS);        \
+    return parse##CLASS(N, IsDistinct);                                        \
+  }
+#define HANDLE_SPECIALIZED_MDNODE_LEAF_DISTINCT(CLASS)                         \
+  if (Lex.getStrVal() == #CLASS) {                                             \
+    if (!IsDistinct)                                                           \
+      return error(DistinctLoc, "missing 'distinct', required for !" #CLASS);  \
+    return parse##CLASS(N, IsDistinct);                                        \
+  }
 #include "llvm/IR/Metadata.def"
 
   return tokError("expected metadata type");
 }
 
 #define DECLARE_FIELD(NAME, TYPE, INIT) TYPE NAME INIT
 #define NOP_FIELD(NAME, TYPE, INIT)
 #define REQUIRE_FIELD(NAME, TYPE, INIT)                                        \

 /// parseDICompileUnit:
 ///   ::= !DICompileUnit(language: DW_LANG_C99, file: !0, producer: "clang",
 ///                      isOptimized: true, flags: "-O2", runtimeVersion: 1,
 ///                      splitDebugFilename: "abc.debug",
 ///                      emissionKind: FullDebug, enums: !1, retainedTypes: !2,
 ///                      globals: !4, imports: !5, macros: !6, dwoId: 0x0abcd,
 ///                      sysroot: "/", sdk: "MacOSX.sdk")
 bool LLParser::parseDICompileUnit(MDNode *&Result, bool IsDistinct) {
-  if (!IsDistinct)
-    return Lex.Error("missing 'distinct', required for !DICompileUnit");
-
 #define VISIT_MD_FIELDS(OPTIONAL, REQUIRED)                                    \
   REQUIRED(language, DwarfLangField, );                                        \
   REQUIRED(file, MDField, (/* AllowNull */ false));                            \
   OPTIONAL(producer, MDStringField, );                                         \
   OPTIONAL(isOptimized, MDBoolField, );                                        \
   OPTIONAL(flags, MDStringField, );                                            \
   OPTIONAL(runtimeVersion, MDUnsignedField, (0, UINT32_MAX));                  \
   OPTIONAL(splitDebugFilename, MDStringField, );                               \
bool LLParser::parseDIExpression(MDNode *&Result, bool IsDistinct) {
   if (parseToken(lltok::rparen, "expected ')' here"))
     return true;
 
   Result = GET_OR_DISTINCT(DIExpression, (Context, Elements));
   return false;
 }
 
 bool LLParser::parseDIArgList(MDNode *&Result, bool IsDistinct) {
-  return parseDIArgList(Result, IsDistinct, nullptr);
+  return tokError("!DIArgList cannot appear outside of a function");
 }
 /// ParseDIArgList:
 ///   ::= !DIArgList(i32 7, i64 %0)
 bool LLParser::parseDIArgList(MDNode *&Result, bool IsDistinct,
                               PerFunctionState *PFS) {
   assert(PFS && "Expected valid function state");
   assert(Lex.getKind() == lltok::MetadataVar && "Expected metadata type name");
   Lex.Lex();

llvm/lib/Bitcode/Reader/MetadataLoader.cpp

for (auto &BB : F)
               SmallVector<uint64_t, 8> Ops;
               Ops.append(std::next(DIExpr->elements_begin()),
                          DIExpr->elements_end());
               DDI->setExpression(DIExpression::get(Context, Ops));
             }
   }
 
   /// Upgrade the expression from previous versions.
-  Error upgradeDIExpression(uint64_t FromVersion,
+  Error upgradeDIExpression(uint64_t FromVersion, bool &IsDistinct,
                             MutableArrayRef<uint64_t> &Expr,
                             SmallVectorImpl<uint64_t> &Buffer) {
     auto N = Expr.size();
     switch (FromVersion) {
     default:
       return error("Invalid record");
     case 0:
       if (N >= 3 && Expr[N - 3] == dwarf::DW_OP_bit_piece)
case 2: {
 
         // Continue with remaining elements.
         SubExpr = SubExpr.slice(HistoricSize);
       }
       Expr = MutableArrayRef<uint64_t>(Buffer);
       LLVM_FALLTHROUGH;
     }
     case 3:
+      IsDistinct = false;
+      LLVM_FALLTHROUGH;
+    case 4:
       // Up-to-date!
       break;
     }
 
     return Error::success();
   }
 
   void upgradeDebugInfo() {
void MetadataLoader::MetadataLoaderImpl::resolveForwardRefsAndPlaceholders(
   // with the final node they refer to.
   Placeholders.flush(MetadataList);
 }
 
 Error MetadataLoader::MetadataLoaderImpl::parseOneMetadata(
     SmallVectorImpl<uint64_t> &Record, unsigned Code,
     PlaceholderQueue &Placeholders, StringRef Blob, unsigned &NextMetadataNo) {
 
   bool IsDistinct = false;
   auto getMD = [&](unsigned ID) -> Metadata * {
     if (ID < MDStringRef.size())
       return lazyLoadOneMDString(ID);
     if (!IsDistinct) {
       if (auto *MD = MetadataList.lookup(ID))
         return MD;
       // If lazy-loading is enabled, we try recursively to load the operand
       // instead of creating a temporary.
MetadataList.assignValue(
         NextMetadataNo);
     NextMetadataNo++;
     break;
   }
   case bitc::METADATA_EXPRESSION: {
     if (Record.size() < 1)
       return error("Invalid record");
 
     IsDistinct = Record[0] & 1;
     uint64_t Version = Record[0] >> 1;
     auto Elts = MutableArrayRef<uint64_t>(Record).slice(1);
 
     SmallVector<uint64_t, 6> Buffer;
-    if (Error Err = upgradeDIExpression(Version, Elts, Buffer))
+    if (Error Err = upgradeDIExpression(Version, IsDistinct, Elts, Buffer))
       return Err;
 
+    if (IsDistinct)
+      return error("Invalid record");
+
     MetadataList.assignValue(
         GET_OR_DISTINCT(DIExpression, (Context, Elts)), NextMetadataNo);
     NextMetadataNo++;
     break;
   }
   case bitc::METADATA_GLOBAL_VAR_EXPR: {
     if (Record.size() != 3)
       return error("Invalid record");

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

void ModuleBitcodeWriter::writeDILabel(
   Stream.EmitRecord(bitc::METADATA_LABEL, Record, Abbrev);
   Record.clear();
 }
 
 void ModuleBitcodeWriter::writeDIExpression(const DIExpression *N,
                                             SmallVectorImpl<uint64_t> &Record,
                                             unsigned Abbrev) {
   Record.reserve(N->getElements().size() + 1);
-  const uint64_t Version = 3 << 1;
+  const uint64_t Version = 4 << 1;
   Record.push_back((uint64_t)N->isDistinct() | Version);
   Record.append(N->elements_begin(), N->elements_end());
 
   Stream.EmitRecord(bitc::METADATA_EXPRESSION, Record, Abbrev);
   Record.clear();
 }
 
 void ModuleBitcodeWriter::writeDIGlobalVariableExpression(

llvm/lib/CodeGen/MIRParser/MIParser.cpp

 }
 
 bool MIParser::parseStandaloneMDNode(MDNode *&Node) {
   lex();
   if (Token.is(MIToken::exclaim)) {
     if (parseMDNode(Node))
       return true;
   } else if (Token.is(MIToken::md_diexpr)) {
+    // FIXME: This should be driven off of the UNIQUED property in Metadata.def
     if (parseDIExpression(Node))
       return true;
   } else if (Token.is(MIToken::md_dilocation)) {
     if (parseDILocation(Node))
       return true;
   } else
     return error("expected a metadata node");
   if (Token.isNot(MIToken::Eof))

 }
 
 bool MIParser::parseMetadataOperand(MachineOperand &Dest) {
   MDNode *Node = nullptr;
   if (Token.is(MIToken::exclaim)) {
     if (parseMDNode(Node))
       return true;
   } else if (Token.is(MIToken::md_diexpr)) {
+    // FIXME: This should be driven off of the UNIQUED property in Metadata.def
     if (parseDIExpression(Node))
       return true;
   }
   Dest = MachineOperand::CreateMetadata(Node);
   return false;
 }
 
 bool MIParser::parseCFIOffset(int &Offset) {

llvm/lib/IR/AsmWriter.cpp

void SlotTracker::CreateFunctionSlot(const Value *V) {
   ST_DEBUG("  Inserting value [" << V->getType() << "] = " << V << " slot=" <<
            DestSlot << " [o]\n");
 }
 
 /// CreateModuleSlot - Insert the specified MDNode* into the slot table.
 void SlotTracker::CreateMetadataSlot(const MDNode *N) {
   assert(N && "Can't insert a null Value into SlotTracker!");
 
-  // Don't make slots for DIExpressions or DIArgLists. We just print them inline
-  // everywhere.
-  if (isa<DIExpression>(N) || isa<DIArgList>(N))
+  // Don't make slots for uniqued nodes. We just print them inline everywhere.
+#define HANDLE_MDNODE_LEAF_UNIQUED(CLASS)                                      \
+  if (isa<CLASS>(N))                                                           \
     return;
+#include "llvm/IR/Metadata.def"
 
   unsigned DestSlot = mdnNext;
   if (!mdnMap.insert(std::make_pair(N, DestSlot)).second)
     return;
   ++mdnNext;
 
   // Recursively add any MDNodes referenced by operands.
   for (unsigned i = 0, e = N->getNumOperands(); i != e; ++i)
if (N->isValid()) {
     for (const auto &I : N->getElements())
       Out << FS << I;
   }
   Out << ")";
 }
 
 static void writeDIArgList(raw_ostream &Out, const DIArgList *N,
                            TypePrinting *TypePrinter, SlotTracker *Machine,
-                           const Module *Context, bool FromValue = false) {
-  assert(FromValue &&
-         "Unexpected DIArgList metadata outside of value argument");
+                           const Module *Context) {
   Out << "!DIArgList(";
   FieldSeparator FS;
   MDFieldPrinter Printer(Out, TypePrinter, Machine, Context);
   for (Metadata *Arg : N->getArgs()) {
     Out << FS;
     WriteAsOperandInternal(Out, Arg, TypePrinter, Machine, Context, true);
   }
   Out << ")";
static void WriteAsOperandInternal(raw_ostream &Out, const Value *V,
   else
     Out << "<badref>";
 }
 
 static void WriteAsOperandInternal(raw_ostream &Out, const Metadata *MD,
                                    TypePrinting *TypePrinter,
                                    SlotTracker *Machine, const Module *Context,
                                    bool FromValue) {
-  // Write DIExpressions and DIArgLists inline when used as a value. Improves
-  // readability of debug info intrinsics.
-  if (const DIExpression *Expr = dyn_cast<DIExpression>(MD)) {
-    writeDIExpression(Out, Expr, TypePrinter, Machine, Context);
-    return;
-  }
-  if (const DIArgList *ArgList = dyn_cast<DIArgList>(MD)) {
-    writeDIArgList(Out, ArgList, TypePrinter, Machine, Context, FromValue);
-    return;
+  assert((FromValue || !(isa<LocalAsMetadata>(MD) || isa<DIArgList>(MD))) &&
+         "Unexpected function-local metadata outside of value argument");
+
+  // Write uniqued MDNodes inline when used as a value.
+#define HANDLE_MDNODE_LEAF_UNIQUED(CLASS)                                      \
+  if (const CLASS *N = dyn_cast<CLASS>(MD)) {                                  \
+    write##CLASS(Out, N, TypePrinter, Machine, Context);                       \
+    return;                                                                    \
   }
+#include "llvm/IR/Metadata.def"
 
   if (const MDNode *N = dyn_cast<MDNode>(MD)) {
     std::unique_ptr<SlotTracker> MachineStorage;
     if (!Machine) {
       MachineStorage = std::make_unique<SlotTracker>(Context);
       Machine = MachineStorage.get();
     }
     int Slot = Machine->getMetadataSlot(N);
if (const MDString *MDS = dyn_cast<MDString>(MD)) {
     Out << "!\"";
     printEscapedString(MDS->getString(), Out);
     Out << '"';
     return;
   }
 
   auto *V = cast<ValueAsMetadata>(MD);
   assert(TypePrinter && "TypePrinter required for metadata values");
-  assert((FromValue || !isa<LocalAsMetadata>(V)) &&
-         "Unexpected function-local metadata outside of value argument");
-
   TypePrinter->print(V->getValue()->getType(), Out);
   Out << ' ';
   WriteAsOperandInternal(Out, V->getValue(), TypePrinter, Machine, Context);
 }
 
 namespace {
 
 class AssemblyWriter {

 void AssemblyWriter::printNamedMDNode(const NamedMDNode *NMD) {
   Out << '!';
   printMetadataIdentifier(NMD->getName(), Out);
   Out << " = !{";
   for (unsigned i = 0, e = NMD->getNumOperands(); i != e; ++i) {
     if (i)
       Out << ", ";
 
-    // Write DIExpressions inline.
+    // Write UNIQUED nodes inline.
     // FIXME: Ban DIExpressions in NamedMDNodes, they will serve no purpose.
     MDNode *Op = NMD->getOperand(i);
     assert(!isa<DIArgList>(Op) &&
            "DIArgLists should not appear in NamedMDNodes");
-    if (auto *Expr = dyn_cast<DIExpression>(Op)) {
-      writeDIExpression(Out, Expr, nullptr, nullptr, nullptr);
-      continue;
-    }
+#define HANDLE_MDNODE_LEAF_UNIQUED(CLASS)                                      \
+  if (auto *N = dyn_cast<CLASS>(Op)) {                                         \
+    write##CLASS(Out, N, nullptr, nullptr, nullptr);                           \
+    continue;                                                                  \
+#include "llvm/IR/Metadata.def"
 
     int Slot = Machine.getMetadataSlot(Op);
     if (Slot == -1)
       Out << "<badref>";
     else
       Out << '!' << Slot;
   }
   Out << "}\n";

 
 static void printMetadataImpl(raw_ostream &ROS, const Metadata &MD,
                               ModuleSlotTracker &MST, const Module *M,
                               bool OnlyAsOperand) {
   formatted_raw_ostream OS(ROS);
 
   TypePrinting TypePrinter(M);
 
-  WriteAsOperandInternal(OS, &MD, &TypePrinter, MST.getMachine(), M,
-                         /* FromValue */ true);
+  WriteAsOperandInternal(OS, &MD, &TypePrinter, MST.getMachine(), M);
 
   auto *N = dyn_cast<MDNode>(&MD);
-  if (OnlyAsOperand || !N || isa<DIExpression>(MD) || isa<DIArgList>(MD))
+  if (OnlyAsOperand || !N)
+    return;
+  // Uniqued MDNodes are always treated as if OnlyAsOperand, as they are
+  // printed inline.
+#define HANDLE_MDNODE_LEAF_UNIQUED(CLASS)                                      \
+  if (isa<CLASS>(MD))                                                          \
     return;
+#include "llvm/IR/Metadata.def"
 
   OS << " = ";
   WriteMDNodeBodyInternal(OS, N, &TypePrinter, MST.getMachine(), M);
 }
 
 void Metadata::printAsOperand(raw_ostream &OS, const Module *M) const {
   ModuleSlotTracker MST(M, isa<MDNode>(this));
   printMetadataImpl(OS, *this, MST, M, /* OnlyAsOperand */ true);

llvm/lib/IR/DebugInfoMetadata.cpp

DEFINE_GETIMPL_LOOKUP(DILabel,
                         (Scope, Name, File, Line));
   Metadata *Ops[] = {Scope, Name, File};
   DEFINE_GETIMPL_STORE(DILabel, (Line), Ops);
 }
 
 DIExpression *DIExpression::getImpl(LLVMContext &Context,
                                     ArrayRef<uint64_t> Elements,
                                     StorageType Storage, bool ShouldCreate) {
+  assert(Storage != Distinct && "DIExpression cannot be distinct");
   DEFINE_GETIMPL_LOOKUP(DIExpression, (Elements));
   DEFINE_GETIMPL_STORE_NO_OPS(DIExpression, (Elements));
 }
 
 unsigned DIExpression::ExprOperand::getSize() const {
   uint64_t Op = getOp();
 
   if (Op >= dwarf::DW_OP_breg0 && Op <= dwarf::DW_OP_breg31)
DEFINE_GETIMPL_LOOKUP(DIMacroFile,
                         (MIType, Line, File, Elements));
   Metadata *Ops[] = { File, Elements };
   DEFINE_GETIMPL_STORE(DIMacroFile, (MIType, Line), Ops);
 }
 
 DIArgList *DIArgList::getImpl(LLVMContext &Context,
                               ArrayRef<ValueAsMetadata *> Args,
                               StorageType Storage, bool ShouldCreate) {
+  assert(Storage != Distinct && "DIArgList cannot be distinct");
   DEFINE_GETIMPL_LOOKUP(DIArgList, (Args));
   DEFINE_GETIMPL_STORE_NO_OPS(DIArgList, (Args));
 }
 
 void DIArgList::handleChangedOperand(void *Ref, Metadata *New) {
   ValueAsMetadata **OldVMPtr = static_cast<ValueAsMetadata **>(Ref);
   assert((!New || isa<ValueAsMetadata>(New)) &&
          "DIArgList must be passed a ValueAsMetadata");
   untrack();
   ValueAsMetadata *NewVM = cast_or_null<ValueAsMetadata>(New);
   for (ValueAsMetadata *&VM : Args) {
     if (&VM == OldVMPtr) {
       if (NewVM)
         VM = NewVM;
       else
         VM = ValueAsMetadata::get(UndefValue::get(VM->getValue()->getType()));
     }
   }
   track();
 }
 void DIArgList::track() {
   for (ValueAsMetadata *&VAM : Args)
     if (VAM)
       MetadataTracking::track(&VAM, *VAM, *this);
 }
 void DIArgList::untrack() {
   for (ValueAsMetadata *&VAM : Args)
     if (VAM)
       MetadataTracking::untrack(&VAM, *VAM);
 }
 void DIArgList::dropAllReferences() {
   untrack();
   Args.clear();
   MDNode::dropAllReferences();
 }

llvm/lib/IR/LLVMContextImpl.h

public:
   StringMap<MDString, BumpPtrAllocator> MDStringCache;
   DenseMap<Value *, ValueAsMetadata *> ValuesAsMetadata;
   DenseMap<Metadata *, MetadataAsValue *> MetadataAsValues;
 
   DenseMap<const Value*, ValueName*> ValueNames;
 
 #define HANDLE_MDNODE_LEAF_UNIQUABLE(CLASS)                                    \
   DenseSet<CLASS *, CLASS##Info> CLASS##s;
+#define HANDLE_MDNODE_LEAF_UNIQUED(CLASS) HANDLE_MDNODE_LEAF_UNIQUABLE(CLASS)
 #include "llvm/IR/Metadata.def"
 
   // Optional map for looking up composite types by identifier.
   Optional<DenseMap<const MDString *, DICompositeType *>> DITypeMap;
 
   // MDNodes may be uniqued or not uniqued.  When they're not uniqued, they
   // aren't in the MDNodeSet, but they're still shared between objects, so no
   // one object can destroy them.  Keep track of them here so we can delete

llvm/lib/IR/LLVMContextImpl.cpp

#endif
 
   // Drop references for MDNodes.  Do this before Values get deleted to avoid
   // unnecessary RAUW when nodes are still unresolved.
   for (auto *I : DistinctMDNodes)
     I->dropAllReferences();
 #define HANDLE_MDNODE_LEAF_UNIQUABLE(CLASS)                                    \
   for (auto *I : CLASS##s)                                                     \
     I->dropAllReferences();
+#define HANDLE_MDNODE_LEAF_UNIQUED(CLASS) HANDLE_MDNODE_LEAF_UNIQUABLE(CLASS)
 #include "llvm/IR/Metadata.def"
 
   // Also drop references that come from the Value bridges.
   for (auto &Pair : ValuesAsMetadata)
     Pair.second->dropUsers();
   for (auto &Pair : MetadataAsValues)
     Pair.second->dropUse();
 
   // Destroy MDNodes.
   for (MDNode *I : DistinctMDNodes)
     I->deleteAsSubclass();
 #define HANDLE_MDNODE_LEAF_UNIQUABLE(CLASS)                                    \
   for (CLASS * I : CLASS##s)                                                   \
     delete I;
+#define HANDLE_MDNODE_LEAF_UNIQUED(CLASS) HANDLE_MDNODE_LEAF_UNIQUABLE(CLASS)
 #include "llvm/IR/Metadata.def"
 
   // Free the constants.
   for (auto *I : ExprConstants)
     I->dropAllReferences();
   for (auto *I : ArrayConstants)
     I->dropAllReferences();
   for (auto *I : StructConstants)

llvm/lib/IR/Metadata.cpp

MDNode *MDNode::replaceWithPermanentImpl() {
   switch (getMetadataID()) {
   default:
     // If this type isn't uniquable, replace with a distinct node.
     return replaceWithDistinctImpl();
 
 #define HANDLE_MDNODE_LEAF_UNIQUABLE(CLASS)                                    \
   case CLASS##Kind:                                                            \
     break;
+#define HANDLE_MDNODE_LEAF_UNIQUED(CLASS) HANDLE_MDNODE_LEAF_UNIQUABLE(CLASS)
 #include "llvm/IR/Metadata.def"
   }
 
   // Even if this type is uniquable, self-references have to be distinct.
   if (hasSelfReference(this))
     return replaceWithDistinctImpl();
   return replaceWithUniquedImpl();
 }

 #define HANDLE_MDNODE_LEAF_UNIQUABLE(CLASS)                                    \
   case CLASS##Kind: {                                                          \
     CLASS *SubclassThis = cast<CLASS>(this);                                   \
     std::integral_constant<bool, HasCachedHash<CLASS>::value>                  \
         ShouldRecalculateHash;                                                 \
     dispatchRecalculateHash(SubclassThis, ShouldRecalculateHash);              \
     return uniquifyImpl(SubclassThis, getContext().pImpl->CLASS##s);           \
   }
+#define HANDLE_MDNODE_LEAF_UNIQUED(CLASS) HANDLE_MDNODE_LEAF_UNIQUABLE(CLASS)
 #include "llvm/IR/Metadata.def"
   }
 }
 
 void MDNode::eraseFromStore() {
   switch (getMetadataID()) {
   default:
     llvm_unreachable("Invalid or non-uniquable subclass of MDNode");
 #define HANDLE_MDNODE_LEAF_UNIQUABLE(CLASS)                                    \
   case CLASS##Kind:                                                            \
     getContext().pImpl->CLASS##s.erase(cast<CLASS>(this));                     \
     break;
+#define HANDLE_MDNODE_LEAF_UNIQUED(CLASS) HANDLE_MDNODE_LEAF_UNIQUABLE(CLASS)
 #include "llvm/IR/Metadata.def"
   }
 }
 
 MDTuple *MDTuple::getImpl(LLVMContext &Context, ArrayRef<Metadata *> MDs,
                           StorageType Storage, bool ShouldCreate) {
   unsigned Hash = 0;
   if (Storage == Uniqued) {

llvm/test/Assembler/invalid-diarglist-outside-function.ll

+; RUN: not llvm-as < %s -disable-output 2>&1 | FileCheck %s
+
+; CHECK: <stdin>:[[@LINE+1]]:6: error: !DIArgList cannot appear outside of a function
+!0 = !DIArgList()

llvm/test/Assembler/invalid-diexpression-distinct.ll

+; RUN: not llvm-as < %s -disable-output 2>&1 | FileCheck %s
+
+; CHECK: <stdin>:[[@LINE+1]]:6: error: 'distinct' not allowed for !DIExpression
+!0 = distinct !DIExpression()

llvm/test/Bitcode/DIExpression-is-distinct-upgrade.ll

+; RUN: llvm-dis -o - %s.bc | FileCheck %s
+
+!llvm.dbg.cu = !{!1}
+!llvm.module.flags = !{!8, !9}
+
+!0 = distinct !DIGlobalVariable(name: "g", scope: !1, file: !2, line: 1, type: !5, isLocal: false, isDefinition: true)
+!1 = distinct !DICompileUnit(language: DW_LANG_C99, file: !2, producer: "clang", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !3, globals: !4)
+!2 = !DIFile(filename: "a.c", directory: "/")
+!3 = !{}
+!4 = !{!7}
+!5 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+; CHECK: expr: !DIExpression()
+!6 = distinct !DIExpression()
+!7 = !DIGlobalVariableExpression(var: !0, expr: !6)
+!8 = !{i32 2, !"Dwarf Version", i32 4}
+!9 = !{i32 2, !"Debug Info Version", i32 3}