------- Comment #3 from ayers at gcc dot gnu dot org 2009-04-10 12:43 ------- Created an attachment (id=17613) --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=17613&action=view) rewrite of dispatch table installation
I agree with the approach you describe, in that we need a look-a-side buffer for the dispatch table to send messages during +initialize and install the dtable after +initialize returns. I was not comfortable with the patch because: /* Assumes that __objc_runtime_mutex is locked down. * If receiver is not null, it is the object whos supercalss must be * initialised if the superclass of the one we are installing the * dispatch table for is not already installed. */ __objc_begin_install_dispatch_table_for_class (Class class, Class receiver) I still have a hard time groking what was intended with the receiver. It all seems very intertwined and I think there is a more straight forward way to implement this. Also, with this patch get_imp fails on class methods. (get_imp also has the nasty effect of installing the dispatch table without calling +initialize and the same goes for __objc_responds_to). I'm not to fond of introducing InitializingList as special type. I think should be fine with using the existing hash map tables for this. I don't think we really need to introduce a new type. Do you really think that method dispatch for partially installed dispatch tables is performance critical? Well... after all this complaining, let's get to something more constructive :-). I've attached a patch (including some test cases which still need to be augmented) that I'd like to propose for a reimplementation originally based on your code. I hope I've added enough comments and asserts to insure the assumptions and prerequisites are met. For the final submission I'll remove some of the asserts. It combines __objc_install_dispatch_table_for_class and __objc_init_install_dtable into: /* This function is called by: objc_msg_lookup, get_imp and __objc_responds_to (and the dispatch table installation functions themselves) to install a dispatch table for a class. If CLS is a class, it installs instance methods. If CLS is a meta class, it installs class methods. In either case +initialize is invoked for the corresponding class. The implementation must insure that the dispatch table is not installed until +initialize completes. Otherwise it opens a potential race since the installation of the dispatch table is used as gate in regular method dispatch and we need to guarantee that +initialize is the first method invoked an that no other thread my dispatch messages to the class before +initialize completes. */ static void __objc_install_dtable_for_class (Class cls) Which implements your suggestion with the following helper functions: static void __objc_prepare_dtable_for_class (Class cls); - builds the dispatch table and stores it in a look-a-side buffer (I used the hash tables instead of a custom type). static struct sarray *__objc_prepared_dtable_for_class (Class cls); - access the prepared table: this is used to identify whether the dispatch table is currently being installed (akin to the __objc_is_initializing_dispatch_table of the proposed patch) and is also used for subclasses that may be +initialized during the +initialize of the super class (i.e. class clusters when NSString's +initialize invokes GSString methods an need to copy NSString's dtable. static void __objc_install_prepared_dtable_for_class (Class cls); - static IMP __objc_get_prepared_imp (Class cls,SEL sel); Could you please have a look and let me know what you think? I'm still going to write some more test, checking the class cluster behavior mentioned above and I'll need some tests wrt to categories. So this is not final but it should address the main issue and the get_imp/__objc_responds_to issue. Cheers, David -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38307
