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

[Sourabh Singh Tomar via Phabricator via llvm-commits]

SouraVX added a comment.

Hi @dblaikie, It seems to that existing implementation also contains a bug. As the Declaration implies here/Please Correct me if I've mistaken/

  using MacroList = SmallVector<Entry, 4>;
  
    /// A list of all the macro entries in the debug_macinfo section.  --- slightly ambigous
    std::vector<MacroList> MacroLists;  -- collection of macrolist per CU's ? correct?. 

hence the dumping code --

  for (const auto &Macros : MacroLists) {  -- iterate through all CU's MacroList ?
      for (const Entry &E : Macros) {

But if you closely notice, all contributions from different CU's are ending up in `MacroList` as opposed to intended `MacroLists`. Hence, even `MacroLists` is vector, still contains only one `MacroList` regardless of the no. of CU's present in macinfo section.  Is this functionality intended ? Then why a vector to be used here.

This bug originates while parsing the content:

  MacroList *M = nullptr;
    while (data.isValidOffset(Offset)) {
  if (!M) {
       MacroLists.emplace_back();
       M = &MacroLists.back();
     }

This minor patch removes the vector and still correctly dumps single/multiple CU's from macinfo section.

  diff --git a/llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h b/llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h
  index 7880bcdf688..4fe3bf43e79 100644
  --- a/llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h
  +++ b/llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h
  @@ -42,7 +42,7 @@ class DWARFDebugMacro {
     using MacroList = SmallVector<Entry, 4>;
  
     /// A list of all the macro entries in the debug_macinfo section.
  -  std::vector<MacroList> MacroLists;
  +  MacroList MacroLists;
  
   public:
     DWARFDebugMacro() = default;
  diff --git a/llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp b/llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp
  index 8cb259ebc62..fa7f69f16d8 100644
  --- a/llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp
  +++ b/llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp
  @@ -17,8 +17,7 @@ using namespace dwarf;
  
   void DWARFDebugMacro::dump(raw_ostream &OS) const {
     unsigned IndLevel = 0;
  -  for (const auto &Macros : MacroLists) {
  -    for (const Entry &E : Macros) {
  +    for (const Entry &E : MacroLists) {
         // There should not be DW_MACINFO_end_file when IndLevel is Zero. However,
         // this check handles the case of corrupted ".debug_macinfo" section.
         if (IndLevel > 0)
  @@ -51,21 +50,14 @@ void DWARFDebugMacro::dump(raw_ostream &OS) const {
         }
         OS << "\n";
       }
  -    OS << "\n";
  -  }
   }
  
   void DWARFDebugMacro::parse(DataExtractor data) {
     uint64_t Offset = 0;
  -  MacroList *M = nullptr;
     while (data.isValidOffset(Offset)) {
  -    if (!M) {
  -      MacroLists.emplace_back();
  -      M = &MacroLists.back();
  -    }
       // A macro list entry consists of:
  -    M->emplace_back();
  -    Entry &E = M->back();
  +    MacroLists.emplace_back();
  +    Entry &E = MacroLists.back();
       // 1. Macinfo type
       E.Type = data.getULEB128(&Offset);

Should we correct/fix this or is it intended part of functionality ?


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

https://reviews.llvm.org/D73086