[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#1902193 <https://reviews.llvm.org/D73086#1902193>, @dblaikie wrote:

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


Done that in 4ce3e5074bbc25825220457001da45eb76788392 <https://reviews.llvm.org/rG4ce3e5074bbc25825220457001da45eb76788392>


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

https://reviews.llvm.org/D73086