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

[Pasi Parviainen via llvm-commits]

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