[PATCH] Fix crash in CheckObjCCircularContainer

[AlexDenisov]

Done, r244193.
--
AlexDenisov
Software Engineer, http://lowlevelbits.org

> On 05 Aug 2015, at 22:25, Argyrios Kyrtzidis <kyrtzidis at apple.com> wrote:
> 
>> +    IdentifierInfo *II = &S.Context.Idents.get(InterfaceDecl->getName());
> 
> 
> You can just do “InterfaceDecl->getIdentifier()”, no need to lookup by string. Also this will eliminate the need to pass Sema as parameter.
> And ‘isSubclassOfNSClass()’ seems generally useful, how about you make it a function of NSAPI ?
> 
> Otherwise LGTM.
> 
> 
>> On Aug 5, 2015, at 1:11 PM, AlexDenisov <1101.debian at gmail.com> wrote:
>> 
>>> Why not get the IdentifierInfo pointer for the class name from the NSAPI object and compare that ?
>> There are more than one way to do things, it’s just lack of knowledge about the code base.
>> 
>>> Also there is code duplication, since the same code pattern is used in 3 places, could you refactor into a function ?
>> 
>> Fixed this as well. Also, I got rid of `NSCountedSet` since it’s a subclass of `NSMutableSet` and will be caught by `isSubclassOfNSClass`.
>> 
>> The new version of the patch attached.
>> --
>> AlexDenisov
>> Software Engineer, http://lowlevelbits.org
>> 
>> <proper_fix_for_circular_containers_v2.patch>
>>> On 05 Aug 2015, at 19:22, Argyrios Kyrtzidis <kyrtzidis at apple.com> wrote:
>>> 
>>>> 
>>>> -  if (S.NSMutableArrayPointer != Message->getReceiverType()) {
>>>> +  ObjCInterfaceDecl *Receiver = Message->getReceiverInterface();
>>>> +  if (!Receiver) {
>>>> +    return None;
>>>> +  }
>>>> +
>>>> +  bool IsMutableArray = false;
>>>> +  do {
>>>> +    QualType QT = S.Context.getObjCInterfaceType(Receiver);
>>>> +    QualType ReceiverType = S.Context.getObjCObjectPointerType(QT);
>>>> +
>>>> +    IsMutableArray = !S.NSMutableArrayPointer.isNull() &&
>>>> +      ReceiverType == S.NSMutableArrayPointer;
>>>> +
>>>> +    if (IsMutableArray) {
>>>> +      break;
>>>> +    }
>>>> +  } while ((Receiver = Receiver->getSuperClass()));
>>>> +
>>>> +  if (!IsMutableArray) {
>>>>   return None;
>>>> }
>>>> 
>>> 
>>> Why not get the IdentifierInfo pointer for the class name from the NSAPI object and compare that ? It seems unnecessary to be doing the type lookups for this.
>>> 
>>> Also there is code duplication, since the same code pattern is used in 3 places, could you refactor into a function ?
>>> 
>>> 
>>> 
>>>> On Jul 31, 2015, at 1:55 PM, AlexDenisov <1101.debian at gmail.com> wrote:
>>>> 
>>>>> To clarify, are you saying that the warning may lead to false positives when used in subclasses ?
>>>> 
>>>> Seems I was wrong.
>>>> Just checked the behaviour with backing storage - it also leads to a circular container problem.
>>>> 
>>>> Also, you can find attachment with a ‘proper’ implementation, which also covers subclassing.
>>>> 
>>>> P.S. I didn’t measure performance, but I think this implementation might have negative impact on the speed.
>>>> --
>>>> AlexDenisov
>>>> Software Engineer, http://lowlevelbits.org
>>>> 
>>>> <proper_fix_for_circular_containers.patch>
>>>>> On 30 Jul 2015, at 18:18, Argyrios Kyrtzidis <kyrtzidis at apple.com> wrote:
>>>>> 
>>>>> 
>>>>>> On Jul 30, 2015, at 1:05 AM, AlexDenisov <1101.debian at gmail.com> wrote:
>>>>>> 
>>>>>> The patch is a simplest fix for crash when CheckObjCCircularContainer
>>>>>> applies to a message to a ’super’, e.g.:
>>>>>> 
>>>>>> @implementation Foo : NSMutableArray
>>>>>> - foo {
>>>>>> [super addObject:nil];
>>>>>> }
>>>>>> @end
>>>>>> 
>>>>>> 
>>>>>> This is, probably, not a proper fix for the problem,
>>>>>> but initial patch wasn’t intended to apply checks to any kind
>>>>>> of subclassing, because it, imho, over-complicates implementation:
>>>>> 
>>>>> To clarify, are you saying that the warning may lead to false positives when used in subclasses ?
>>>>> If that’s the case could we just disable it inside collection subclasses, at least until the false positives can be addressed ?
>>>>> 
>>>>>> 
>>>>>> This particular problem touches subclassing from a class-cluster,
>>>>>> which means that the concrete subclass will have some backing storage, e.g.:
>>>>>> 
>>>>>> @implementation FootableArray : NSMutableArray
>>>>>> {
>>>>>> NSMutableArray *_backingStorage;
>>>>>> }
>>>>>> 
>>>>>> - addObject:(id)object {
>>>>>> [_backingStorage addObject:object];
>>>>>> }
>>>>>> 
>>>>>> @end
>>>>>> 
>>>>>> In this case even adding `self` to `self` would not lead to a circular container:
>>>>>> 
>>>>>> - foo {
>>>>>> [self addObject:self]; // puts `self` into the `_backingStorage`
>>>>>> }
>>>>>> 
>>>>>> I would apply this patch as is and postpone a ‘proper and bullet-proof implementation’
>>>>>> when I, or somebody who is also interested, will have more time.
>>>>>> 
>>>>>> If there are any questions/suggestions/objections - let’s discuss them.
>>>>>> --
>>>>>> AlexDenisov
>>>>>> Software Engineer, http://lowlevelbits.org
>>>>>> 
>>>>>> 
>>>>>> <fix_circular_container_crash.patch>
>>>>> 
>>>> 
>>> 
>> 
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 496 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150806/b92af6d0/attachment.sig>