[llvm-dwarfdump] - Print an error message if section decompression failed.

[grimar]

Summary

llvm-dwarfdump currently prints no message if decompression fails
for some reason. I noticed that during work on one of LLD patches
where LLD produced an broken output. It was a bit confusing to see
no output for section dumped and no any error message at all.

Patch adds error message for such cases.

[aprantl]

Seems generally fine, couple of suggestions inline.

include/llvm/DebugInfo/DWARF/DWARFContext.h

313 ↗

(On Diff #97827)

I would remove the "maybe" here.

lib/DebugInfo/DWARF/DWARFContext.cpp

948 ↗

(On Diff #97827)

Is a section likely to be < 32 bytes?

[grimar]

Thanks for review Adrian ! My answers inline.

include/llvm/DebugInfo/DWARF/DWARFContext.h

313 ↗

(On Diff #97827)

It can be a bit confusing as "decompress" looks like we always do that but this method do nothing if content is uncompressed.
Looking in other LLVM code, maybeDoSomething looks to be used naming for such purposes.
I can't say I like it though, but not sure what can be some different good naming for functionality like "decompess data only if it is compressed".

One way would be to rename it to "decompress" but wrap in check like:

 if (Decompressor::isCompressed(Sec)) {
     if (auto Err = decompress(Section, name, data)) {
        errs() << "error: failed to decompress '" + name + "', " +
                    toString(std::move(Err))
             << '\n';
      continue;
}

but that would expose Decompressor outside decompress() method and looks not better IMO.

lib/DebugInfo/DWARF/DWARFContext.cpp

948 ↗

(On Diff #97827)

I don't think so. But that is a part of original code, I just moved this line and would not do any changes
in this patch for it.

32 probably can be a good size estimation for testcases we have around, though not sure it is worth to rely on them.

One more thing I just noticed. 'Out' is used for decompress() call which has next interface:

Error Decompressor::decompress(SmallString<32> &Out)

so changing it needs update of API. I think it is probably reasonable to make it be more generic like

Error Decompressor::decompress(SmallVectorImpl<char> &Out)

[aprantl]

include/llvm/DebugInfo/DWARF/DWARFContext.h

313 ↗

(On Diff #97827)

Okay, thanks for the explanation it wasn't clear to me that it is also expected to be used with uncompressed input. Could you please add a doxygen comment to the function explaining that it decompresses the section or otherwise returns its contents verbatim?

lib/DebugInfo/DWARF/DWARFContext.cpp

948 ↗

(On Diff #97827)

Yes, I agree. I would personally prefer a std::vector& here, because it is highly unlikely to ever use the "small" case and the small vector then just adds extra overhead.

[grimar]

include/llvm/DebugInfo/DWARF/DWARFContext.h

313 ↗

(On Diff #97827)

r302249. Hope it looks ok :]

lib/DebugInfo/DWARF/DWARFContext.cpp

948 ↗

(On Diff #97827)

I'll try to revisit this place and Decompressor API on next week.

llvm/trunk/include/llvm/DebugInfo/DWARF/DWARFContext.h

class DWARFContextInMemory : public DWARFContext {
   StringRef CUIndexSection;
   StringRef GdbIndexSection;
   StringRef TUIndexSection;
 
   SmallVector<SmallString<32>, 4> UncompressedSections;
 
   StringRef *MapSectionToMember(StringRef Name);
 
+  Error maybeDecompress(const object::SectionRef &Sec, StringRef Name,
+                        StringRef &Data);
+
 public:
   DWARFContextInMemory(const object::ObjectFile &Obj,
     const LoadedObjectInfo *L = nullptr);
 
   DWARFContextInMemory(const StringMap<std::unique_ptr<MemoryBuffer>> &Sections,
                        uint8_t AddrSize,
                        bool isLittleEndian = sys::IsLittleEndianHost);
 

llvm/trunk/lib/DebugInfo/DWARF/DWARFContext.cpp

static bool isRelocScattered(const object::ObjectFile &Obj,
   if (!MachObj)
     return false;
   // MachO also has relocations that point to sections and
   // scattered relocations.
   auto RelocInfo = MachObj->getRelocation(Reloc.getRawDataRefImpl());
   return MachObj->isRelocationScattered(RelocInfo);
 }
 
+Error DWARFContextInMemory::maybeDecompress(const SectionRef &Sec,
+                                            StringRef Name, StringRef &Data) {
+  if (!Decompressor::isCompressed(Sec))
+    return Error::success();
+
+  Expected<Decompressor> Decompressor =
+      Decompressor::create(Name, Data, IsLittleEndian, AddressSize == 8);
+  if (!Decompressor)
+    return Decompressor.takeError();
+
+  SmallString<32> Out;
+  if (auto Err = Decompressor->decompress(Out))
+    return Err;
+
+  UncompressedSections.emplace_back(std::move(Out));
+  Data = UncompressedSections.back();
+
+  return Error::success();
+}
+
 DWARFContextInMemory::DWARFContextInMemory(const object::ObjectFile &Obj,
     const LoadedObjectInfo *L)
     : IsLittleEndian(Obj.isLittleEndian()),
       AddressSize(Obj.getBytesInAddress()) {
   for (const SectionRef &Section : Obj.sections()) {
     StringRef name;
     Section.getName(name);
     // Skip BSS and Virtual sections, they aren't interesting.
     bool IsBSS = Section.isBSS();
     if (IsBSS)
       continue;
     bool IsVirtual = Section.isVirtual();
     if (IsVirtual)
       continue;
     StringRef data;
 
     section_iterator RelocatedSection = Section.getRelocatedSection();
     // Try to obtain an already relocated version of this section.
     // Else use the unrelocated section from the object file. We'll have to
     // apply relocations ourselves later.
     if (!L || !L->getLoadedSectionContents(*RelocatedSection,data))
       Section.getContents(data);
 
-    if (Decompressor::isCompressed(Section)) {
-      Expected<Decompressor> Decompressor =
-          Decompressor::create(name, data, IsLittleEndian, AddressSize == 8);
-      if (!Decompressor)
-        continue;
-      SmallString<32> Out;
-      if (auto Err = Decompressor->decompress(Out))
-        continue;
-      UncompressedSections.emplace_back(std::move(Out));
-      data = UncompressedSections.back();
+    if (auto Err = maybeDecompress(Section, name, data)) {
+      errs() << "error: failed to decompress '" + name + "', " +
+                    toString(std::move(Err))
+             << '\n';
     }
 
     // Compressed sections names in GNU style starts from ".z",
     // at this point section is decompressed and we drop compression prefix.
     name = name.substr(
         name.find_first_not_of("._z")); // Skip ".", "z" and "_" prefixes.
 
     if (StringRef *SectionData = MapSectionToMember(name)) {

llvm/trunk/test/DebugInfo/dwarfdump-decompression-error.test

+REQUIRES: zlib
+
+// dwarfdump-decompression-error.elf-x86-64 is prepared using following
+// source code and invocation:
+// test.cpp:
+// int main() { return 0; }
+//
+// gcc test.cpp -o out -g -Wl,--compress-debug-sections,zlib
+//
+// After that result object was modified manually. One random byte in compressed
+// content of .debug_info section was changed to 0xff. That breaks normal 
+// decompression flow in runtime.
+RUN: llvm-dwarfdump %p/Inputs/dwarfdump-decompression-error.elf-x86-64 2>&1 | FileCheck %s
+
+CHECK: error: failed to decompress '.debug_info', zlib error: Z_DATA_ERROR