[Lldb-commits] [PATCH] D66791: [lldb][ELF] Read symbols from .gnu_debugdata sect.

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/lit/Modules/ELF/minidebuginfo-corrupt-xz.yaml:1
+# REQUIRES: system-linux, lzma
+
----------------
kwk wrote:
> labath wrote:
> > system-linux shouldn't be required here. One of the reasons I wanted to have these tests is that they can run on any system, not just on linux.
> @labath what about the other file `minidebuginfo-set-and-hit-breakpoint.test`. Remove `system-linux` from there as well?
You need to restrict that test somehow, because it only has a chance for working on elf platforms (as you're compiling binaries for the host and running them). It probably *should* work on all elf platforms, but we currently don't have a good way of expressing that and we're just restricting to system-linux currently.

If you feel adventurous you can try introducing a `system-elf` feature (or something), and then changing this test (and possibly others) to use that instead, but please do that as a separate patch.


================
Comment at: lldb/source/Host/common/LZMA.cpp:129
+  uint64_t uncompressedSize = 0;
+  auto err = getUncompressedSize(InputBuffer, uncompressedSize);
+  if (err)
----------------
kwk wrote:
> labath wrote:
> > MaskRay wrote:
> > > if (auto err = ...)
> > >   return err;
> > I have a feeling this pattern is actually more common in lldb. I tend to use this one, but I am fine with either.
> So what now? I had `getCompressedSize` return an error before and changed it upon request to expected. I can live with both but like the expected thing a bit better in terms of: this goes in and this comes out mentality.
My interpretation of @MaskRay's comment was that he wanted you do write something like 
```
if (auto err = uncompressedSize.takeError())
  return err;
```
That is orthogonal to what type is returned by `getCompressedSize`, and is indeed a more common pattern in at least some parts of llvm. I am personally fine with both.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66791