Page MenuHomePhabricator

[DebugInfo] Refactor code for emitting DWARF expressions for FP constants

Authored by labath on Nov 9 2020, 2:25 AM.



This patch moves the selection of the style used to emit the numbers
(DW_OP_implicit_value vs. DW_OP_const+DW_OP_stack_value) into
DwarfExpression::addUnsignedConstant. This logic is not FP-specific, and
it will be needed for large integers too.

Split off from D90916.

Diff Detail

Event Timeline

labath created this revision.Nov 9 2020, 2:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 9 2020, 2:25 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
labath requested review of this revision.Nov 9 2020, 2:25 AM
dblaikie accepted this revision.Nov 17 2020, 6:02 PM

Looks good


This looks like a change in behavior - is it? Should it be tested?


This looks like a change in behavior (now other <= 8 floating point sizes can be emitted - is it just that other such sizes are impossible/never come up? In which case maybe an assertion would be suitable?) & might possibly benefit from testing.

This revision is now accepted and ready to land.Nov 17 2020, 6:02 PM
labath updated this revision to Diff 306376.Nov 19 2020, 5:04 AM

Add a _Float16 test

labath added inline comments.Nov 19 2020, 5:04 AM

It shouldn't normally be a change because this adjustment in DW_OP_stack_value emission is matched by a change in the callers (previously it was emitted through the DwarfExpr.addExpression(std::move(ExprCursor)) call, now that call is skipped).

It could theoretically make a difference if we had a floating point constant accompanied by a non-empty dwarf expression -- previously that expression would be emitted, but now it's skipped. However, such a combination wouldn't be very useful (the expression would operate on the constant as an integer), and judging by the lack of failing tests, we don't actually make use of it. (Though my knowledge of this part of the code is fairly limited).


Actually, it was already possible to emit the DW_OP_stack_value flavour of _Float16 (for example). This added the ability to handle the DW_OP_implicit_value form as well, because the new code layout made it harder to split the two cases. I didn't want to add a test without checking that this actually works (though I had no reason to suspect it wouldn't), but I have now verified that both gdb and lldb handle _Float16 (on arm), so I am adding a test for it.

labath updated this revision to Diff 306377.Nov 19 2020, 5:11 AM
  • reduce the test
Harbormaster completed remote builds in B79445: Diff 306376.
dblaikie accepted this revision.Nov 20 2020, 8:02 PM
dblaikie added inline comments.

Cool, thanks!

This revision was landed with ongoing or failed builds.Nov 23 2020, 12:59 AM
This revision was automatically updated to reflect the committed changes.
dyung added a subscriber: dyung.Nov 23 2020, 6:53 PM

Hi Pavel, your change caused an assertion failure to be hit in our internal test suite. I have filed a repro with the assertion failure and a callstack in PR48277. Can you please take a look?

I also ran into this issue (and added a second repro to the bug report); I'd suggest a revert, but I think @labath may get to it fairly soon, so holding off of reverting until the end of the day.

I reverted this because it caused issues with complex floats. See PR48277 and D92013 for details.