Would generating the testcase on-the-fly inside the RUN: line or perhaps via python be an option? Otherwise I guess this is fine, too.

As documented in comment 8 of the PR, the range that a constant-value DBG_VALUE defines a variable location over does not currently terminate at the end of the basic block the DBG_VALUE is in. Instead, its range is valid across all basic blocks (as finally laid out) until another DBG_VALUE for the variable is seen. This often covers many instructions where the variable didn't actually have a location, and even down code paths where the variable was never assigned the constant value.

IIUC this upgrade is sweeping the variant of DISubprograms that have SPflags and do not yet have this patch under the rug? The way the Swift compiler branches work out I can live with that but other downstream users may have a problem with that as it could break LTO builds that include object files versions of clang between 347239 and this patch.

We don't typically require autoupgrade functionality in the IR parser, as there are no compatibility guarantees for textual LLVM IR. Implementing bitcode support is all that is required. Sometimes it can be convenient to support older formats in LLParser to avoid updating hundreds of tests, but it's definitely not required.

Renamed enumerators.

I was following http://www.llvm.org/docs/HowToSetUpLLVMStyleRTTI.html I will rename it to eKind...

Excellent point!

Yes. Can you please add upgrade functionality into MetadataLoader and add a bitcode upgrade test into test/Bitcode?

One one hand this seems fine to remove, but on the other hand — won't these functions come in handy to compare and debug differences between the LLDB and LLVM DWARF parsers, while we are switching over?

In D59272#1426904, @rnk wrote:

This makes me nervous. The pair of the DIVariable and DILocation is used to uniquely identify the variable that a dbg.value applies to. In practice, inlining is usually what creates multiple instances of the same variable that get updated by dbg.values. You can see how the location is used in DwarfDebug::collectVariableInfoFromMFTable.

If you update the patch with the documentation, I'd be happy to commit it for you!

LGTM, however it would be important to also document the new flags in docs/SourceLevelDebugging.rst or LangRef.rst.

In D59235#1426716, @JDevlieghere wrote:

Agreed, and we've been doing this for new patches for a while now. However, I very strongly prefer having asserts over "returning a default value", which only hides real bugs.

LGTM, perhaps let's give this a day for others to chime in in case they missed the thread on lldb-dev.

In D54327#1422996, @schweitz wrote:

In D54327#1310565, @probinson wrote:

I am curious how debug info for common blocks plays with LTO. I think we would not want different common-block descriptions to be de-duplicated just because the global name was the same.

Our existing compilers do not make use of LLVM's LTO. Is there a suggestion on how to correctly deal with LTO's de-duplication?

Interesting find. Thanks!

This looks mostly good now.

How can this change be NFC?

Whenever I see a GetFooAtIndex() method I expect this to be exposed via the SBAPI, but I assume this isn't the case here?

So just for the record, I'm fine with making this kind of change in case this got lost in the noise :-)

@JDevlieghere since this is in general code: Could this affect dsymutil at all?

@JDevlieghere since this is in general code: Could this affect dsymutil at all?

Is there a way to force clang to generate this and/or are existing tests failing because this support is missing?

The code looks fine, but this needs a testcase.

Is there a way to make the DWARF unit non-optional in this function?

Looks mostly good!

Ping. As far as I can tell, now this patch should be entirely NFC.