<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Feb 18, 2016 at 6:23 PM, Davide Italiano <span dir="ltr"><<a href="mailto:davide@freebsd.org" target="_blank">davide@freebsd.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Thu, Feb 18, 2016 at 6:03 PM, David Blaikie via llvm-commits<br>
<<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
> Author: dblaikie<br>
> Date: Thu Feb 18 20:03:45 2016<br>
> New Revision: 261298<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=261298&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=261298&view=rev</a><br>
> Log:<br>
> llvm-dwp: Don't test compression when zlib isn't available<br>
><br>
> Modified:<br>
> llvm/trunk/test/tools/llvm-dwp/X86/compress.test<br>
> llvm/trunk/tools/llvm-dwp/llvm-dwp.cpp<br>
><br>
> Modified: llvm/trunk/test/tools/llvm-dwp/X86/compress.test<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-dwp/X86/compress.test?rev=261298&r1=261297&r2=261298&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-dwp/X86/compress.test?rev=261298&r1=261297&r2=261298&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/test/tools/llvm-dwp/X86/compress.test (original)<br>
> +++ llvm/trunk/test/tools/llvm-dwp/X86/compress.test Thu Feb 18 20:03:45 2016<br>
> @@ -1,6 +1,8 @@<br>
> RUN: llvm-dwp %p/../Inputs/compress/a.dwo -o %t<br>
> RUN: llvm-dwarfdump %t | FileCheck %s<br>
><br>
> +REQUIRES: zlib<br>
> +<br>
> Simple test built from this input which produces DWARF long enough to be compressed in the .[z]debug_info section:<br>
><br>
> void f(int a, int b, int c, int d) {<br>
><br>
> Modified: llvm/trunk/tools/llvm-dwp/llvm-dwp.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-dwp/llvm-dwp.cpp?rev=261298&r1=261297&r2=261298&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-dwp/llvm-dwp.cpp?rev=261298&r1=261297&r2=261298&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/tools/llvm-dwp/llvm-dwp.cpp (original)<br>
> +++ llvm/trunk/tools/llvm-dwp/llvm-dwp.cpp Thu Feb 18 20:03:45 2016<br>
> @@ -350,7 +350,7 @@ static std::error_code write(MCStreamer<br>
> uint64_t OriginalSize;<br>
> if (!zlib::isAvailable() ||<br>
> !consumeCompressedDebugSectionHeader(Contents, OriginalSize))<br>
> - continue;<br>
> + return make_error_code(std::errc::invalid_argument);<br>
> UncompressedSections.resize(UncompressedSections.size() + 1);<br>
> if (zlib::uncompress(Contents, UncompressedSections.back(), OriginalSize) !=<br>
> zlib::StatusOK) {<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br>
</div></div>I picked one random llvm-dwp commit to make a more general comment<br>
(please bear with me).<br>
Please consider those as suggestions, and feel free to disagree with me.<br></blockquote><div><br></div><div>Sure thing - thanks for keeping an eye out/providing feedback, in any case!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I've been trying to make over the last months the error handling in<br>
the tools a little bit more uniform (Rafael did some earlier work on<br>
this area too).<br>
Almost each tool I touched calls exit(1) on error, whilst I noticed<br>
you just return an error code to the caller. Is there a particular<br>
reason why you're doing this? From what I see all the error conditions<br>
you're running into are fatal (at least this is what I gathered from a<br>
quick scan of the code).<br></blockquote><div><br></div><div>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.<br><br>In short, I haven't made any real decisions about the best way to do error handling here.<br><br>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).<br><br>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)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Also, writeStringsAndOffsets() returns std::error_code() even if it<br>
never fails for now.<br>
I assumed you put that "just in case" but in other tools (most<br>
noticeably lld) we decided to change return type to std::error to void<br>
until we don't have a case where the function can fail for real.<br></blockquote><div><br></div><div>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.<br><br>- Dave</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Thanks,<br>
<span class="HOEnZb"><font color="#888888"><br>
--<br>
Davide<br>
<br>
"There are no solved problems; there are only problems that are more<br>
or less solved" -- Henri Poincare<br>
</font></span></blockquote></div><br></div></div>