[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3413
+        section->GetName().GetCString());
+    return 0;
   }
----------------
lemo wrote:
> labath wrote:
> > `lit/Modules/compressed-sections.yaml` test will need to be updated to account for the return 0.
> compressed-sections.yaml seems to be gated by REQUIRES: zlib, so it would not hit the 0-length path, am I wrong?
The `.bogus` section in that test deliberately contains malformed data, so you should still hit the first error case.

However, looking at the lldb-test implementation, it just ignores the `ReadSectionData` return value, and relies on the size in the DataExtractor instead (one day I'll declare war on functions of this type), which is probably why you hadn't noticed this.

I guess in this case, it would be nice to reset the data extractor before returning 0 and possibly teaching lldb-test to report discrepancies in the returned sizes.


================
Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3413
+        section->GetName().GetCString(),
+        llvm::toString(Decompressor.takeError()).c_str());
+    return 0;
----------------
This needs to be `std::move(Error)`. If you built with asserts enabled and hit this line, you would crash because you did not consume the `Error` object.


https://reviews.llvm.org/D50274