Page MenuHomePhabricator

[DebugInfo] Normalize common kinds of DWARF sub-expressions.
ClosedPublic

Authored by JDevlieghere on Sep 4 2018, 9:05 AM.

Details

Summary

Normalize common kinds of DWARF sub-expressions to make debug info encoding a bit more compact:

  1. DW_OP_constu [X < 32] -> DW_OP_litX
  2. DW_OP_constu [all ones] -> DW_OP_lit0, DW_OP_not

Addresses Adrian's comments in https://reviews.llvm.org/D48676.

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.Sep 4 2018, 9:05 AM

Can you add a testcase for the 0xffffffff case?

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
186 ↗(On Diff #163841)
  1. This is short one f.
  2. I'd use some form of uint_max or ~0ULL so this isn't an issue ;-)
aprantl requested changes to this revision.Sep 4 2018, 9:40 AM
This revision now requires changes to proceed.Sep 4 2018, 9:40 AM
  • Normalize more usages.
  • Compare against 32-bit unsigned max.

I generated some unrelated files while fixing the tests that were included in the previous diff.

aprantl added inline comments.Sep 4 2018, 11:10 AM
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
31 ↗(On Diff #163866)

These are 64 bit values. We either need ~0ULL or std::limits::max<uint64_t>().

probinson added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
31 ↗(On Diff #163866)

The DWARF expression stack uses target-address-size values, and that isn't necessarily 64-bit. I'm not sure we can make this compaction work in all cases.

aprantl added inline comments.Sep 4 2018, 1:29 PM
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
31 ↗(On Diff #163866)

But if we do implement the 64-bit case, it will be correct there, we just don't get the more compact representation on a 32-bit arch, so that would still be win an many targets without breaking anything?

probinson added inline comments.Sep 4 2018, 1:48 PM
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
31 ↗(On Diff #163866)

Acknowledging we're not compacting -1 on 32-bit targets, then okay.

LGTM, with outstanding changes addressed & perhaps add a note about how this doesn't trigger for 32-bit targets. Be sure to watch the bots afterwards, since we didn't emit DW_OP_LIT before AFAIK and LLDB may have bugs.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 5 2018, 3:19 AM
This revision was automatically updated to reflect the committed changes.