Page MenuHomePhabricator

[DebugInfo] Make legal and emit DW_OP_swap and DW_OP_xderef
ClosedPublic

Authored by kzhuravl on Feb 7 2017, 12:20 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

kzhuravl created this revision.Feb 7 2017, 12:20 PM
aprantl edited edge metadata.Feb 7 2017, 12:37 PM

Thanks!

  • please include testcases
  • there probably needs to be a Verifier change as well

As you probably noticed the other binary operators (like DW_OP_plus) are currently accepting their RHS argument as an "argument" rather than using the top-of-stack (-1). I consider this a bug, so I'm fine with these new operators behaving the way they are (using the stack) in this patch.

frej added a subscriber: frej.Feb 13 2017, 4:24 AM
kzhuravl updated this revision to Diff 88669.Feb 16 2017, 12:49 AM

Add additional test

Thanks!

  • please include testcases

Done.

  • there probably needs to be a Verifier change as well

Isn't Verifier calling DIExpression::isValid()?

As you probably noticed the other binary operators (like DW_OP_plus) are currently accepting their RHS argument as an "argument" rather than using the top-of-stack (-1). I consider this a bug, so I'm fine with these new operators behaving the way they are (using the stack) in this patch.

Do you know if there is bugzilla for it or if someone is working on fixing this bug?

Thanks.

I'd like to request one more feature:

isValid() should reject

!DIExpression(DW_OP_swap)

because there is only one (implicit) element on the stack.

kzhuravl updated this revision to Diff 88975.Feb 17 2017, 3:35 PM

Address review feedback

aprantl accepted this revision.Feb 20 2017, 10:48 AM
aprantl added inline comments.
lib/IR/DebugInfoMetadata.cpp
622 ↗(On Diff #88975)

I was just going to comment that a better way to implement this would be to add a local variable that keeps track of the stack depth. But I realize that we can't actually do that without introducing something like a DW_LLVM_OP_implicit_location as a placeholder for the location this DIExpression is attached to, or else pass the number of implicit stack elements into isValid (isValid(unsigned StackDepth).

For example:

!DIExpression(DW_OP_constu 1, DW_OP_swap, DW_OP_xderef)

is fine when attached to a llvm.dbg.value (with one implicit stack element).
But it would be wrong if attached to a !DIGlobalVariableExpression that is describing a constant (with no implicit stack elements).

This is not necessarily something we need to fix in this patch. It might be nice to add a FIXME about the limitation here.

This revision is now accepted and ready to land.Feb 20 2017, 10:48 AM
This revision was automatically updated to reflect the committed changes.
kzhuravl marked an inline comment as done.