Page MenuHomePhabricator

Fix an offset underflow bug in DwarfExpression when describing small values with subregisters
ClosedPublic

Authored by aprantl on Nov 20 2019, 1:14 PM.

Details

Summary

DwarfExpression::addMachineReg() knows how to build a larger register that isn't expressible in DWARF by combining multiple subregisters. However, if the entire value fits into just one subregister, it would still emit the other subregisters, leading to all sorts of inconsistencies down the line.

This patch fixes that by moving an already existing(!) check whether the subregister's offset is before the end of the value to the right place.

rdar://problem/57294211

Diff Detail

Event Timeline

aprantl created this revision.Nov 20 2019, 1:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 20 2019, 1:14 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
aprantl edited the summary of this revision. (Show Details)Nov 20 2019, 1:15 PM
vsk added inline comments.Nov 20 2019, 1:39 PM
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
168

Is it a requirement that all bits in MachineReg be described? What happens is there's an unset bit in Coverage?

aprantl marked an inline comment as done.Nov 20 2019, 2:02 PM
aprantl added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
168

All "holes" are covered by empty "no DWARF register encoding" empty pieces (see line 164 and 176).

vsk accepted this revision.Nov 20 2019, 4:22 PM

Lgtm!

This revision is now accepted and ready to land.Nov 20 2019, 4:22 PM
This revision was automatically updated to reflect the committed changes.

The test dwarf-size-field-overflow.test now takes over 60 seconds to run, which seems quite excessive.

Is there no way to test this in a less-expensive manner?

The test dwarf-size-field-overflow.test now takes over 60 seconds to run, which seems quite excessive.

Is there no way to test this in a less-expensive manner?

I almost halved the number of iterations in 1b9ef. On my iMac Pro in Release+Asserts it's running in 1.18s wall clock time now. Note however that even before my change it was only taking ~5s. Were you measuring a debug build?

The test dwarf-size-field-overflow.test now takes over 60 seconds to run, which seems quite excessive.

Is there no way to test this in a less-expensive manner?

I almost halved the number of iterations in 1b9ef. On my iMac Pro in Release+Asserts it's running in 1.18s wall clock time now. Note however that even before my change it was only taking ~5s. Were you measuring a debug build?

Yes, in debug mode.

Thanks -- it now takes 22 sec, which is better. (Before the first change, it took 12 sec).