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

[Rafael Espíndola via llvm-commits]

Should be fixed by r252336.

sorry about that.

Cheers,
Rafael


On 6 November 2015 at 14:48, Rafael Espíndola
<rafael.espindola at gmail.com> wrote:
> 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
>>>
>>>
>>>
>>