Page MenuHomePhabricator

[DebugInfo] Add check for zero debug line opcode_base
ClosedPublic

Authored by jhenderson on Mon, Feb 10, 3:56 AM.

Details

Summary

The number of standard opcodes is defined to be opcode_base - 1, so a value of 0 for the opcode_base caused a crash as an attempt was made to reserve many entries in a vector. This change fixes the crash, by issuing a warning and skipping reading of standard opcode lengths in the event of an opcode_base of 0.

Diff Detail

Event Timeline

jhenderson created this revision.Mon, Feb 10, 3:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Feb 10, 3:56 AM
Herald added a subscriber: hiraditya. · View Herald Transcript

Please add an assembly+dumper test or a unit test, not both, in this instance (& usually preferred in general - there are some cases where it's worthwhile having both).

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
359–374

I don't /think/ this merits a warning. It seems to be unexceptionally DWARF conformant to have an opcode base of 0:

"Opcode base is typically one greater than the highest-numbered standard opcode defined for the specified version of the line number information (12 in DWARF Versions 3, 4 and 5, and 9 in Version 2). If opcode_base is less than the typical value, then standard opcode numbers greater than or equal to the opcode base are not used in the line number table of this unit (and the codes are treated as special opcodes)."

llvm/test/tools/llvm-dwarfdump/X86/Inputs/debug_line_malformed.s
398–411

Since this is only testing opcode base - perhaps it could have fewer/shorter details here? One directory and one file, perhaps? (I guess you've added/plan to add sufficient validation that it wouldn't be acceptable (would produce other warnings that would complicate the test case) to have zero files/directories and a line table program that's only a DW_LNE_end_sequence?)

probinson added inline comments.Mon, Feb 10, 2:59 PM
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
359–374

That non-normative comment seems to have a reasonable interpretation for an opcode base of 0, but then the normative definition of standard_opcode_lengths in the next item isn't really written in a way that supports an opcode base of less than 2.

Opcode base is really "the first special opcode" and zero can't be a special opcode because zero introduces an extended opcode. So I agree with James, it is reasonable to have this warning.

dblaikie added inline comments.Mon, Feb 10, 3:17 PM
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
359–374

Ah, sorry - I think I meant to delete this comment when I read further.

I think it supports the opcode base of 1, maybe? (at least there's no interpretation I can find that would differentiate the behavior of opcode base 1 or 0, at least? Which to me, means it makes sense that the reasonable thing to do is to use 1 and never use zero from a normalization perspective - and that zero is weird enough that warning seems OK to me)

probinson added inline comments.Mon, Feb 10, 4:34 PM
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
359–374

That's reasonable, to accept 1 (which implies the standard_opcode_lengths array has zero elements) but warn on 0.

jhenderson marked 2 inline comments as done.Tue, Feb 11, 3:26 AM
jhenderson added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
359–374

Yup, that all matches my understanding as I read the spec. An opcode_base of 0 would imply "-1" standard opcodes, and a first special opcode value of 0 (which would clash with the definition of an extended opcode). so, 1 is fine, but 0 isn't.

llvm/test/tools/llvm-dwarfdump/X86/Inputs/debug_line_malformed.s
398–411

I'd like to keep at least one directory and file table, as it shows what happens with the prologue parsing after the bad opcode_base (i.e. it keeps going, assuming no opcode lengths). There's probably no need for more than one of each though. Also, I've deliberately got a special opcode, because a) the opcode_base is used in the calculation this triggers, and b) 0x1 would normally be a standard opcode.

Rebase, remove unit test and remove redundant parts of lit test.

dblaikie accepted this revision.Tue, Feb 11, 8:06 AM

Looks good, thanks!

This revision is now accepted and ready to land.Tue, Feb 11, 8:06 AM
This revision was automatically updated to reflect the committed changes.