[PATCH] D74309: [DebugInfo] Add check for zero debug line opcode_base

[James Henderson via Phabricator via llvm-commits]

jhenderson marked 2 inline comments as done.
jhenderson added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:358-367
+  if (OpcodeBase == 0) {
+    // If the opcode base is 0, we cannot read the standard opcode lengths (of
+    // which there are supposed to be one fewer than the opcode base). Assume
+    // there are no standard opcodes and continue parsing.
+    RecoverableErrorCallback(createStringError(
+        errc::invalid_argument,
+        "parsing line table prologue at offset 0x%8.8" PRIx64
----------------
probinson wrote:
> dblaikie wrote:
> > probinson wrote:
> > > dblaikie wrote:
> > > > 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)."
> > > > 
> > > > 
> > > 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.
> > 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)
> That's reasonable, to accept 1 (which implies the standard_opcode_lengths array has zero elements) but warn on 0.
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.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/Inputs/debug_line_malformed.s:398-411
+.asciz "dir1"           # Include table
+.asciz "dir2"
+.byte   0
+.asciz "file1"          # File table
+.byte   0, 0, 0
+.asciz "file2"
+.byte   1, 2, 3
----------------
dblaikie wrote:
> 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?)
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74309/new/

https://reviews.llvm.org/D74309