GSIMap
Hi, I'm trying to make NSHashTable / NSMapTable use the correct read / write barrier functions in GC mode, but I don't really understand the GSIMap code. Does it define macros for reading / writing pointers anywhere? In GC mode, we need to call the relevant read and write barrier functions for assigning pointer values, depending in the pointer functions: For storing strong pointers, we should call objc_assign_strongCast(value, address), and read directly. For storing weak pointers, we should call objc_assign_weak(value, address) and read using objc_read_weak(address). This is the main thing required still for full Apple-compatible GC support. There also seem to be a few places (I've not identified them yet) where GNUstep code is storing strong pointers in globals that are not marked as strong. The compiler will only generate a write barrier if the pointer is either id, or has the __strong qualifier. Without these, we need to mark the entire data segment as potentially containing pointers, which adds some overhead (I'm currently doing this, but I'd like to turn it off). David -- Sent from my brain ___ Gnustep-dev mailing list Gnustep-dev@gnu.org https://lists.gnu.org/mailman/listinfo/gnustep-dev
Re: GSIMap
On 1 Jun 2011, at 19:30, David Chisnall wrote: > Hi, > > I'm trying to make NSHashTable / NSMapTable use the correct read / write > barrier functions in GC mode, but I don't really understand the GSIMap code. > Does it define macros for reading / writing pointers anywhere? In GC mode, > we need to call the relevant read and write barrier functions for assigning > pointer values, depending in the pointer functions: NSHashTable and NSMapTable use both NSPointerFunctions and the old callbacks (Apple added new classes for these objects, while retaining backward compatibility with the old API)... see NSConcretePointerFunctions.[hm] for the new functions and the CallBacks files for the old ones. If you look at the actual hash/map table code (eg NSConcreteHashTable.m) you will see the defines which tell GSIMAP which versions to use. eg. #define GSI_MAP_RETAIN_KEY(M, X)\ (M->legacy ? M->cb.old.retain(M, X.ptr) \ : pointerFunctionsAcquire(&M->cb.pf, &X.ptr, X.ptr)) ___ Gnustep-dev mailing list Gnustep-dev@gnu.org https://lists.gnu.org/mailman/listinfo/gnustep-dev
Re: GSIMap
On 1 Jun 2011, at 23:20, Richard Frith-Macdonald wrote: > > On 1 Jun 2011, at 19:30, David Chisnall wrote: > >> Hi, >> >> I'm trying to make NSHashTable / NSMapTable use the correct read / write >> barrier functions in GC mode, but I don't really understand the GSIMap code. >> Does it define macros for reading / writing pointers anywhere? In GC mode, >> we need to call the relevant read and write barrier functions for assigning >> pointer values, depending in the pointer functions: > > NSHashTable and NSMapTable use both NSPointerFunctions and the old callbacks > (Apple added new classes for these objects, while retaining backward > compatibility with the old API)... see NSConcretePointerFunctions.[hm] for > the new functions and the CallBacks files for the old ones. > > If you look at the actual hash/map table code (eg NSConcreteHashTable.m) you > will see the defines which tell GSIMAP which versions to use. > eg. > #define GSI_MAP_RETAIN_KEY(M, X)\ > (M->legacy ? M->cb.old.retain(M, X.ptr) \ > : pointerFunctionsAcquire(&M->cb.pf, &X.ptr, X.ptr)) I am not sure this helps. The GSIMap code seems to do things like: GSI_MAP_RETAIN_KEY(map, node->key) node->key = key; This is actually wrong in retain / release mode (-retain is not guaranteed to return self), but in GC mode, these two lines need to somehow become: objc_assign_strongCast(key, &(node->key)); there also doesn't seem to be any macro for reading the keys. For example, when ever you read node->key or node->value as a weak pointer, lines like: GSI_MAP_EQUAL(map, node->key, key) Need to be expanded to something like: [objc_read_weak(&(node->key) isEqual: key] David -- Sent from my STANTEC-ZEBRA ___ Gnustep-dev mailing list Gnustep-dev@gnu.org https://lists.gnu.org/mailman/listinfo/gnustep-dev
Re: GSIMap
On 1 Jun 2011, at 23:48, David Chisnall wrote: > On 1 Jun 2011, at 23:20, Richard Frith-Macdonald wrote: > >> >> On 1 Jun 2011, at 19:30, David Chisnall wrote: >> >>> Hi, >>> >>> I'm trying to make NSHashTable / NSMapTable use the correct read / write >>> barrier functions in GC mode, but I don't really understand the GSIMap >>> code. Does it define macros for reading / writing pointers anywhere? In >>> GC mode, we need to call the relevant read and write barrier functions for >>> assigning pointer values, depending in the pointer functions: >> >> NSHashTable and NSMapTable use both NSPointerFunctions and the old callbacks >> (Apple added new classes for these objects, while retaining backward >> compatibility with the old API)... see NSConcretePointerFunctions.[hm] for >> the new functions and the CallBacks files for the old ones. >> >> If you look at the actual hash/map table code (eg NSConcreteHashTable.m) you >> will see the defines which tell GSIMAP which versions to use. >> eg. >> #define GSI_MAP_RETAIN_KEY(M, X)\ >> (M->legacy ? M->cb.old.retain(M, X.ptr) \ >> : pointerFunctionsAcquire(&M->cb.pf, &X.ptr, X.ptr)) > > I am not sure this helps. The GSIMap code seems to do things like: > > GSI_MAP_RETAIN_KEY(map, node->key) > node->key = key; > > This is actually wrong in retain / release mode (-retain is not guaranteed to > return self), The guarantee is that it's specifically documented to do so in the protocol see http://developer.apple.com/library/mac/#documentation/Cocoa/Reference/Foundation/Protocols/NSObject_Protocol/Reference/NSObject.html So any implementation of -retain which doesn't return self is faulty, and the programmer really deserves any error they have introduced by overriding/replacing the default one. > but in GC mode, these two lines need to somehow become: > > objc_assign_strongCast(key, &(node->key));\ > > there also doesn't seem to be any macro for reading the keys. For example, > when ever you read node->key or node->value as a weak pointer, lines like: > > GSI_MAP_EQUAL(map, node->key, key) > > Need to be expanded to something like: > > [objc_read_weak(&(node->key) isEqual: key] Then we need some modification for if/when we support non-boehm GC. The current sequence: node->key = key; GSI_MAP_RETAIN_KEY(map, node->key); could become: GSI_MAP_ACQUIRE_KEY(map, &key_in_node, key); and we could trivially add a macro to reference the map content. ___ Gnustep-dev mailing list Gnustep-dev@gnu.org https://lists.gnu.org/mailman/listinfo/gnustep-dev
Re: GSIMap
On 2 Jun 2011, at 06:28, Richard Frith-Macdonald wrote: > > On 1 Jun 2011, at 23:48, David Chisnall wrote: > >> On 1 Jun 2011, at 23:20, Richard Frith-Macdonald wrote: >> >>> >>> On 1 Jun 2011, at 19:30, David Chisnall wrote: >>> >>>> Hi, >>>> >>>> I'm trying to make NSHashTable / NSMapTable use the correct read / write >>>> barrier functions in GC mode, but I don't really understand the GSIMap >>>> code. Does it define macros for reading / writing pointers anywhere? In >>>> GC mode, we need to call the relevant read and write barrier functions for >>>> assigning pointer values, depending in the pointer functions: >>> >>> NSHashTable and NSMapTable use both NSPointerFunctions and the old >>> callbacks (Apple added new classes for these objects, while retaining >>> backward compatibility with the old API)... see >>> NSConcretePointerFunctions.[hm] for the new functions and the CallBacks >>> files for the old ones. >>> >>> If you look at the actual hash/map table code (eg NSConcreteHashTable.m) >>> you will see the defines which tell GSIMAP which versions to use. >>> eg. >>> #define GSI_MAP_RETAIN_KEY(M, X)\ >>> (M->legacy ? M->cb.old.retain(M, X.ptr) \ >>> : pointerFunctionsAcquire(&M->cb.pf, &X.ptr, X.ptr)) >> >> I am not sure this helps. The GSIMap code seems to do things like: >> >> GSI_MAP_RETAIN_KEY(map, node->key) >> node->key = key; >> >> This is actually wrong in retain / release mode (-retain is not guaranteed >> to return self), > > The guarantee is that it's specifically documented to do so in the protocol > see > http://developer.apple.com/library/mac/#documentation/Cocoa/Reference/Foundation/Protocols/NSObject_Protocol/Reference/NSObject.html > So any implementation of -retain which doesn't return self is faulty, and the > programmer really deserves any error they have introduced by > overriding/replacing the default one. My reading of that is that the requirement to return self is only a requirement for objects implementing their own reference counting. The more formal description of -retain in the static analyser expects -retain to return an owning reference, but not necessarily to the same object as the receiver. This is used in NeXT code for immutable objects that are allocated in temporary zones, for example. They return a copy on retain, so that the entire zone can be freed at once, giving a very primitive form of generational GC. It's used in Apple's implementation of _NSConcretesStackBlock (and the one in GNUstep!), so that block allocated on the stack is copied to the heap if anyone retains it (in GC mode on OS X, you are required to make an explicit call to Block_copy(), with libobjc2 this is inserted by the write barrier automatically). >> but in GC mode, these two lines need to somehow become: >> >> objc_assign_strongCast(key, &(node->key));\ >> >> there also doesn't seem to be any macro for reading the keys. For example, >> when ever you read node->key or node->value as a weak pointer, lines like: >> >> GSI_MAP_EQUAL(map, node->key, key) >> >> Need to be expanded to something like: >> >> [objc_read_weak(&(node->key) isEqual: key] > > Then we need some modification for if/when we support non-boehm GC. > > The current sequence: > > node->key = key; > GSI_MAP_RETAIN_KEY(map, node->key); > > could become: > > GSI_MAP_ACQUIRE_KEY(map, &key_in_node, key); > > and we could trivially add a macro to reference the map content. That's more or less what I was thinking. David -- Sent from my Difference Engine ___ Gnustep-dev mailing list Gnustep-dev@gnu.org https://lists.gnu.org/mailman/listinfo/gnustep-dev
Re: GSIMap
On 10 Jun 2011, at 14:14, Richard Frith-Macdonald wrote: > > On 2 Jun 2011, at 10:40, David Chisnall wrote: > >> >> On 2 Jun 2011, at 06:28, Richard Frith-Macdonald wrote: >> >>> >>> On 1 Jun 2011, at 23:48, David Chisnall wrote: >>> This is actually wrong in retain / release mode (-retain is not guaranteed to return self), >>> >>> The guarantee is that it's specifically documented to do so in the protocol >>> see >>> http://developer.apple.com/library/mac/#documentation/Cocoa/Reference/Foundation/Protocols/NSObject_Protocol/Reference/NSObject.html >>> So any implementation of -retain which doesn't return self is faulty, and >>> the programmer really deserves any error they have introduced by >>> overriding/replacing the default one. >> >> My reading of that is that the requirement to return self is only a >> requirement for objects implementing their own reference counting. > > I guess that's a possible reading (though it certainly doesn't say that) ... > but hard to justify I think. The NSObject class adopts/conforms to the > NSObject protocol and Apple don't document an alternative behavior afaik, so > really it (and anything based on it) should do what the protocol says. It > seems to me that your reading assumes that documented behaviors are telling > you what to do when you override a method rather than telling you what a > method does which, while a possible point of view, is pretty much unique to > you I think. In my experience, whenever documentation is telling you what to > do when you override something, it makes a clear distinction between default > behavior and the overridden behavior. > >> The more formal description of -retain in the static analyser expects >> -retain to return an owning reference, but not necessarily to the same >> object as the receiver. > > I think you must have misread ... the clang static analyser agrees with me: > > c = [c retain]; > > produces the warning > > 'Assigned value is always the same as the existing value' In that case, someone should tell Apple: as I said in the original post, this contract is not honoured by all of their classes. Both Apple, and LanguageKit's closure implementations return a different object in response to -retain if the block is allocated on the stack. If someone tries to store a block in a dictionary, then GNUstep's implementation will store a pointer to the on-stack version, Apple's implementation will store a pointer to the heap version. Both will send a -retain message. The Apple version will work as developers expect, the GNUstep version is susceptible to trivial return-to-libc attacks. So, while you may be right in theory, that doesn't alter the fact that this is dangerous practice and should not be used in real code. David -- Sent from my IBM 1620 ___ Gnustep-dev mailing list Gnustep-dev@gnu.org https://lists.gnu.org/mailman/listinfo/gnustep-dev
Re: GSIMap
On 2 Jun 2011, at 10:40, David Chisnall wrote: > > On 2 Jun 2011, at 06:28, Richard Frith-Macdonald wrote: > >> >> On 1 Jun 2011, at 23:48, David Chisnall wrote: >> >>> >>> This is actually wrong in retain / release mode (-retain is not guaranteed >>> to return self), >> >> The guarantee is that it's specifically documented to do so in the protocol >> see >> http://developer.apple.com/library/mac/#documentation/Cocoa/Reference/Foundation/Protocols/NSObject_Protocol/Reference/NSObject.html >> So any implementation of -retain which doesn't return self is faulty, and >> the programmer really deserves any error they have introduced by >> overriding/replacing the default one. > > My reading of that is that the requirement to return self is only a > requirement for objects implementing their own reference counting. I guess that's a possible reading (though it certainly doesn't say that) ... but hard to justify I think. The NSObject class adopts/conforms to the NSObject protocol and Apple don't document an alternative behavior afaik, so really it (and anything based on it) should do what the protocol says. It seems to me that your reading assumes that documented behaviors are telling you what to do when you override a method rather than telling you what a method does which, while a possible point of view, is pretty much unique to you I think. In my experience, whenever documentation is telling you what to do when you override something, it makes a clear distinction between default behavior and the overridden behavior. > The more formal description of -retain in the static analyser expects -retain > to return an owning reference, but not necessarily to the same object as the > receiver. I think you must have misread ... the clang static analyser agrees with me: c = [c retain]; produces the warning 'Assigned value is always the same as the existing value' ___ Gnustep-dev mailing list Gnustep-dev@gnu.org https://lists.gnu.org/mailman/listinfo/gnustep-dev
Re: GSIMap
On Jun 10, 2011, at 15:23, David Chisnall wrote: > > In that case, someone should tell Apple: as I said in the original post, this > contract is not honoured by all of their classes. Both Apple, and > LanguageKit's closure implementations return a different object in response > to -retain if the block is allocated on the stack. Not true. Under OS X, retaining a block on the stack does nothing; -copy must be used. This is both the documented and observed behaviour. There are two reasons for this: 1) -retain is a no-op under garbage collection, but blocks still need to be copied to the heap; 2) it would violate the contract of -retain as per the NSObject protocol documentation. -- Jens Ayton ___ Gnustep-dev mailing list Gnustep-dev@gnu.org https://lists.gnu.org/mailman/listinfo/gnustep-dev
Re: GSIMap
On Jun 10, 2011, at 6:14 AM, Richard Frith-Macdonald wrote: > is pretty much unique to you I think I was also assuming that -retain can return a different object. Otherwise it should be void (aka AddRef()) ... hh ___ Gnustep-dev mailing list Gnustep-dev@gnu.org https://lists.gnu.org/mailman/listinfo/gnustep-dev
Re: GSIMap
Am 25.06.2011 um 09:56 schrieb Helge Hess: > On Jun 10, 2011, at 6:14 AM, Richard Frith-Macdonald wrote: >> is pretty much unique to you I think > > I was also assuming that -retain can return a different object. Otherwise it > should be void (aka AddRef()) ... > The NSObject Protocol documentation make it clear, that this is not so, and also gives the reason for returning self, - (id) retain Return Value self. Discussion ... As a convenience, retain returns self because it may be used in nested expressions. ... Ciao Nat! - Apple is the steam train that owns the tracks. - S. Jobs ___ Gnustep-dev mailing list Gnustep-dev@gnu.org https://lists.gnu.org/mailman/listinfo/gnustep-dev
Re: GSIMap
On 25 Jun 2011, at 08:56, Helge Hess wrote: > On Jun 10, 2011, at 6:14 AM, Richard Frith-Macdonald wrote: >> is pretty much unique to you I think > > I was also assuming that -retain can return a different object. Otherwise it > should be void (aka AddRef()) ... I'm all in favour of defensive programming like doing things like checking the result of -retain but ... See the Apple documentation at http://developer.apple.com/library/mac/#documentation/Cocoa/Reference/Foundation/Protocols/NSObject_Protocol/Reference/NSObject.html As you can see retain returns self, it does so as a convenience for use in nested expressions and it is specifically a no-op in gc environments (which means that the compiler/runtime is free to remove the call to -retain entirely if you are using gc). Short of saying something like 'this method really, *really*, REALLY never returns a different object from the receiver', I don't see how the documentation could be clearer, and I'm certain there is a lot of code written by a lot of people which depends on the documented behavior, so overriding -retain to do something else would be a fault/bug. The clang compiler even complains that you are assigning a value to itsself if you write 'x = [x retain]' So if you write code which overrides -retain to return a different object, the code is broken and you deserve what you get ... the documentation says its wrong, the compiler says it's wrong, and in a gc environment the retain method probably won't even be called. Anyway, this is my last post on this topic ... I think it's been done to death. ___ Gnustep-dev mailing list Gnustep-dev@gnu.org https://lists.gnu.org/mailman/listinfo/gnustep-dev
