[llvm] r252192 - Reapply r250906 with many suggested updates from Rafael Espindola.

[Rafael Espíndola via llvm-commits]

I am taking a look. That buffer has just been created by lld, it is
odd that it is failing.

On 6 November 2015 at 14:38, Pasi Parviainen <pasi.parviainen at iki.fi> wrote:
> On 6.11.2015 20:15, Kevin Enderby wrote:
>>
>> Hello Pasi,
>>
>> When I ran a make check-all I did not see any crashes.  Can you point me
>> to the test case for the lld-link crash?  Then I can fix this. Top of tree
>> for my just now with a make check-all also is clean and the 4 lld build bots
>> appear to be green at this time.
>
>
> Failing tests are:
>     lld :: COFF/dll.test
>     lld :: COFF/export-exe.test
>     lld :: COFF/export.test
>     lld :: COFF/export32.test
>     lld :: COFF/noentry.test
>
> All these tests are marked to require winlib, so windows bot
> (lld-x86_64-win7) could have detected these, if it only were running the
> tests.
>
>> The constructor for Archive::Child::Child() only allows nullptr as EC
>> argument when Parent and Start are also nullptr.  Which is needed for the
>> default constructor of child_iterator().  At one point we needed this as I
>> think ArchiveWriter was using this.  As it would have seemed to be more
>> correct to pass EC as a reference but I didn’t see how to make that work
>> with a default constructor for child_iterator().
>>
>> In the original diff I had this as an assert (see
>> http://reviews.llvm.org/D13989 <http://reviews.llvm.org/D13989> ):
>>
>>      if (MemberSize.getError()) {
>>        assert (EC && "Error must be caught");
>>        *EC = MemberSize.getError();
>>        return;
>>      }
>>
>> But Rafael said in the review: "You can write this as”:
>>
>>       if ((EC = MemberSize.getError())
>>        return;
>>
>> The fix to a crash lld-link is likely simply passing a proper value for EC
>> and testing for the error.  And I suspect this is what Rafael would like to
>> see as he doesn’t want the code to eat these errors in any place.
>
>
> Then the interface contract is violated in ShortImportCreator::create method
> (lld/COFF/DriverUtils.cpp) where constructor is called with non-null Start
> argument. It ends with:
>
>         object::Archive::Child C(Parent, Buf, nullptr);
>         return NewArchiveIterator(C, DLLName);
>
> Pasi
>
>
>> Happy to make this change if you can point me to the crash or the test
>> case.
>>
>> Kev
>>
>>> On Nov 6, 2015, at 12:23 AM, Pasi Parviainen <pasi.parviainen at iki.fi>
>>> wrote:
>>>
>>>> @@ -86,7 +86,8 @@ Archive::Child::Child(const Archive *Par
>>>>                         uint16_t StartOfFile)
>>>>       : Parent(Parent), Data(Data), StartOfFile(StartOfFile) {}
>>>>
>>>> -Archive::Child::Child(const Archive *Parent, const char *Start)
>>>> +Archive::Child::Child(const Archive *Parent, const char *Start,
>>>> +                      std::error_code *EC)
>>>>       : Parent(Parent) {
>>>>     if (!Start)
>>>>       return;
>>>> @@ -94,7 +95,10 @@ Archive::Child::Child(const Archive *Par
>>>>     uint64_t Size = sizeof(ArchiveMemberHeader);
>>>>     Data = StringRef(Start, Size);
>>>>     if (!isThinMember()) {
>>>> -    Size += getRawSize();
>>>> +    ErrorOr<uint64_t> MemberSize = getRawSize();
>>>> +    if ((*EC = MemberSize.getError()))
>>>> +      return;
>>>
>>>
>>> lld-link is crashing here since matching changes to lld in r252193. Crash
>>> is caused by passing nullptr as EC argument. If nullptr is valid value for
>>> EC then this check should be more like:
>>>
>>>     if (!MemberSize) {
>>>       if (EC)
>>>         *EC = MemberSize.getError();
>>>       return;
>>>     }
>>>
>>> Pasi
>>
>>
>>
>