Page MenuHomePhabricator

[DebugInfo] Emit DW_OP_implicit_value for Floating point constants
ClosedPublic

Authored by SouraVX on Jul 10 2020, 8:19 AM.

Details

Summary

llvm is missing support for DW_OP_implicit_value operation.
DW_OP_implicit_value op is indispensable for cases such as
optimized out long double variables.

For intro refer: DWARFv5 Spec Pg: 40 2.6.1.1.4 Implicit Location Descriptions

Consider the following example:

int main() {
        long double ld = 3.14;
        printf("dummy\n");
        ld *= ld;
        return 0;
}

when compiled with tunk clang as
clang test.c -g -O1 produces following location description
of variable ld:

DW_AT_location        (0x00000000:
                     [0x0000000000201691, 0x000000000020169b): DW_OP_constu 0xc8f5c28f5c28f800, DW_OP_stack_value, DW_OP_piece 0x8, DW_OP_constu 0x4000, DW_OP_stack_value, DW_OP_bit_piece 0x10 0x40, DW_OP_stack_value)
                  DW_AT_name    ("ld")

Here one may notice that this representation is incorrect(DWARF4
stack could only hold integers(and only up to the size of address)).
Here the variable size itself is 128 bit.
GDB and LLDB confirms this:

(gdb) p ld
$1 = <invalid float value>
(lldb) frame variable ld
(long double) ld = <extracting data from value failed>

GCC represents/uses DW_OP_implicit_value in these sort of situations.
Based on the discussion with Jakub Jelinek regarding GCC's motivation
for using this, I concluded that DW_OP_implicit_value is most appropriate
in this case.

Link: https://gcc.gnu.org/pipermail/gcc/2020-July/233057.html

GDB seems happy after this patch:(LLDB doesn't have support
for DW_OP_implicit_value)

(gdb) p ld
p ld
$1 = 3.14000000000000012434

Diff Detail

Event Timeline

SouraVX created this revision.Jul 10 2020, 8:19 AM
dstenb added a subscriber: dstenb.Jul 10 2020, 8:24 AM

Not sure it's worth committing such a narrow implementation - might be worth a bit of generalization even in this first patch? Or do you have specific/near-term plans to generalize this further (both the addImplicitValue itself, which 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))

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

