[PATCH] D73086: [DWARF5] Added support for debug_macro section parsing and dumping in llvm-dwarfdump.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:91-97
+      // Contribution of one CU finished, dump this and
+      // check if Offset is still valid, if so, start parsing/dumping
+      // debug_macro again.This is not needed for debug_macinfo section since
+      // contribution from different CU's got merged into single debug_macinfo
+      // section with macro termination sequence at the end of the section.
+      if (getVersion() >= 5)
+        return;
----------------
I can't seem to make sense of this comment. Both the commend and the code seem incorrect to me - there should still be one debug_macro contribution per-CU in DWARFv5 (it'd not be possible for there to be only one across all CUs without DWARF-aware linking, which is something the DWARF spec tries very hard to not require)

Have you tried compiling two files with DWARFv5 debug_macro contents using your prototype patches & then looking at the result? I would expect there should be two debug_macro contributions, and that this functionality as currently commented/written would only dump one of them.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:108-117
     case DW_MACINFO_define:
     case DW_MACINFO_undef:
       // 2. Source line
-      E.Line = data.getULEB128(&Offset);
+      E.Line = data.getULEB128(Offset);
       // 3. Macro string
-      E.MacroStr = data.getCStr(&Offset);
+      E.MacroStr = data.getCStr(Offset);
+      break;
----------------
This switch has a mix of DW_MACRO and DW_MACINFO enumerators in it, which seems suspicous/questionable.

Perhaps it'd be better to use DW_MACRO macros exclusively and include a comment prior to the switch explaining that the macinfo values are a strict subset (with identical constant values) of the macro values that existed previously?


================
Comment at: llvm/test/DebugInfo/X86/debug-macro-v5.s:11-29
+# CHECK-NEXT: DW_MACRO_start_file - lineno: 0 filenum: 0
+# CHECK-NEXT:   DW_MACRO_start_file - lineno: 1 filenum: 1
+# CHECK-NEXT:     DW_MACRO_define_strp - lineno: 1 macro: FOO 5
+# CHECK-NEXT:   DW_MACRO_end_file
+# CHECK-NEXT:   DW_MACRO_start_file - lineno: 2 filenum: 2
+# CHECK-NEXT:     DW_MACRO_define_strp - lineno: 1 macro: BAR 6
+# CHECK-NEXT:   DW_MACRO_end_file
----------------
Maybe strip this down to just one example of each of the new-in-v5 forms?


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

https://reviews.llvm.org/D73086