[PATCH] D77557: [DWARFDebugLine] Use truncating data extractors for prologue parsing

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Looks good to me, but I'll hold off approval so you can touch up the tests.



================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/Inputs/debug_line_malformed.s:195
 .Linvalid_description_header_end0:
-# The bytes from here onwards will also be read as part of the main body.
-                        # --- Prologue interpretation --- | --- Main body interpretation ---
-.byte   0, 1            # More standard opcodes           | First part of DW_LNE_end_sequence
-# Directory table format
-.byte   1               # One element per directory entry | End of DW_LNE_end_sequence
-.byte   1               # DW_LNCT_path                    | DW_LNS_copy
-.byte   0x08            # DW_FORM_string                  | DW_LNS_const_add_pc
-# Directory table entries
-.byte   1               # 1 directory                     | DW_LNS_copy
-.asciz  "/tmp"          # Directory name                  | four special opcodes + start of DW_LNE_end_sequence
-# File table format
-.byte   1               # 1 element per file entry        | DW_LNE_end_sequence length
-.byte   1               # DW_LNCT_path                    | DW_LNE_end_sequence opcode
-.byte   0x08            # DW_FORM_string                  | DW_LNS_const_add_pc
-# File table entries
-.byte   1               # 1 file                          | DW_LNS_copy
-.asciz  "xyz"           # File name                       | three special opcodes + start of DW_LNE_set_address
-# Header end
+.byte   0, 1, 1         # DW_LNE_end_sequence
+.byte   1               # DW_LNS_copy
----------------
labath wrote:
> jhenderson wrote:
> > labath wrote:
> > > jhenderson wrote:
> > > > I wonder if it's worth a comment at the start here saying something to explain why the formatting of the following bytes is weird. Perhaps something like "The following bytes would be read as part of the prologue header, were data extraction not stopped once the data end is reached, hence they are formatted like a header".
> > > > 
> > > > Something similar applies for the other changes in this file.
> > > Actually, I was thinking of just deleting this stuff as it is no longer interesting to test. I wanted to do it as a separate commit, as that will change the offsets of everything, making it hard to see what changed. WDYT?
> > I'm happy to remove any unnecessary additional test cases, where they provide no new value, but we need to be slightly careful we don't lose coverage of parsing the file/directory name tables. If a test case doesn't cover a unique code path, then I'm good for it to be deleted, and as a separate patch is fine.
> I'm not talking about deleting entire test cases -- just the bits of line tables that are prologue extensions. They were needed/interesting to test when the prologue parsing would not respect prologue boundaries, but now I don't think they serve any purpose and I think we can just put a simple DW_LNE_end_sequence as the line table contents.
The "standard" contents are a set address and end sequence for reasons I forget these days, so we should probably keep just that much to match the other cases. Beyond that, I'm happy for things to be simplified where they don't add value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77557