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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D73086#1901390 <https://reviews.llvm.org/D73086#1901390>, @SouraVX wrote:

> 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 ?


Yeah, it looks like maybe the missing code that was probably intended (by the existence of MacroLists, etc), was this an assignment of null to M, here:

  if (E.Type == 0) {
    // Reached end of a ".debug_macinfo" section contribution.
    M = nullptr;
    continue;
  }

& some test that dumps more than one contribution & shows they're dumped as separate results - though it seems the dumping code doesn't print anything at the start of a MacroList (between the "for MacroLists" and "for Macros" loop headers) - so it'd be impossible to test this change. Also, such a header would be useful/necessary for reading the DWARF in a meaningful way - currently there's no way to know which debug_macro contribution is associated with which CU - the DW_AT_macro_info attribute prints the offset, but without those offsets printed in the debug_macinfo section, there's no way to know where the macros for a given CU are. See the simple "0x00000000:" header printed at the start of debug_loc and debug_ranges contributions - something like that would be good.


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

https://reviews.llvm.org/D73086