[PATCH] D13989: This removes the eating of the error in Archive::Child::getSize() when the charactersin the size field in the archive header for the member is not a number.

[Kevin Enderby via llvm-commits]

Hi Rafael,

OK, I audit the patch so that every error in handled.  And that patch is:
-------------- next part --------------
A non-text attachment was scrubbed...
Name: audited_t.diff
Type: application/octet-stream
Size: 31394 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151103/a90b0398/attachment.obj>
-------------- next part --------------

I changed things to handle the error in the same matter as each tool seemed to handle similar errors.


But honestly I was taking what you said at the start of this all:

> On Oct 13, 2015, at 2:23 PM, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
> 
> Ideally we want to push them up until we get out of lib, at which
> point we are in tools and can handle the error in whatever way is
> appropriate for that tool.


to mean that once the error was pushed out of the lib the tool could really could handle the error in whatever way is appropriate for that tool, including eating the error and ignoring it.

These places where it was eating the error were in the tools not the lib, like llvm-nm and llvm-objdump, were we ignored the error and just moved on to dumping the next thing.  Which at the time I felt was appropriate for those tools.  As I said above, I now have them handing this error in same matter as each tool seemed to handle similar errors.  In some cases where the archive is contained in a universal file it could “continue" on to the next item in the universal file, but since it seems you don’t want to handle corrupt it now returns.

Kev

> On Nov 2, 2015, at 5:39 PM, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
> 
> OK, I converted a few more uses of archive_iterator that never store
> an error to Child.
> 
> There are still a few cases where we are eating the error:
> 
> +                if (AI->getError())
> +                  break;
> 
> Please audit the patch so that every error in handled.
> 
> Rebased patch attached.
> 
> Cheers,
> Rafael
> 
> 
> 
> 
> On 2 November 2015 at 18:31, Rafael Espíndola
> <rafael.espindola at gmail.com> wrote:
>>> I would say it depends on the tool and what the tool is doing.
>>> 
>>> In the case above the llvm-ar tool is printing out the archive header.  Since I often run tools on malformed files during development I like tools to show be what is there rather than just calling “fail” and bailing out in my opinion.  In this case what I really want is to see the characters of the archive header’s field for Size as during development I likely messed things up and need to see what is there so I can fix things.
>>> 
>>> But to do this I would need to use the getHeader() method then print the Size[10] characters in this case.  And this really should be done for all the fields.  Which would mean making all the methods like getUID(), getGID(), etc, all return ErrorOr<> like getSize().  But that seams like changes that should be left for another patch.
>> 
>> Maybe. But we should not be changing behavior in this patch. And when
>> adding better error messages, it should still be done by calling fail.
>> It is important that we get a non zero error code and don't complicate
>> the code trying to handle corrupt files.
>> 
>>> I agree with you on that but I think the two places that needed this change need to deal better with errors.  As they are in the Jit and should not call report_fatal_error() with malformed input.  But I think it best for Lang or others to clean up the error handling and work to eliminate the use of report_fatal_error() for malformed input.
>> 
>> Yes. Before your first patch this was getting to llvm_unreachable, so
>> it is an improvement.
>> 
>> Taking a look at the new patch.
>> 
>> Cheers,
>> Rafael
> <t.diff>