Page MenuHomePhabricator

[DwarfDebug] Skip entries to big for 16 bit size field in Dwarf < 5.
ClosedPublic

Authored by fhahn on Mar 18 2019, 3:37 PM.

Details

Summary

Nothing prevents entries from being bigger than the 16 bit size field in
Dwarf < 5. For entries that are too big, there is nothing we can do
other than not emitting them, unless I missed something.

This fixes PR41038.

The minimal test case attached to the bug report is 500 KB as bitcode and
2.2 MB as IR. Not sure if it makes sense to attach it?

Diff Detail

Repository
rL LLVM

Event Timeline

fhahn created this revision.Mar 18 2019, 3:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2019, 3:38 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

Would generating the testcase on-the-fly inside the RUN: line or perhaps via python be an option? Otherwise I guess this is fine, too.

probinson added a comment.EditedMar 19 2019, 5:51 AM

Generating the test on the fly with Python seems plausible. Note you don't need 16K components, just enough components to make the location description exceed 16K bytes. If each component is (say) a 64-bit field with a max-int constant value, that's something like 11 bytes of description per field, so ~1500 fields ought to do it.

  • Duh, not 16K bytes. 64K bytes. ~6000 fields.
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1987 ↗(On Diff #191195)

We have to emit a zero length here, otherwise the location-list entry will be syntactically incorrect.

fhahn updated this revision to Diff 191294.Mar 19 2019, 7:14 AM

Add test generated using Python and set size to 0.

fhahn marked an inline comment as done.Mar 19 2019, 7:18 AM

Generating the test on the fly with Python seems plausible. Note you don't need 16K components, just enough components to make the location description exceed 16K bytes. If each component is (say) a 64-bit field with a max-int constant value, that's something like 11 bytes of description per field, so ~1500 fields ought to do it.

  • Duh, not 16K bytes. 64K bytes. ~6000 fields.

Thanks! I've added a script that generates a crashing test case. 11K seemed to be the lowest threshold.

llvm/test/MC/X86/dwarf-size-field-overflow.test
48 ↗(On Diff #191294)

If you make this (i * 1000000) you will need about half as many fields, which will make the test run a little faster.
DWARF expressions will encode the constant value in the smallest possible size, regardless of the size of the associated field; so 0 encodes in 1 byte, 512 in 2 bytes, and so on, even for a 64-bit field. That's why larger constant values will mean you need fewer fields.
It's a bit picky, but as it's trivial to make this test run faster (many fewer lines to generate and parse etc) it seems worthwhile.

fhahn updated this revision to Diff 191342.Mar 19 2019, 10:09 AM

Reduce number of dbg value calls to 4000.

probinson accepted this revision.Mar 19 2019, 12:21 PM

LGTM

llvm/test/MC/X86/dwarf-size-field-overflow.test
9 ↗(On Diff #191294)

I could wish for something more obviously "no expression here" but that's not your doing.

This revision is now accepted and ready to land.Mar 19 2019, 12:21 PM
fhahn marked an inline comment as done.Mar 19 2019, 1:34 PM

Thanks for all the feedback!

llvm/test/MC/X86/dwarf-size-field-overflow.test
9 ↗(On Diff #191294)

Yes something that makes it more obvious would be great!

This revision was automatically updated to reflect the committed changes.

Filed PR41152 for the unobvious display of a zero-length location expression.

dblaikie added inline comments.
llvm/trunk/test/MC/X86/dwarf-size-field-overflow.test
8–9

This looks incorrect - is it?

By setting the value to zero, now it refers to a different location list than the one that's intended, right? (it refers to the first one)

So while it doesn't crash, it produces the wrong description (describing one variable as being at the location of another variable).

Seems like if we're going to support this (out of curiosity - where did this come up in the wild? Would it be reasonable to reject this with a report_fatal_error, perhaps? (I know, not elegant/nice, but figured I'd ask)) perhaps we need to keep track of the location offsets as they are added and not include a DW_AT_location at all if its offset would be too large? Not sure if that's possible (if we have the necessary information about all prior location lists to compute their size) - might require delaying adding DW_AT_locations.

(@aprantl @probinson - am I misreading this? What're your thoughts on this?)

probinson added inline comments.Mar 26 2019, 8:20 AM
llvm/trunk/test/MC/X86/dwarf-size-field-overflow.test
8–9

I read this entry as being a location list at offset 0x0 in the location-list section, and that list has one entry for address range 0x0 to 0x8, with a zero-length location description (the entity has no location).
It would be nice if the dump explicitly indicated the zero-length location description, but I believe it's syntactically correct DWARF.
PR41038 describes how it came up in a Swift program.

We could side-step the issue by emitting a single DW_OP_nop to indicate that something went wrong.

dblaikie added inline comments.Mar 26 2019, 9:02 AM
llvm/trunk/test/MC/X86/dwarf-size-field-overflow.test
8–9

Ah, thanks - I see my misreading. I thought this was emitting the DW_AT_location value, the offset within the debug_loc section, and that that offset was too large.

My mistake.

Should we skip the entry entirely then, rather than emit an entry with no location/empty location? (Ideally, I suppose, then - if we only have one entry and its too large, we should skip the list entirely too)

Out of curiosity, because I can't quite spot it - where is this part of debug_loc defined in DWARFv4? I don't see any mention of a location description being prepended by its length there - but I'm guessing I'm misreading/missing it in there somewhere.

2.6.2 says that a location list entry consists of a beginning address, an ending address, and then a single location description. 2.6.1 which describes location descriptions doesn't appear to say anything about a length prefix?

Out of curiosity, because I can't quite spot it - where is this part of debug_loc defined in DWARFv4? I don't see any mention of a location description being prepended by its length there - but I'm guessing I'm misreading/missing it in there somewhere.

No, it's just spectacularly hard to find. In the v4 spec this tidbit is hidden away in section 7.7.3, second paragraph. "A location list entry consists of two address offsets followed by a 2-byte length, followed by a block of contiguous bytes that contains a DWARF location description."

Out of curiosity, because I can't quite spot it - where is this part of debug_loc defined in DWARFv4? I don't see any mention of a location description being prepended by its length there - but I'm guessing I'm misreading/missing it in there somewhere.

No, it's just spectacularly hard to find. In the v4 spec this tidbit is hidden away in section 7.7.3, second paragraph. "A location list entry consists of two address offsets followed by a 2-byte length, followed by a block of contiguous bytes that contains a DWARF location description."

Ah, thanks!