[PATCH] D73618: [DebugInfo] Check that we do not run past a line table end when parsing

[James Henderson via Phabricator via llvm-commits]

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


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test:39
 ## Don't stop looking for the later unit if non-fatal issues are found.
-# RUN: llvm-dwarfdump -debug-line=0x2ec %t-malformed.o 2> %t-malformed-off-last.err \
+# RUN: llvm-dwarfdump -debug-line=0x339 %t-malformed.o 2> %t-malformed-off-last.err \
 # RUN:   | FileCheck %s --check-prefix=LAST --implicit-check-not='debug_line[{{.*}}]'
----------------
dblaikie wrote:
> jhenderson wrote:
> > dblaikie wrote:
> > > Should this dump in verbose mode to show more clearly which operations were parsed and which ones were not? So they match up with the LNE_* descriptions in the comments in the test?
> > > 
> > > 
> > This last check is just for the last (good) section, so doesn't really need to show things in that much detail. However, I could certainly see a benefit for checking things more verbosely. We already do verbose dumps on line 29 of the test, but the checks don't check the operands. I'll look at updating that, but it should be a separate change.
> Oh, I figured this change related to the other change in this review (Case 13) which wasn't super clear to me & figured verbosity might help.
It's only indirectly related. Case 13 required adding new data to the input, which pushed the last table later, meaning the offset needed updating.


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp:794-796
+  // Arbitrary padding byte to ensure the end of the opcode as claimed by the
+  // opcode length field does not go past the table end.
+  LT.addByte(0xaa);
----------------
dblaikie wrote:
> jhenderson wrote:
> > dblaikie wrote:
> > > Could you explain this further? What's incorrect about the existing usage (where is the opcode length field? Is that the 0x2 in the line above? Why would it be too short? (should the DWARFGenerator API be changed? is it computing a length that's too short for the table?))
> > `addExtendedOpcode` takes a length for an extended opcode, followed by the opcode itself, and then data for the operands. I realise this byte should be moved into the operands argument, so I'll fix that. I don't think the API here needs changing, as it would prevent us creating broken situations like this (i.e. an extended opcode without sufficient data for its claimed length).
> What's the mention of the "table end" here, then? I guess "table end" is indicated by the table length field elsewhere? Is it autogenerated (with a possible override for invalid situations)? Should it be autogenerated differently? 
Yeah, the table length is auto-generated, but it's auto-generated based on the raw data we add (i.e. not the interpretation of the data, but the physical number of bytes we add). See `writeDefaultPrologue` in DWARFGenerator.cpp. The ability to override the length is provided by the `createBasicPrologue` function which returns a line table prologue that can be updated for use in the test. See e.g. `ErrorForTooLargePrologueLength`. I think this is a reasonable approach.

In this test case, we are overriding the DW_LNE_end_sequence length with something different, by specifying the "2" argument. Previously, by not specifying any data after it in the test, the majority of the bytes covered by this size were past the end of the table, which is itself an interesting test case, albeit not the focus of this test. Consequently it was triggering the new error, which I didn't want. When I next update this patch, I'll replace this and the previous line of code and comment with:
`LT.addExtendedOpcode(0x2, DW_LNE_end_sequence, {{LineTable::Byte, 0xaa});`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73618