Page MenuHomePhabricator

[Debuginfo] (1/7) [DW_OP_implicit_pointer/second strategy] Support for DW_OP_LLVM_implicit_pointer
ClosedPublic

Authored by alok on Jul 18 2020, 4:54 PM.

Details

Summary
This patch stems from D84112.

New dwarf operator DW_OP_LLVM_implicit_pointer is introduced (present only in LLVM IR)
This is needed because representation and specification (types of operands) of
it a bit different than actual dwarf expression DW_OP_implicit_pointer.
It will later be converted to DW_OP_implicit_pointer while creating actual DWARF info

Diff Detail

Event Timeline

alok created this revision.Jul 18 2020, 4:54 PM
jmorse added a subscriber: jmorse.Jul 30 2020, 5:38 AM

Hi @alok, would you be able to re-title the reviews in this stack to indicate what's in the patch itself? That'll make it easier to navigate.

llvm/include/llvm/BinaryFormat/Dwarf.h
125

Any particular reason for 0x100c instead of the sequentially next 0x1004?

llvm/test/DebugInfo/X86/LLVM_implicit_pointer.ll
3

Would you be able to add a file-comment along the lines of "round-trip test for implicit pointer metadata"? That'll make it easier for future readers to know what's being tested.

alok marked 2 inline comments as done.Aug 3 2020, 4:05 AM

Thanks for your reviewing this.

llvm/include/llvm/BinaryFormat/Dwarf.h
125

Thanks for pointing this out. It shall be corrected in next version of patch.

llvm/test/DebugInfo/X86/LLVM_implicit_pointer.ll
3

It shall be included in next version of patch.

alok added a reviewer: jmorse.Aug 3 2020, 4:06 AM
alok updated this revision to Diff 282621.Aug 3 2020, 7:37 AM
alok marked 2 inline comments as done.
alok retitled this revision from [Debuginfo] (1/N) Support for DW_OP_implicit_pointer for named and unnamed variables (second strategy). to [Debuginfo] (1/8) [DW_OP_implicit_pointer/second strategy] Support for DW_OP_LLVM_implicit_pointer.
alok edited the summary of this revision. (Show Details)
alok added a comment.Aug 12 2020, 7:15 AM

@jmorse Do you have any more comment on this ?

No real additional comments on this or the 2nd patch -- for further progress though I think you need to stimulate some discussion in the llvm-dev "DW_OP_implicit_pointer design/implementation in general" thread, to convince David / Adrian that this patch series addresses their concerns from the earlier discussion.

aprantl added inline comments.Aug 12 2020, 4:34 PM
llvm/docs/LangRef.rst
5187

there's an article missing .. the variable?

5187

I'm also missing a Verifier check that enforces this constraint?

5188

instruction's

5189

I think we could expand this a bit.

  • is it illegal in a DIGlobalVariableExpression?

Maybe include an example use?

dblaikie added inline comments.Sep 23 2020, 7:39 PM
llvm/lib/IR/DebugInfoMetadata.cpp
1025

Probably need to have test coverage for these invalid cases

(& I'm not quite clear on the reasoning for where this can appear (why only at the beginning? I would've naively guessed maybe only at the end? (top of the stack) but haven't thought it through in any great detail, for sure - making sure it generalizes over multiple lost indirection would be great (I guess given the DWARF format, it can't generalize over parts of an object lacking indirection, but maybe it'd be nice if the IR format supported it (eg: "struct x { int y; int *z; }" if 'z' no longer has pointable storage - can we still describe what it points to, while also describing the value of 'y'?))

alok marked an inline comment as done.Oct 12 2020, 2:32 AM
alok added inline comments.
llvm/lib/IR/DebugInfoMetadata.cpp
1025

Thanks for your comment. I shall add the invalid test cases.

Transformation of these operators are
DW_OP_LLVM_explicit_pointer (Generated by optimization phases), which is later converted to DW_OP_LLVM_implicit_pointer and which is finally converted to DW_OP_implicit_pointer.
DW_OP_LLVM_explicit_pointer is not bound by condition of appearing only at start. these are later converted to implicit_pointer versions to conform to DWARF standard.
I shall look into the use-case explained by you.

alok updated this revision to Diff 298549.Oct 15 2020, 11:29 PM
alok marked an inline comment as done.

Updated to re-base and incorporate comment from @dblaikie .

aprantl added inline comments.Oct 27 2020, 7:00 PM
llvm/docs/LangRef.rst
5189

I think this could really benefit from an example. Also, it is odd to refer to a DILocalVariable in the documentation of a DIExpression operand, since DIExpressions can be pointed to by dbg.values (which may bind them to one or more DILocalVariables) but they can also appear in other places. I understand that the DWARF definition of DW_OP_implicit_pointer makes some of this strangeness necessary, but we should do at least our best in documenting how this is supposed to be used.

alok marked an inline comment as done.Oct 30 2020, 11:27 AM
alok added inline comments.
llvm/docs/LangRef.rst
5189

Thanks for your comment. I shall work on this.

alok updated this revision to Diff 304746.Nov 12 2020, 1:38 AM
alok retitled this revision from [Debuginfo] (1/8) [DW_OP_implicit_pointer/second strategy] Support for DW_OP_LLVM_implicit_pointer to [Debuginfo] (1/7) [DW_OP_implicit_pointer/second strategy] Support for DW_OP_LLVM_implicit_pointer.
alok added a reviewer: aprantl.
alok marked an inline comment as done.

Updated comments from @aprantl and @dblaikie (on other review).

alok added a comment.Nov 23 2020, 12:14 AM

Updated comments from @aprantl and @dblaikie (on other review).

Ping !

Seems generally good to me - but @aprantl and/or @JDevlieghere is probably the keeper of DWARF expression tracking stuff, so they ought to take a look/sign off.

Out of interest, can this be composed? eg:

struct x { int i; int *j; };
void f1(int p) {
  x y  = {5, &p};
  f(); // opaque function call - breaking here it'd be good to still be able to describe y.j as pointing to the value stored in 'p', even though that value shouldn't be in memory anywhere (shuold just be in a register)
  return *y.j;
}
jmorse added a comment.Dec 3 2020, 8:23 AM

FTR, this LGTM

llvm/docs/LangRef.rst
5187
alok updated this revision to Diff 309729.Dec 5 2020, 3:51 AM

Updated for comment from @jmorse

alok edited the summary of this revision. (Show Details)Dec 5 2020, 3:53 AM
aprantl accepted this revision.Dec 14 2020, 3:31 PM

This is needed because representation and specification (types of operands) of

it a bit different than actual dwarf expression DW_OP_implicit_pointer.

Can you add this to the documention to explain what/how this is different from the DWARF op?

Otherwise this LGTM

This revision is now accepted and ready to land.Dec 14 2020, 3:31 PM
This revision was automatically updated to reflect the committed changes.