In D70642#1769887, @aprantl wrote:

In D70642#1769175, @alok wrote:

In D70642#1767321, @aprantl wrote:

Thanks. This part looks good, but the bitcode upgrade is still missing.

Sorry as I could not include that in first set of modifications. I was just thinking about which operations can be upgraded. One candidate for this was new expression DW_OP_LLVM_implicit_pointer which comes later that the current patch so escapes the need for upgrade. Should I look for existing operations for potential candidates for upgrade. or should i re-arrange the DW_OP_LLVM_implicit_pointer patch such as first it should not use DW_OP_LLVM_arg0 and then after current patch it needs upgrade. Could you please clear my doubt here ?

The upgrade I was thinking about here is reading old bitcode that contains

llvm.dbg.value(%foo, DILocalVariable(...), DIExpression(DW_OP_deref))

and upgrades it to

llvm.dbg.value(%foo, DILocalVariable(...), DIExpression(DW_OP_arg0, DW_OP_deref))

However, for this to work, we also need to implement support for DW_OP_argN to AsmPrinter to interpret the new operands and DIBuilder.

My assumption is that after this change DIExpression() is illegal and should be replaced by DIExpression(DW_OP_LLVM_arg0).
This transformation will cause a lot of churn in the Clang and LLVM testsuite (most of which you will be able to fix with a a sed script), but it will allow to resolve the ambiguity between:

!DIGlobalVariableExpression(!DIGlobalVariable("a"), !DIExpression())
!DIGlobalVariableExpression(!DIGlobalVariable("a"), !DIExpression(DW_OP_constu 42, DW_OP_stack_value))
!DIGlobalVariableExpression(!DIGlobalVariable("a"), !DIExpression(DW_OP_constu 42, DW_OP_plus, DW_OP_stack_value))

in the new scheme, these expressions would be

!DIExpression(DW_OP_LLVM_arg0)
!DIExpression(DW_OP_constu 42, DW_OP_stack_value) # constant, no arg!
!DIExpression(DW_OP_LLVM_arg0, DW_OP_constu 42, DW_OP_plus, DW_OP_stack_value))

respectively. We currently disambiguate between these cases by context: if the DIGlobalVariableExpression is a !dbg attachment of an llvm::Global, the global is the implicit arg0, otherwise there is no implict argument.

Since this is going to be a lot of work, we could also opt for a smaller, incremental change first that introduces only DW_OP_LLVM_arg1..DW_OP_LLVM_arg7 and keep the DW_OP_LLVM_arg0 implicit. Then we don't need to worry about bitcode upgrades now.

In D70642#1767321, @aprantl wrote:

Thanks. This part looks good, but the bitcode upgrade is still missing.

Updated to rename proposed LLVM Dwarf extension to DW_OP_LLVM_explicit_pointer.

Update includes comments from @jmorse.

This is updated to incorporate comments from @aprantl.

In D70642#1765515, @aprantl wrote:

Thanks! This will need some bitcode-compatibility planning as well:

We'll need to bump the version number, and then in MetadataLoader.cpp / upradeDIExpression() add an a DW_OP_LLVM_arg0 at the beginning for all older versions.
This should be tested with a .bc file that contains an old-form DIExpression, run through llvm-dis | llvm-as | llvm-dis and checked that the expression is upgraded correctly.

This is updated to incorporate review comments from @Orlando .

In D69999#1758776, @Orlando wrote:

Hi @alok,

Adrian said:

Thanks! Let's cycle back to this after the llvm-dev thread that @dblaikie started has been resolved.

Reading the thread I'm not sure if it counts as having been 'resolved', so hopefully these comments are not out of place.
To help keep things running smoothly please make sure:
a) variable names are capitalised, and
b) comments are written as sentences (capitals, punctuation),

Updated to incorporated review comment.

Updated to rebase due to change in parent patch D70260.

Updated to incorporate comments from @Orlando .

In D70260#1758219, @Orlando wrote:

Hi @alok. I haven't been following the DW_OP_implicit_pointer discussion too closely, so just some inline nitpick comments/questions from me.

Thanks,
Orlando

Updated to rebase due to changes in trunk and parent patches.

Updated to rebase due to changes in trunk and parent patches.

Updated to rebase due to changes in trunk and parent patches.

Updated to re-base due to changes in trunk and parent patches.

updated to re-base this due to changes in trunk and parent patches.

Updated after splitting out versions for DW_OP_LLVM_implicit_pointer, DW_OP_LLVM_argN and llvm.dbg.derefval intrinsic.

Updated to

• Rebase
• To remove trailing space
• typecasted return value of getData to uint8_t

-Updated to use "git show" in place of "git diff" used earlier

In D69999#1743443, @alok wrote:

Review comments from @aprantl have been incorporated.

Review comments from @aprantl have been incorporated.

In D69999#1741360, @aprantl wrote:

