Re: CA broken; how do I differentiate NSValues for size and point?

[Fred Kiefer]

Hi Ivan,

great analysis and nice code to reproduce this, but you seem to have missed my 
mail on the issue (even though it is included in your mail) and the work around 
I added to base. I was already fully aware of this and the patch I added should 
prevent your test code from throwing the exception. Of course this is not the 
real solution. We may have to add the same checks for known types in 
GSObjCSetVal that we already have in other places. This is rather ugly code 
which I would prefer to avoid. I hope to see Richard in two weeks time and we 
may come up with a better solution together.

If the issue persists even with my work around in place it would be good to 
know. I have a few ideas for better work arounds but would prefer a cleaner 
solution.

Fred

PS: In your test code the second PASS is missing a „!“.

> Am 27.05.2018 um 19:44 schrieb Ivan Vučica <address@hidden>:
> 
> I'll open a bug for -base. In the meantime I'll help Stjepan disable 
> shadowOffset.
> 
> Minimum repro::
> 
> #import <Foundation/Foundation.h>
> 
> @interface A : NSObject
> {
>   NSSize _s;
> }
> @end
> @implementation A
> - (NSSize) s {
>   return self->_s;
> }
> - (void) setS: (NSSize) s {
>   self->_s = s;
> }
> @end
> int main() {
>   NSSize in = NSMakeSize(1.0, 2.0);
>   NSValue * v = [NSValue valueWithBytes: &in
>                                objCType: @encode(NSSize)];
>   A * a = [A new];
>   [a setValue: v
>        forKey: @"s"];
> 
> 
>   return 0;
> }
> 
> 
> Output:
> $ clang `gnustep-config --objc-flags` `gnustep-config --objc-libs` 
> `gnustep-config --base-libs` repro.m -o repro && ./repro
> clang: warning: argument unused during compilation: '-fobjc-nonfragile-abi'
> clang: warning: argument unused during compilation: '-fobjc-nonfragile-abi'
> 2018-05-27 18:42:13.137 repro[14432:14432] match! point is {_NSPoint=dd}, 
> type is {_NSSize=dd}
> ./repro: Uncaught exception NSInvalidArgumentException, reason: 
> [GSSizeValue-pointValue] should be overridden by subclass
> Aborted
> 
> 
> 
> 
> 
> On Sun, May 27, 2018 at 6:18 PM Ivan Vučica <address@hidden> wrote:
> So turns out we don't use shadowOffset in any demo code. So examining 
> CAAnimation was pointless.
> 
> However I managed to get a backtrace using lldb, and this happens when the 
> code sets the default shadowOffset value. So you pointed to the right place; 
> shadowOffset is the problem. This is the place of the exception, in -[CALayer 
> init] which calls +[CALayer defaultValueForKey:]:
> 
>   if ([key isEqualToString: @"shadowOffset"])                                 
>                                
>     {
>       CGSize offset = CGSizeMake(0.0, -3.0);
>       return [NSValue valueWithBytes: &offset objCType: @encode(CGSize)];
>     }
> 
> What is strange is that this happens deep inside base, when I am setting 
> valueWithBytes, with a CGSize type.
> 
> So I looked closer at the backtrace and it's happening in 
> base/Source/Additions/GSObjCRuntime.m. Relevant chunk:
> 
>     frame #2: 0x00007ffff59e9d7d libgnustep-base.so.1.25`+[NSException 
> raise:format:](self=0x00007ffff5f69878
> , _cmd="S", name=0x00007ffff5f69438, format=0x00007ffff6003828) + 365 at 
> NSException.m:1376
>     frame #3: 0x00007ffff5b9ca2f 
> libgnustep-base.so.1.25`-[NSObject(self=0x000000000148f568, _cmd="\xffffffa9
> \x02", aSel="\x0e\x01") subclassResponsibility:] + 255 at 
> NSObject+GNUstepBase.m:134
>     frame #4: 0x00007ffff5b1d32b libgnustep-base.so.1.25`-[NSValue 
> pointValue](self=0x000000000148f568, _cmd=
> "\x0e\x01") + 43 at NSValue.m:394
>     frame #5: 0x00007ffff5b58649 
> libgnustep-base.so.1.25`GSObjCSetVal(self=0x00000000014591c8, key="shadowOff
> set", val=0x000000000148f568, sel="\xffffffda\e", type="{_NSSize=dd}", 
> size=12, offset=0) + 3497 at GSObjCRun
> time.m:1794
>     frame #6: 0x00007ffff5a25857 
> libgnustep-base.so.1.25`SetValueForKey(self=0x00000000014591c8, anObject=0x0
> 00000000148f568, key="shadowOffset", size=12) + 1079 at NSKeyValueCoding.m:150
>     frame #7: 0x00007ffff5a253ee 
> libgnustep-base.so.1.25`-[NSObject(self=0x00000000014591c8, _cmd="P\x04", an
> Object=0x000000000148f568, aKey=0x00007ffff62cc560) setValue:forKey:] + 382 
> at NSKeyValueCoding.m:370
>     frame #8: 0x00007ffff5a2881d libgnustep-base.so.1.25`-[GSKVOBase 
> setValue:forKey:](self=0x00000000014591c
> 8, _cmd="P\x04", anObject=0x000000000148f568, aKey=0x00007ffff62cc560) + 221 
> at NSKeyValueObserving.m:238
>     frame #9: 0x00007ffff60a22ac libQuartzCore.so.0`-[CALayer 
> init](self=0x00000000014591c8, _cmd="\xffffffa1
> 
> I added a debug statement to GSObjCSetVal:
> 
>           case _C_STRUCT_B:
>             if (GSSelectorTypesMatch(@encode(NSPoint), type))
>               {
>                 NSLog(@"match! point is %s, type is %s", @encode(NSPoint), 
> type);
>                 NSPoint v = [val pointValue];
> 
> This is the output:
> 2018-05-27 17:58:46.980 hello_carenderer[10274:10274] match! point is 
> {_NSPoint=dd}, type is {_NSPoint=dd}
> 2018-05-27 17:58:46.981 hello_carenderer[10274:10274] match! point is 
> {_NSPoint=dd}, type is {_NSSize=dd}
> 
> Therefore, it's a bug in -base that makes it interpret sizes as points!
> 
> I'm still coming up with a minimum repro case.
> 
> (n.b. some of the confusion on why CGSize and CGPoint get understood as their 
> NS equivalents is this chunk from opal:
>   typedef NSPoint CGPoint;
>   typedef NSSize CGSize;
>   typedef NSRect CGRect;
> I do not believe this to be correct, but I will not be addressing it at this 
> time.
> 
> Separately: are typedefs of structs meant to be encoded as the original 
> struct name? Probably yes?)
> 
> 
> On Sun, May 27, 2018 at 5:29 PM Ivan Vučica <address@hidden> wrote:
> I added a test to the -base from May 20:
> 
>   NSPoint point = {.x = 16.0, .y = 32.0};
>   NSValue *pointV = [NSValue valueWithPoint: point];
> 
>   result = !strcmp(@encode(NSPoint), [pointV objCType]);
>   PASS(result, "@encoding(NSPoint) == pointV objCType");
> 
>   result = strcmp(@encode(NSSize), [pointV objCType]);
>   PASS(result, "@encoding(NSSize) == pointV objCType");
> 
> with this temporarly ine:
>   NSLog(@"%s %s %s", @encode(NSPoint), @encode(NSSize), [pointV objCType]);
> 
> It passes and prints out:
> 
> 2018-05-27 17:27:38.823 size-point-encoding[25395:25395] {_NSPoint=dd} 
> {_NSSize=dd} {_NSPoint=dd}
> 
> I''m annoyed at how I did not notice that I did use CGSize. I will introduce 
> support for NSSize/CGSize.
> 
> Thanks!
> 
> On Mon, May 21, 2018 at 12:53 AM Fred Kiefer <address@hidden> wrote:
> I added a bit of code in base that allows to use NSValue objects for size and 
> point with methods for the other type. This is a bit closer to the Cocoa 
> behaviour but will require more tweaking to have it fully correct. It will at 
> least allow you to run this test code again.
> 
> > Am 20.05.2018 um 16:02 schrieb Fred Kiefer <address@hidden>:
> > 
> > I spend some time to understand this issue. But to be honest it took me 
> > more time to understand why this ever worked :-)
> > 
> > The problem here is that base uses two different ways to provide concrete 
> > subclasses for NSValue. One being GSValue, which supports generic data and 
> > the other mechanism is to have a specific subclass for types like NSPoint 
> > and NSSize. For the later base only generates specific methods for that 
> > types. Resulting in different classes for NSPoint and NSSize, which are 
> > incompatible although they have the same underlying data size.
> > In your demo application that animation for shadowOffset tries to set the 
> > offset size with a suitable default value of class GSSizeValue. But the 
> > code in GSObjCSetVal only compares the type information without looking at 
> > the type names. That way NSSize is regarded as the same as NSPoint and the 
> > pointValue get executed, resulting in the error you reported. Either 
> > GSObjCSetVal has to be more careful and use a check for the actual type 
> > first. Or we need to make the NSValue subclasses more general.
> > 
> > But why did it work before? Most likely because at that time CGSize and 
> > CGPoint, where different from NSSize and NSPoint so we did not get the 
> > specific optimisation in NSValue.
> > 
> > Hope this helps,
> > Fred
> > 
> >> Am 20.05.2018 um 14:03 schrieb Ivan Vučica <address@hidden>:
> >> 
> >> Hi,
> >> 
> >> Pretty much all Core Animation demos are currently broken under GNUstep 
> >> with a variation on the following:
> >> 
> >> 2018-05-20 12:54:25.464 QuartzCoreDemo[13476:13476] Problem posting 
> >> notification: <NSException: 0x15b53b8> NAME:NSInvalidArgumentException 
> >> REASON:[GSSizeValue-pointValue] should be overridden by subclass 
> >> INFO:(null)
> >> 
> >> CA is accessing -pointValue method if it determines that the passed 
> >> NSValue matches the -objCType of NSPoint. It does not check for size 
> >> values.
> >> 
> >> Clearly, sometimes it is trying to interpolate size values, which will 
> >> match the signature and it will incorrectly attempt to access -pointValue 
> >> which is then not implemented by GSSizeValue. I am not sure where might it 
> >> be interpolating size values, but it seems to be doing so. (Alternative 
> >> bug is that NSValue is incorrectly ingesting points as sizes, then 
> >> complaining when the point provided is being interpreted as a point. I can 
> >> try checking this later.)
> >> 
> >> I cannot check -respondsToSelector: because the class /does/ in fact 
> >> respond to -pointValue; it just throws an exception.
> >> 
> >> Adding a try/catch in this kind of situation would make for some very poor 
> >> code hygiene, in my opinion.
> >> 
> >> - Is NSValue supposed to be a class cluster like this? (Not on Mac at this 
> >> time, can't check.)
> >> - Is there a way out?
> >> - Would it make sense to extend GSSizeValue and add -pointValue to it? 
> >> (They're both 2d vectors.)
> >> 
> >> Thanks