[PATCH] D73086: [DWARF5] Added support for debug_macro section parsing and dumping in llvm-dwarfdump.
David Blaikie via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 2 19:56:02 PST 2020
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
More information about the llvm-commits
mailing list