[Bug libobjc/38307] Calling of the +initialize method is not completely thread-safe

["ayers at gcc dot gnu dot org" <gcc-bugzilla at gcc dot gnu dot org>]

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