[PATCH] D83560: [DebugInfo] Added support for DW_OP_implicit_value in llvm

Sat Jul 11 10:33:05 PDT 2020

SouraVX marked an inline comment as done.
SouraVX added a comment.

In D83560#2145768 <https://reviews.llvm.org/D83560#2145768>, @dblaikie wrote:

>

Thanks @dblaikie for your feedback!

> Not sure it's worth committing such a narrow implementation - might be worth a bit of generalization even in this first patch?

This operation(as you might have noticed)  has limited usage, since `DW_OP_stack_value` and friends fits in other cases very well. This is specifically made to cater needs(such as in this case) where the variable size is bigger than (size of address). This will cater all the needs in math(HPC) based applications where usage of `long double` is ubiquitous.

> looks to be very specific to long double right now (but without any assertions, API features, or comments to enforce that restriction) - and in the DwarfDebug.cpp caller, which could presumably be used for all constant values, maybe (not sure if that would be a good thing or not - haven't looked at the alternatives, etc))

Yes, that's very specific and you'll notice that it has very strict enforcement requirements `isConstrantFP`  followed by `if (AP.getDwarfVersion() >= 4 && RawBytes.getBitWidth() == 80)`(making sure we don't mess up existing infra).
For the documentation/comments part, admittedly I didn't put any behavior/requirement specifics in the declaration, However the definition part is fully documented and self-explanatory(IMO). Should we put some brief comments there(declaration part) too ?

> @aprantl @probinson will probably want to weigh in on getting support from their debuggers before this is committed, or having this under a flag, etc.

As of now `GDB ` has fairly good support for this. `LLDB` doesn't have it, I'm not sure how much effort is needed to put this in ?

As a side note and as opportunistic usage of `DW_OP_implict_value` usage:
I noted following WRT `GCC` usage of this:
`GCC` uses `DW_OP_implicit_value`  for `long double` as well as `float` `double`(may be more, these I've verified). `clang` uses `DW_OP_constu` followed by `DW_OP_stack_value` for cases of `float` `double`(correctly) and `long double` (incorrectly as depicted).  I assume that since both `float` and `double` are within(64 bit) it's okay to represent that using `DW_OP_constu` and `DW_OP_stack_value`, but (as you may notice) Spec doesn't say anything whether it should also be used for floating point types
Spec: Pg. 27:

  DW_OP_constu
  The single operand of the DW_OP_constu operation provides an unsigned LEB128 integer constant.

One more(I would say advantage) for using `DW_OP_implicit_value` over some `DW_OP_stack_value` based expression for cases of `float` and `double` is that it consumes less space(to be exact 1 byte lesser in case of `float` and `double`). (GCC sort of uses an algorithm to choose between these(which representation will take less space))
Consider a simple case:

  float f=3.14f;
  CLANG based - DW_OP_constu + DW_OP_stack_value expression = 7 bytes
  GCC based- DW_OP_implicit_value = 6 bytes
  double d = 3.14;
  CLANG based - DW_OP_constu + DW_OP_stack_value expression = 11 bytes
  GCC based- DW_OP_implicit_value = 10 bytes

These findings also encourage the need for support for `DW_OP_implicit_value` from `LLDB` side, otherwise `LLDB` won't be able to show locations based on these expression.

================
Comment at: llvm/test/DebugInfo/X86/DW_OP_implicit_value.ll:13-18
+;;int main() {
+;;        long double ld = 3.14;
+;;        printf("dummy\n");
+;;        ld *= ld;
+;;        return 0;
+;;}
----------------
dblaikie wrote:
> I'd probably write this as:
> ```
> long double src();
> void f1() {
>   long double ld = 3.14;
>   ld = src();
> }
> ```
> That should be enough to use the implicit_value to represent the value of 'ld' during the call to src, and to use a location list to do it (since the assignment to 'ld' is shortening the lifetime -  using function calls at least I find are clearer opaque clobbers/sinks - rather than the complexity of printf, or wondering whether the use of multiplication is significant, whether this has to be main, has to have an integer return value for some reason, etc.)
Typically the workflow I follow while working on these test cases, is not to solely rely on DWARF but to have a executable as well ready to be loaded to debugger. Otherwise(you may notice, we could have never discovered this, there was an expression but through `GDB` I came to know that, it' incorrect.)
That's why I putted the test case as it is. If you've concerns WRT this I'm okay with that as well(however I didn't see benefit/drawback in both approaches)
:)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83560