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

[Pasi Parviainen via llvm-commits]

On 6.11.2015 22:00, Rafael Espíndola wrote:
> Should be fixed by r252336.
>
> sorry about that.

Thank you, that did indeed fix the crashes.

Pasi.

> 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
>>>>
>>>>
>>>>
>>>