[PATCH] D32713: [DWARFv5] Parse new line-table header format.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLine.cpp:90-93
+    if (s && s[0])
+      IncludeDirectories.push_back(s);
+    else
+      break;
----------------
Hmm - getCStr has to find the null terminator anyway (to move the offset forward) - perhaps it'd be good to have a version that returns StringRef so the length doesn't need to be recomputed? & then use that here:

  if (s.empty())
    break;
  IncludeDirectories.push_back(s);

(reducing indentation, etc)


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLine.cpp:97-98
+  while (*offset_ptr < end_prologue_offset) {
+    const char *name = debug_line_data.getCStr(offset_ptr);
+    if (name && name[0]) {
+      DWARFDebugLine::FileNameEntry fileEntry;
----------------
Same thing here - and much more code to benefit from unindenting


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLine.cpp:134-136
+  SmallVector<ContentDescriptor, 4> Descriptors;
+  if (!parseV5EntryFormat(debug_line_data, offset_ptr, end_prologue_offset,
+                          Descriptors))
----------------
Would it be reasonable to have parseV5EntryFormat return the vector instead of taking an out parameter? (& have empty represent failure - or if necessary, ErrorOr<SmallVector>)


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLine.cpp:159-160
+  }
+  if (IncludeDirectories.size() != EntryCount)
+    return false;
+
----------------
Looks like this only happens if Descriptors doesn't contain DW_LNCT_path - worth checking for that separately/up-front to be more explicit?


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLine.cpp:171
+
+  EntryCount = debug_line_data.getU8(offset_ptr);
+  for (unsigned i = 0; i < EntryCount; ++i) {
----------------
Might be better to use a different variable name here to make it clear that these are separate things with separate sizes, etc. (maybe even bundling up the file extraction and directory extracton into separate helper functions)


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLine.cpp:172
+  EntryCount = debug_line_data.getU8(offset_ptr);
+  for (unsigned i = 0; i < EntryCount; ++i) {
+    if (*offset_ptr >= end_prologue_offset)
----------------
Generally != EntryCount rather than < EntryCount.

& the loop may be better optimized with int rather than unsigned, unfortunately :)


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLine.cpp:177
+    for (auto Descriptor : Descriptors) {
+      DWARFFormValue Value(dwarf::Form(Descriptor.second));
+      if (!Value.extractValue(debug_line_data, offset_ptr, nullptr))
----------------
Any chance Descriptor.second could be typed as dwarf::Form to begin with to avoid the need for a cast here?


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLine.cpp:200
+  }
+  if (FileNames.size() != EntryCount)
+    return false;
----------------
When would this condition occur? Looks like the only way through the loop that doesn't add an element to FileNames is the "return false" for extractValue failure (& the other return false for an earlier error) so the loop necessarily adds an element for every iteration of EntryCount times, etc.


================
Comment at: test/DebugInfo/Inputs/dwarfdump-header.s:205
+        .short  5               # DWARF version number
+	.byte   8               # Address Size
+        .byte   0               # Segment Selector Size
----------------
indent?


================
Comment at: test/DebugInfo/Inputs/dwarfdump-header.s:231
+        .byte   0x08            # DW_FORM_string
+	# Directory table entries
+        .byte   2               # Two directories
----------------
indent?


https://reviews.llvm.org/D32713