[PATCH] D104271: llvm-dwarfdump: Print warnings on invalid DWARF

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp:103
+}
+
 DWARFDebugAbbrev::DWARFDebugAbbrev() { clear(); }
----------------
jhenderson wrote:
> jankratochvil wrote:
> > This function look to me as too much code for too little benefit but when @dblaikie has requested it then why not.
> > 
> I'm not entirely convinced we need to print all valid abbrev values, even in ranged form, since typically, the abbrevs will go from 1-max in a contiguous set. That being said, I'm not opposed to it.
> 
> I think the code could be simplified anyway. Something like the following is more readable to me. It also handles adjacent codes being non-contiguous within the set of abbrevs:
> ```
> std::string DWARFAbbreviationDeclarationSet::getCodeRange() const {
>   // Create a sorted list of all abbrev codes.
>   std::vector<uint32_t> Codes;
>   Codes.reserve(Decls.size());
>   std::transform(Decls.begin(), Decls.end(), std::back_inserter(Codes),
>     [](const DWARFAbbreviationDeclaration &Decl){
>       return Decl.getCode();
>   });
>   std::sort(Codes.begin(), Codes.end());
> 
>   std::string Buffer = "";
>   raw_string_ostream Stream(Buffer);
>   // Each iteration through this look represents a single contiguous range in the set of codes.
>   for(auto Current = Codes.begin(), End = Codes.end(); Current != End;) {
>     uint32_t RangeStart = *Current;
>     // Add the current range start.
>     Stream << *Current;
>     uint32_t RangeEnd = RangeStart;
>     // Find the end of the current range.
>     while(++Current != End && *Current == RangeEnd + 1)
>       ++RangeEnd;
>     // If there is more than one value in the range, add the range end too.
>     if (RangeStart != RangeEnd)
>       Stream << "-" << RangeEnd;
>     // If there is at least one more range, add a separator.
>     if (Current != End)
>       Stream << ", ";
>   }
>   return Buffer;
> }
> ```
Yep, all this complexity would fall under the clause I mentioned in the original feedback: " (I forget if the abbrev table is necessarily contiguous - if it isn't, then maybe that's too complicated)" - so the table isn't contiguous, and maybe this is too complicated to be worth it? I really don't mind either way, at this point.

@jhenderson's version seems nice, if we're going to do this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104271