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

David Blaikie via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sat Jul 11 11:15:29 PDT 2020


dblaikie added a comment.

In D83560#2145933 <https://reviews.llvm.org/D83560#2145933>, @SouraVX wrote:

> 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).


The call site criticism I had was: "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))"

> 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 ?

The rest of the criticism " looks to be very specific to long double right now (but without any assertions, API features, or comments to enforce that restriction) " applied to the "addImplicitValue" function: Its name is very general, its signature is very general, but the implementation is very specific about a certain width field, etc. Which doesn't seem ideal/quite right. Comments inside the implementation without assertions - with a very general function name seems like it'd be pretty easy to misuse and get unexpected behavior. If the code is going to be so specific then the function name should probably describe that specific behavior and if possible assertions to ensure that's done - perhaps even have it take APFloat. (& I'm not sure whether it's worth choosing between implicit_value and uconst - and just always do implicit_value)

>> @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 ?

Neither am I - and, yeah, if we /just/ did it for 128 bit types, which you've demonstrated are already unusable by lldb - then there would be no harm in making this change sooner. But if we're going to generalize it (& I think the code merits generalization if it weren't for any other constraints) then we'd need to consider how it'd break existing consumers, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83560





More information about the llvm-commits mailing list