[PATCH] D66746: [LiveDebugValues] Omit entry values for DBG_VALUEs with pre-existing expressions

[David Stenberg via Phabricator via llvm-commits]

dstenb added a comment.

In D66746#1646665 <https://reviews.llvm.org/D66746#1646665>, @djtodoro wrote:

> > I have interpreted the size as meaning the byte size of the DWARF block that the operation will cover. Assuming that, at the time of running LiveDebugValues I don't think there is a good way to query the size of the block that the entry value will cover; we don't know that until we actually emit the DWARF, as far as I can tell. That is why I have assumed that a hard coded operand of 1 is emitted there, with the assumption that only simple register location descriptions are supported.
> > 
> > However, I now got uncertain when looking at prependOpcodes() which is used to add the operation to the DIExpression:
> > 
> > Ops.push_back(dwarf::DW_OP_entry_value);
> > // Add size info needed for entry value expression.
> > // Add plus one for target register operand.
> > Ops.push_back(Expr->getNumElements() + 1);
>
>
>
> > As seen, there the number of pre-existing elements plus one is used. I don't think the number of elements does not map one-to-one with the byte size of the DWARF block, so I'm not sure how to interpret that. Can you help me understand what the operand in the DIExpression world indicates, @djtodoro?
>
> I think your point is right. We wanted to have there hard-coded value `1` for the size of following expression. Except if we did not cover all the cases where we should avoid complex expressions, we always generate an entry value expression with an empty pre-existing `DIExpression`, so we assumed that this code will cover current situation and may be extended to support complex debug expressions as well. But, I also think it is hard to distinguish the size of a complex `DIExpression` until DWARF being printed, so maybe we can change the code to `Ops.push_back(1);` and put some kind of assertion there.
>
> > As far as I understand, we now emit the operation from the DIExpression as-is in the DWARF. That means that if we have a register that turns into a complex expression we will still say that the size of that expression is one byte. I have seen such cases with our downstream target, but I'll see if I can trigger that behavior with an upstream target with a source level reproducer.
>
> If you can produce such scenario it will be desirable. We enabled the feature in this initial stage only for `x86` targets, and tried to cover all the situations found for the target. We meant to cover all the places where we should avoid generation of debug entry values with complex expressions (now). Eventually, we should handle all types of expressions.


It can be reproduced with the following C file compiled for sparc64:

  volatile long double global;
  extern void clobber();
  int foo(long double p) {
    global = p;
    clobber();
    return 123;
  }

compiled using:

  clang --target=sparc64 -g -O2 -Xclang -femit-debug-entry-values -c -integrated-as sparc.c

yields the following entry value after LiveDebugValues:

  CALL @clobber, csr, implicit $o6, implicit-def $o6, debug-location !24 {
    STDFri killed renamable $i0, 8, renamable $d1, implicit killed $q0, debug-location !19 :: (store 8)
  }
  DBG_VALUE $q0, $noreg, !17, !DIExpression(DW_OP_entry_value, 1), debug-location !18

which currently results in the following bad location list entry:

  [0x0000000000000020,  0x0000000000000028): DW_OP_GNU_entry_value(DW_OP_regx D0), DW_OP_piece 0x8, DW_OP_regx D1, DW_OP_piece 0x8, DW_OP_stack_value)

Please note that you have to apply D66888 <https://reviews.llvm.org/D66888>, and also add sparc to the list of allowed targets in `ParseCodeGenArgs()`, to get that reproducer working.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66746/new/

https://reviews.llvm.org/D66746