Page MenuHomePhabricator

[DebugInfo] Support for DW_OP_implicit_pointer (DW_OP_LLVM_argN)
Needs ReviewPublic

Authored by alok on Nov 24 2019, 9:41 AM.

Details

Summary

This patch (1/N) stems from D69787
It is suggested by @aprantl

Summary:

It Introduces a new Dwarf operator DW_OP_LLVM_argN (N=0-7).
Which represents Nth argument of containing intrinsic.

Example:

For example in intrinsic
  "call @llvm.dbg.someintrinsic(DILocalVariable("x"), DILocalVariable("y"),
         metadata !DIExpression(DW_OP_LLVM_arg0, DW_OP_LLVM_arg1, DW_OP_plus))"
  'DW_OP_LLVM_arg0' represents 'DILocalVariable("x")' and
  'DW_OP_LLVM_arg1' represents 'DILocalVariable("y")'.

Diff Detail

Event Timeline

alok created this revision.Nov 24 2019, 9:41 AM
TWeaver marked an inline comment as done.Nov 25 2019, 1:09 AM
TWeaver added a subscriber: TWeaver.

Hiya,

thanks for the patch!

just a small nit from me.

Cheers.

llvm/docs/LangRef.rst
4828

'argumnet' should be 'argument'

alok marked an inline comment as done.Nov 25 2019, 2:15 AM
alok added inline comments.
llvm/docs/LangRef.rst
4828

Thanks for pointing this out. It will be corrected in next version.

alok updated this revision to Diff 230859.Nov 25 2019, 2:16 AM

Updated to incorporated review comment.

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.

llvm/include/llvm/BinaryFormat/Dwarf.def
663 ↗(On Diff #230859)

Since these extensions are only used inside LLVM IR, they shouldn't leak into dwarfdump & friends. Can you take a look at how DW_OP_LLVM_fragment is defined and copy that? I think it is hardcoded in Dwarf.cpp

alok marked 2 inline comments as done.Dec 3 2019, 3:18 AM

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.

Thanks for your comment. I shall look into it.

llvm/include/llvm/BinaryFormat/Dwarf.def
663 ↗(On Diff #230859)

Thanks for your comment. It will be incorporated in next version.

alok updated this revision to Diff 231859.Dec 3 2019, 3:21 AM
alok marked an inline comment as done.

This is updated to incorporate comments from @aprantl.

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

alok added a comment.Dec 4 2019, 8:46 AM

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 ?

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.

llvm/docs/LangRef.rst
4833

I just realized that DIGLobalVariableExpression also takes a DIExpression. I think we should also allow DW_OP_LLVM_arg0 there.

alok added a comment.Dec 5 2019, 8:24 PM

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.

Thanks a lot for the clarification. Next version of patch will include this.

alok updated this revision to Diff 236016.Fri, Jan 3, 3:37 AM

This is re-based and updated to apply LLVM_arg0 to existing DWARF operator and to upgrade for older styled DWARF expressions. This is done as suggested by @aprantl .

alok updated this revision to Diff 237238.Thu, Jan 9, 8:27 PM

Re-based and some cleanup.