Page MenuHomePhabricator

[DwarfExpression] Disallow some rewrites to avoid undefined behavior
ClosedPublic

Authored by bjope on Sep 6 2019, 4:46 AM.

Details

Summary

The value operand in DW_OP_plus_uconst/DW_OP_constu value can be
large (it uses uint64_t as representation internally in LLVM).
This means that in the uint64_t to int conversions, previously done
by DwarfExpression::addMachineRegExpression, could lose information.
Also, the negation done in "-Offset" was undefined behavior in case
Offset was exactly INT_MIN.

To avoid the above problems, we now avoid transformation like
[Reg, DW_OP_plus_uconst, Offset] --> [DW_OP_breg, Offset]
and
[Reg, DW_OP_constu, Offset, DW_OP_plus] --> [DW_OP_breg, Offset]
when Offset > INT_MAX.

And we avoid to transform
[Reg, DW_OP_constu, Offset, DW_OP_minus] --> [DW_OP_breg,-Offset]
when Offset > INT_MAX+1.

The patch also adjusts DwarfCompileUnit::constructVariableDIEImpl
to make sure that "DW_OP_constu, Offset, DW_OP_minus" is used
instead of "DW_OP_plus_uconst, Offset" when creating DIExpressions
with negative frame index offsets.

Notice that this might just be the tip of the iceberg. There
are lots of fishy handling related to these constants. I think both
DIExpression::appendOffset and DIExpression::extractIfOffset may
trigger undefined behavior for certain values.

Diff Detail

Repository
rL LLVM

Event Timeline

bjope created this revision.Sep 6 2019, 4:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 6 2019, 4:46 AM
bjope marked an inline comment as done.Sep 6 2019, 5:14 AM
bjope added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
650 ↗(On Diff #219062)

This fix was (the minimum) needed to make other lit tests pass. I.e not ending up with huge unsigned offsets in DW_OP_plus_uconst or DW_OP_plus resulting in changes in DWARF output.

So the "regressions" seen in the added dw_op_constu.mir test case are not supposed to happen in reality (as long as we use DW_OP_minus when having negative offsets).

JDevlieghere added inline comments.Sep 6 2019, 9:00 AM
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
284 ↗(On Diff #219062)

We should use C++ style casts and std::numeric_limits.

bjope updated this revision to Diff 219131.Sep 6 2019, 9:55 AM

Now using std::numeric_limits and C++ style casts (thanks JDevlieghere!).

Also updated checks in test/DebugInfo/NVPTX/dbg-declare-alloca.ll. By using DIExpression::appendOffset in DwarfCompileUnit::constructVariableDIEImpl we now get rid of an, afaict, useless "DW_OP_add_uconst 0".

bjope marked an inline comment as done.Sep 6 2019, 9:55 AM
JDevlieghere accepted this revision.Sep 6 2019, 10:06 AM

LGTM. Thanks!

This revision is now accepted and ready to land.Sep 6 2019, 10:06 AM
This revision was automatically updated to reflect the committed changes.
aprantl added inline comments.Sep 9 2019, 2:14 PM
llvm/trunk/test/DebugInfo/X86/dw_op_constu.mir
285

We should fix the comment printer to emit very large constants as hex...