llvm/test/DebugInfo/X86/DW_OP_implicit_value.ll
13–18 ↗(On Diff #277053)

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

SouraVX marked an inline comment as done.Jul 11 2020, 10:33 AM

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.

llvm/test/DebugInfo/X86/DW_OP_implicit_value.ll
13–18 ↗(On Diff #277053)

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

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.

Sure, I understand the need having specific name(and my mistake of choosing a generic name addImplicitValue for the purpose of this patch). I've noted down and work on your inputs/concerns and revise once @aprantl and @probinson also have a look on it. Thanks again for your inputs :)

Regarding that strange check(80 bit width), at that time I didn't find a way to distinguish b/w float and long double so I did it in a brute force fashion(IEEE representation to distinguish). I'll see if I can do it better.

@aprantl @probinson could you folks please provide a quick comment on this as a whole ?
Meanwhile, I also have a patch(locally) which switches to DW_OP_implicit_value for all the floating point constants float, double, long double working fantastic. For correctness checked that executables are debuggable with GDB.

aprantl added inline comments.Jul 21 2020, 4:23 PM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
2504–2518

This restriction seems overly strict and also partially wrong.

First off, the condition should be sunk into DwarfExpression. That's were we do the low-level DWARF expression encoding. Maybe we should add a DwarfExpression::addFPConstant(APFloat) function?

An then in the new function, since we cannot represent anything > 64 bit without DW_OP_implicit_value, we should error out if it's not supported, and not emit a broken constant like we did before.

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h
303

See my comment above. It might be better to have something that takes an APFloat?

SouraVX updated this revision to Diff 279793.EditedJul 22 2020, 6:16 AM

Addressed review comments by @aprantl, Thanks for this.
Changes:

  • Edited the summary to reflect the intention.
  • This patch switches to DW_OP_implicit_value for all the floating point constants float, double and long double, for demonstration purposes, considering 1 byte space saving(discussed above).
  • Changes are still a bit conservative, in a sense emission of DW_OP_implicit_value is guarded with additional check for debugger tunning.

This will ensure that, we didn't break any existing machinery/consumers.

SouraVX added inline comments.Jul 22 2020, 6:30 AM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
2504–2518

An then in the new function, since we cannot represent anything > 64 bit without DW_OP_implicit_value, we should error out if it's not supported, and not emit a broken constant like we did before.

I'll address this in next revision(This one doesn't through error).

aprantl added inline comments.Jul 22 2020, 8:54 AM
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
234

return;

236

Why would we special-case Float80 instead of just emitting everything that we didn't emit in the above block here unconditionally?

248

should we emit an invalid opcode / and assembler comment / ... to indicate that there was an error?

SouraVX marked 4 inline comments as done.Jul 22 2020, 10:29 AM

For cases other than those gracefully handled(`float, double, long double) for DW_OP_implicit_value I'm considering a hard error sort of assert/llvm_unreachable or 2nd option could be just bail out early.

And for other consumer targets, still using _stack_value based expression, I'm considering bailing out early for variables > 64 bits instead of incorrectly representing it.

@aprantl do you any suggestions/objection WRT this. Please share!

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
234

Ah, Sorry will take care of that.

236

Initially thought of implementing that way, but due padding issues this cannot be done.
For instance, for long double bit width will be 80 bits(10 bytes) and emit as

emitUnsigned(NumBytes);
for(...)
emitData();

This will ends up in expression of NumBytes(10) in this case, however DW_AT_byte_size for long double type is 16 bytes -- this conflict resulting in following error in GDB

(gdb) p ld
access outside bounds of object referenced via synthetic pointer.

For addressing padding issues and another distant issue(some other Floating point constant with different IEEE representation should not be emitted erronously.)

For cases other than those gracefully handled(`float, double, long double) for DW_OP_implicit_value I'm considering a hard error sort of assert/llvm_unreachable or 2nd option could be just bail out early.

We should only use llvm_unreachable when it's actually unreachable. If it is possible to create IR input that would cause us to go down this path, we should use some other error reporting mechanism instead. I think I'd be happy with emitting a warning to llvm::dbgs(), or an assembler comment (if that is available here, not sure). We could also emit something like DW_OP_nop or DW_OP_lit0 DW_OP_div, or DW_OP_user_hi. Or we could flag the DwarfExpression object as having an error and enforce the owner to check this before the expression is emitted, but I think that's too much to ask just for this patch :-)

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
236

Thanks for the explanation!

aprantl accepted this revision.Jul 22 2020, 10:51 AM
This revision is now accepted and ready to land.Jul 22 2020, 10:51 AM
SouraVX updated this revision to Diff 279911.Jul 22 2020, 12:16 PM
SouraVX marked 2 inline comments as done.

Addressed @aprantl review comments. Thanks for this!

SouraVX retitled this revision from [DebugInfo] Added support for DW_OP_implicit_value in llvm to [DebugInfo] Emit DW_OP_implicit_value for Floating point constants.Jul 22 2020, 12:18 PM

I think I'd be happy with emitting a warning to llvm::dbgs(), or an assembler comment (if that is available here, not sure).

Here, I've used LLVM_DEBUG so that it won't hurt any thing. This seems Okay to you ?

This revision was automatically updated to reflect the committed changes.
SouraVX added a comment.EditedJul 22 2020, 8:25 PM

Temporarily reverted in 9d2da6759b4d05d834371bcaaa8fc3d9b3385b18.
Due to failing/assertion in test case in Sparc Backend.
test/DebugInfo/Sparc/subreg.ll

SouraVX added a comment.EditedJul 23 2020, 1:32 AM

Temporarily reverted in 9d2da6759b4d05d834371bcaaa8fc3d9b3385b18.
Due to failing/assertion in test case in Sparc Backend.
test/DebugInfo/Sparc/subreg.ll

Fixed in 59a76d957a2603ee0548293ded170c98e1308c48

There was another failure in WebAssembly debug info test case caught by build-bot. That's also fixed with commit id mentioned.
Once the change stabilizes, I plan to do a quick refactoring(wherever applicable WRT this change).

bjope added a subscriber: bjope.Jul 24 2020, 9:08 AM
bjope added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
234

I'm working on a big-endian target (together with a debugger based on gdb 8.1.0).
I needed to reverse the order here (and in the loop at line 243), to get same result as before.

Does that make sense? Should we care about endianess in DwarfExpression.cpp?

bjope added inline comments.Jul 24 2020, 9:47 AM
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
234

One thing I noticed is that APFloat::bitcastToAPInt, according to comments in APFloat.cpp, is creating an APInt that is a bit map of the value as it would appear in memory. However, that would only be true if the APInt was stored as one value taking endianess into consideration. When looking at the rawBytes of the resulting APInt after bitcastToAPInt the bytes are in litte-endian order. So a byteSwap is generally needed for big-endian.

aprantl added inline comments.Jul 24 2020, 10:09 AM
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
234

I don't know what the answer to the question, but I would encourage patches that hide the endianness problems as much as possible. I.e., the DwarfExpression API should have a high-level addConstantFP(const APFloat &Value) call, and DwarfExpression should know about endianness and do the right thing automatically (whatever the right thing is). What would be even better would be to hide endianness in AsmPrinter (instead of emitData1() use a high-level emitEndianCorrectedDataN() call — if such a thing makes sense.

SouraVX added a subscriber: amyk.Aug 1 2020, 11:44 PM

It seems to me this is reverted again in 7c182663a857fc87552fa2861c7f94046d55845e . Reverting author @amyk didn't updated here.
I'll see if I can get around the endianness, if that's the only issue ?

MaskRay added a subscriber: MaskRay.Aug 2 2020, 9:15 AM
MaskRay added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
229

The bit ordering is different on SPARC V9, PowerPC, etc. Use write* from https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/Support/Endian.h#L415

238

This is x86_fp80 instead of IEEE 754 quadruple-precision floating-point format.

SouraVX added a comment.EditedAug 2 2020, 12:15 PM

It's not just endiannes that is causing trouble to PPC target. Looking at a comment regarding PPC target's long double representation in AsmPrinter.cpp

// PPC's long double has odd notions of endianness compared to how LLVM
// handles it: p[0] goes first for *big* endian on PPC.

This patch does not handle this, I'll see if we can fix this up without too many changes, otherwise I'll file a new review covering all scenarios endianess and this target specific.

EDIT:
@MaskRay, I didn't refreshed the page, saw you comments later. But Thanks :)
Will work on those.

SouraVX reopened this revision.Aug 2 2020, 12:30 PM
This revision is now accepted and ready to land.Aug 2 2020, 12:30 PM
SouraVX updated this revision to Diff 284322.Aug 10 2020, 5:21 AM

Thanks @MaskRay for pointers. Tried similar approach(slightly different, since expression has to be emitted by a separate streamer owned by DebugLocDwarfExpression) to take care of Endianess.

  • Temporarily removed long double support since IMHO this might require a deeper surgery. Patch reflects the overall skeleton required for suporting DW_OP_implicit_value.
  • @amyk, is it possible for you to try this patch on PPC targets ? So that we won't end up having surprises.
SouraVX marked 3 inline comments as done.Aug 10 2020, 7:02 AM
SouraVX added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
234

This revision take cares of endiannes.

amyk added a comment.Aug 11 2020, 8:49 AM

Thanks @MaskRay for pointers. Tried similar approach(slightly different, since expression has to be emitted by a separate streamer owned by DebugLocDwarfExpression) to take care of Endianess.

  • Temporarily removed long double support since IMHO this might require a deeper surgery. Patch reflects the overall skeleton required for suporting DW_OP_implicit_value.
  • @amyk, is it possible for you to try this patch on PPC targets ? So that we won't end up having surprises.

I have tried this patch on little endian and big endian PowerPC, everything looks OK and I do not see any failures with this patch.

Thank @amyk for confirming this. @aprantl and @MaskRay could you guys please share your thoughts on this.

Temporarily removed long double support since IMHO this might require a deeper surgery. Patch reflects the overall skeleton required for suporting DW_OP_implicit_value.

I made this remark, since existing approach leads to manual padding bytes calculation. Exploring a better way to accomplish this.
Some of these big-constants are handled in a more cleaner way in AsmPrinter.cpp static void emitGlobalConstantFP, but I doubt this sort of thing can be re-used. Since expression emission requires it's own streamer(ByteStreamer)and intricacies.

SouraVX added a comment.EditedAug 17 2020, 1:27 PM

@aprantl do you have any concerns WRT this.
Yes commit message and other should reflect the intent, apart from this.

aprantl accepted this revision.Aug 19 2020, 8:55 AM
This revision was landed with ongoing or failed builds.Aug 19 2020, 12:51 PM
This revision was automatically updated to reflect the committed changes.

Thanks @aprantl for taking out time for reviewing this. I've committed changes in ef8992b9f0189005e0d9e09bd0967301bd7a7cc6 with updated commit message reflecting the intent.
I'll be looking into handling long double in a cleaner way.
Thanks!