My first thought was that this might be an opportunity to finally implement explicit dbg.value arguments. (There is prior discussion of this online somewhere but I couldn't find it in my mailbox).

The idea behind a explicit dbg.value arguments is to move away from the implicit first argument of dbg.value to a format where that makes them explicit and thus more flexible.

For example, currently we would write:

call @llvm.dbg.value(%1, !DILocalVariable(name: "x"), !DIExpression(DW_OP_uconst, 4, DW_OP_plus)

which (assuming %1 compiles to reg0) may generate something like

DW_TAG_variable
  DW_AT_name("x")
  DW_AT_location(DW_OP_reg0, DW_OP_uconst, 4, DW_OP_plus)

Note the implicit first element in the DIExpression.

An explicit notation would be something like:

call @llvm.dbg.value(%1, !DILocalVariable(name: "x"), !DIExpression(DW_OP_LLVM_arg0, DW_OP_uconst, 4, DW_OP_plus)

which has the advantage that we could put DW_OP_LLVM_arg0 into places other than just the beginning of the expression.

Similarly, we could then further extend it to allow referring to more than one LLVM SSA value within an expression, using a variadic dbg.value. For example:

call @llvm.dbg.value(%1, %2, !DILocalVariable(name: "x"), !DIExpression(DW_OP_LLVM_arg0 /* %1 */ , DW_OP_uconst, DW_OP_LLVM_arg1 /* %2 */, DW_OP_plus)

This mechanism seems to apply nicely to DW_OP_implicit_pointer — applied to your testcase we'd write:

call @llvm.dbg.value(!DILocalVariable(name: "arr"), !DILocalVariable(name: "ptr"), !DIExpression(DW_OP_LLVM_implicit_pointer, DW_OP_LLVM_arg0, 4)

I think this would make the handling of DW_OP_LLVM_implicit_pointer less magic in LLVM IR and would open the door to future features, such as the variadic dbg.value.
What do you think?

This patch is updated post split. part of it went to D69999, which is also the dependency for current patch. (for current patch to work, D69999 is needed)

Updated for comments from @aprantl (Adrian Prantl)

In D69999#1739404, @aprantl wrote:

• Can you update LangRef.rst / SourceLevelDebugging.rst to document the semantics of the DW_OP_implicit_pointer in LLVM IR. Specifically what the two arguments mean while in LLVM IR?

In D69886#1738047, @aprantl wrote:

Could you please split off a patch just adds the IR, Bitcode and verifier support from this review into a separate one? That one should be very straightforward to review & land and it will make the rest easier.

In D69886#1736182, @vsk wrote:

Hey @alok, thanks for working on this.

In D69886#1736833, @dstenb wrote:

Thanks for adding the source reproducers; however, they are typically not stored as separate files (with one exception being tests operating on binary files). Feel free to inline the C code as comments in the IR files though!

This version is updated for comment of David Stenberg (@dstenb)
(removed original reproducer c programs and added those in test cases itself as comments)

Updated for comments from @djtodoro (Djordje Todorovic) to apply tricks to shorten the tests

In D69886#1736660, @djtodoro wrote:

@alok Thanks for doing this. I just put advices on how to make your tests shorter, so please address them in the other tests as well.

This update includes incorporation of review comments from Vedant Kumar (@vsk)

In D69886#1736266, @dblaikie wrote:

Apologies for not having enough time to review this in more detail at the moment, so this feedback is shorting from the hip slightly based on reading the existing conversations and patch description

Broadly:

I'd expect more than 3 patches and in this order:

1. Backend/DWARF emission support for new debug info IR/metadata/intrinsic usage/DW_OP operators. This should include tests with hand-crafted IR (or generated from future patches - but I think this should be narrow enough that a small example could be given mostly with IR generated from clang/LLVM, and only a minor edit to use the new feature)

This is to incorporate review comments.

• To use llc in place of clang in tests
• To delete unnecessary attributes from tests

In D69886#1735632, @jmorse wrote:

Hi Alok,

Thanks for splitting up the patch and tests; I'm not very familiar with implicit pointers and I'm still working it through my mind, so I have a couple of questions if you'll bear with me. At the moment though, it looks like implicit-pointer dbg.value insts refer directly to DILocalVariables -- this will become a problem if the same function is inlined twice, as there's nothing to distinguish which inlining instance the dbg.value refers to. You can probably make a metadata pair of {DILocalVariable, DILocation} where the DILocation is the inlining location, and use that as the operand of the dbg.value instead. There should also be a test for multiple inlining instances too.

Thanks a lot for taking your time to review this, let me try to answer this, actually the functionality of DW_OP_implicit_pointer is to preserve the dereferenced values of optimized out variables, it doesnt preserve the location information. Our interest is to emit first operand which is offset of target variable within debuginfo dwarf section. We preserve DILocalVariable (temporarily) until debug info section is formed and variable's offset within it is known. Please let me know in case I misunderstood the question.