[llvm] r261298 - llvm-dwp: Don't test compression when zlib isn't available

[David Blaikie via llvm-commits]

On Thu, Feb 18, 2016 at 6:23 PM, Davide Italiano <davide at freebsd.org> wrote:

> On Thu, Feb 18, 2016 at 6:03 PM, David Blaikie via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
> > Author: dblaikie
> > Date: Thu Feb 18 20:03:45 2016
> > New Revision: 261298
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=261298&view=rev
> > Log:
> > llvm-dwp: Don't test compression when zlib isn't available
> >
> > Modified:
> >     llvm/trunk/test/tools/llvm-dwp/X86/compress.test
> >     llvm/trunk/tools/llvm-dwp/llvm-dwp.cpp
> >
> > Modified: llvm/trunk/test/tools/llvm-dwp/X86/compress.test
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-dwp/X86/compress.test?rev=261298&r1=261297&r2=261298&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/test/tools/llvm-dwp/X86/compress.test (original)
> > +++ llvm/trunk/test/tools/llvm-dwp/X86/compress.test Thu Feb 18 20:03:45
> 2016
> > @@ -1,6 +1,8 @@
> >  RUN: llvm-dwp %p/../Inputs/compress/a.dwo -o %t
> >  RUN: llvm-dwarfdump %t | FileCheck %s
> >
> > +REQUIRES: zlib
> > +
> >  Simple test built from this input which produces DWARF long enough to
> be compressed in the .[z]debug_info section:
> >
> >    void f(int a, int b, int c, int d) {
> >
> > Modified: llvm/trunk/tools/llvm-dwp/llvm-dwp.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-dwp/llvm-dwp.cpp?rev=261298&r1=261297&r2=261298&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/tools/llvm-dwp/llvm-dwp.cpp (original)
> > +++ llvm/trunk/tools/llvm-dwp/llvm-dwp.cpp Thu Feb 18 20:03:45 2016
> > @@ -350,7 +350,7 @@ static std::error_code write(MCStreamer
> >          uint64_t OriginalSize;
> >          if (!zlib::isAvailable() ||
> >              !consumeCompressedDebugSectionHeader(Contents,
> OriginalSize))
> > -          continue;
> > +          return make_error_code(std::errc::invalid_argument);
> >          UncompressedSections.resize(UncompressedSections.size() + 1);
> >          if (zlib::uncompress(Contents, UncompressedSections.back(),
> OriginalSize) !=
> >              zlib::StatusOK) {
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
> I picked one random llvm-dwp commit to make a more general comment
> (please bear with me).
> Please consider those as suggestions, and feel free to disagree with me.
>

Sure thing - thanks for keeping an eye out/providing feedback, in any case!


> I've been trying to make over the last months the error handling in
> the tools a little bit more uniform (Rafael did some earlier work on
> this area too).
> Almost each tool I touched calls exit(1) on error, whilst I noticed
> you just return an error code to the caller. Is there a particular
> reason why you're doing this? From what I see all the error conditions
> you're running into are fatal (at least this is what I gathered from a
> quick scan of the code).
>

In short, I wouldn't try to draw any conclusions about error handling from
this code as it is today. It asserts on various invalid inputs where it
should have error handling, it returns vague error codes where accurate
diagnostics would be desired, etc.

In short, I haven't made any real decisions about the best way to do error
handling here.

In theory, I like building things as a library, so I'm hesitant to exit
mid-way through various underlying functions, but making that work is,
well, work. error_code is insufficient (doesn't traffic important
information back to the caller (like any parameters about the specific
instance of a problem (file name, etc))) and still requires a bit of
overhead to setup specific error_category enums that might be appropriate
for the domain. Diagnostic handlers also require a bit of setup and don't
always fit perfectly (& still need to ferry the failure up to the
top/intermediate points to ensure further work isn't done).

I'm actually at a point where I want to improve/deliberately choose at
least one diagnostic experience (duplicate CU DWO IDs - the existing
binutils dwp tool just prints the duplicate ID, not the names of the two
files those IDs came from, which isn't really helpful)


>
> Also, writeStringsAndOffsets() returns std::error_code() even if it
> never fails for now.
> I assumed you put that "just in case" but in other tools (most
> noticeably lld) we decided to change return type to std::error to void
> until we don't have a case where the function can fail for real.
>

I'd generally agree, for sure. Not sure how that one got there - perhaps I
needed it at one point, or thought I was about to use it in a CL and didn't
end up using it but it stuck around.

- Dave


>
> Thanks,
>
> --
> Davide
>
> "There are no solved problems; there are only problems that are more
> or less solved" -- Henri Poincare
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160219/66a4bb43/attachment.html>