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

Tue Apr 7 01:35:50 PDT 2020

jhenderson added a comment.

In D77557#1964697 <https://reviews.llvm.org/D77557#1964697>, @dblaikie wrote:

> Some nice cleanup indeed - thanks! Please wait for @jhenderson to also approve, since he's been doing more work in this area. (never sure whether to approve or not in this case - not sure if the official "approval" might then remove it from other people's review queue, etc)

FWIW, in my case, I just use my emails as my review queue, so I don't mind others approving too.

================
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
----------------
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.

================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/Inputs/debug_line_malformed.s:324

-# Invalid MD5 hash, when data beyond the prologue length has
-# been read before the MD5 problem is identified.
+# Header while reading an md5 hash
 .long   .Linvalid_md5_end1-.Linvalid_md5_start1   # Length of Unit
----------------
I think this comment is missing a word somewhere. Probably "truncated"? Also missing a trailing full stop. Also "md5" -> "MD5".

================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test:115
 # NONFATAL-NEXT: Line table prologue
-# NONFATAL:      standard_opcode_lengths[DW_LNS_set_isa] = 1
-# NONFATAL-NEXT: include_directories[  0] = "/tmp"
-# NONFATAL-NEXT: file_names[  0]:
-# NONFATAL-NEXT:            name: "xyz"
+# NONFATAL:      standard_opcode_lengths[DW_LNS_set_isa] = 0
+# NONFATAL-NOT:  include_directories
----------------
This should probably show that the last opcode within range is read successfully in addition to showing the past-the-end one is recorded as zero.

================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test:143

-## V5 invalid MD5 hash form when data beyond the prologue length has
-## been read before the MD5 problem is identified.
+## V5 prologue ends while reading an MD5 hash
 # NONFATAL:      debug_line[0x000002ae]
----------------
Missing trailing full stop.

This may not have been clear from the test comments before, but this and the previous case were supposed to work in tandem, because the invalid MD5 form message was reported in both cases, but not the "not at end" message IIRC.

I feel like the input for this case probably should be updated to not have a malformed MD5 form. Is there any interaction any more to do with continuing to parse that these 2 cases now don't need to cover?

================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test:206
+# ALL-NEXT: warning: parsing line table prologue at 0x000001ee found an invalid directory or file table description at 0x00000219
+# ALL-NEXT: warning: failed to parse entry content descriptors: malformed uleb128, extends past end
+# ALL-NEXT: warning: parsing line table prologue at 0x0000022f found an invalid directory or file table description at 0x00000251
----------------
How straightforward would it be to provide the offset as extra context here, like we do with "unexpected end of data"?

================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp:470
     Errs.emplace_back(
-        (Twine("parsing line table prologue at 0x00000000 should have ended at "
-               "0x000000") +
-         Twine::utohexstr(ExpectedEnd) + " but it ended at 0x000000" +
-         Twine::utohexstr(ExpectedEnd + 2))
-            .str());
+        "failed to parse file entry because extracting the form value failed.");
   }
----------------
I'm not sure where this error is coming from (I don't recall seeing it before), but it's got a spurious trailing full stop, which is inconsistent with the other error messages. It might want fixing separately I guess.

================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp:478-482
-        (Twine("parsing line table prologue at 0x00000000 should have ended at "
-               "0x000000") +
-         Twine::utohexstr(ExpectedEnd) + " but it ended at 0x000000" +
-         Twine::utohexstr(ExpectedEnd + 2))
-            .str());
----------------
What happened to this error message?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77557