Re: [Patch ObjC/Darwin 1/3] Fix wrong code gen for m64 Objective-C

["Nicola Pero" <nicola dot pero at meta-innovation dot com>]

>> Hmmm.  Ok, I had not read objc_eh_runtime_type() yet.  I see you  
>> changed
>> things there.  It used to just emit the class name, it now tries to  
>> look
>> it up in the class names that have already been emitted, presumably to
>> unique class names emitted in the object file.
>>
>> I hope that doesn't break anything.
>
> ummm...
> exceptions don't work for GNU Objective-C++

Sure - I was thinking of GNU Objective-C here.  Isn't objc_eh_runtime_type()
used for exceptions with GNU Objective-C ?


> @@ -5847,122 +4117,15 @@ build_private_template (tree klass)
>       /* Copy the attributes from the class to the type.  */
>       if (TREE_DEPRECATED (klass))
>        TREE_DEPRECATED (record) = 1;
> +      if (CLASS_HAS_EXCEPTION_ATTR (klass))
> +       CLASS_HAS_EXCEPTION_ATTR (record) = 1;

Here, and in following code, you're adding support for the 'objc_exception'
attribute.  Is it worth adding a comment explaining what it does ?

Maybe in start_class later, where the attributes are recognized ?


> @@ -10756,6 +7476,10 @@ objc_gen_property_data (tree klass, tree class_met
>        /* @dynamic property - nothing to check or synthesize.  */
>        if (PROPERTY_DYNAMIC (x))
>         continue;
> +  /* Add any property that is declared in the interface, but undeclared in the
> +     implementation to thie implementation. These are the 'dynamic' properties.
> +
> +  objc_v2_merge_dynamic_property ();*/

This comment is clearly merged from the Apple branch; I guess you added it as
you were wondering if we have a problem in our code there.  We don't.  You can
remove the comment ;-)

In our implementation, when we find a @dynamic, we immediately create an IMPL_PROPERTY_DECL,
mark it as @dynamic, and put it in the chain of properties for the @implementation.  Apple
doesn't do this.  They just mark the original property in the @interface as @dynamic.  So
they later have an objc_v2_merge_dynamic_property() step which iterates over all properties
in the @implementation, spots the @dynamic ones, and adds them to the IMPL_PROPERTY_DECL.
We don't need that step because we create the PROPERTY_DECL when the @dynamic expressions
are parsed.


>           PROTOCOL_FORWARD_DECL (protocol) = NULL_TREE;
> +/*       PROTOCOL_V2_FORWARD_DECL (protocol) = NULL_TREE;*/

This seems also coming from the Apple branch too, and it's to do with code generation for m64
Apple.  Is there anything missing in our implementation ?  If so, maybe we could add a FIXME/TODO ?
Else, we can remove the comment ?


> -^L
> -/* "Encode" a data type into a string, which grows in util_obstack.
> -
> -   The format is described in gcc/doc/objc.texi, section 'Type
> -   encoding'.

Here, as already discussed, you moved a large chunk of code to another part of the file.

I assume you didn't modify it; it's hard to check.

Is it worth committing that change separately, so when people look at the subversion history
they clearly see separated commits for the move-the-code-without-changing-it change, and
the actual code changes ?


> +static bool
> +objc_type_valid_for_messaging (tree type, bool accept_classes)

This was also moved around.  Is it worth committing this separately too, maybe preliminary
to the big patch with the actual code changes ?


> @@ -12279,80 +8377,42 @@ get_super_receiver (void)
> [...]
> +      gcc_assert (imp_list->imp_context == objc_implementation_context
> +                 && imp_list->imp_template == implementation_template);
> [...]
> -         tree super_name = CLASS_SUPER_NAME (implementation_template);
> +         tree super_name = CLASS_SUPER_NAME (imp_list->imp_template);
> [...]
> -                    CLASS_NAME (implementation_template));
> +                    CLASS_NAME (imp_list->imp_template));

I wonder what the reason is for using imp_list instead of objc_implementation_context
and implementation_template here.

They are all global variables, so no advantage there.  But imp_list is a data structure
to track all classes and categories compiled in the module, and you're relying on the
fact that imp_list itself points to the head of the linked list, and hence it is also
the latest class/category found, ie the one currently being compiled.  But what if we
want to change the data structure ?  The fact that imp_list is the current class/category
being compiled seems to be almost an accident.  We could decide to put the list of
classes in reverse order, or use a hashtable.

It would seem safer to use objc_implementation_context and implementation_template, which
are designed precisely to point to the class/category currently being compiled ?


> @@ -12886,93 +8928,6 @@ finish_objc (void)
> [...]
> +  /* Compute and emit the meta-data tables for this runtime.  */
> +  (*runtime.generate_metadata) ();

Looking at the runtime hooks, there is lot of duplication between the different ones
in the implementation of this hook.  So maybe (in 4.7?) we could restructure the code.

In particular, maybe the dump_interface() code should be there, as it's not runtime
dependent.

So, in finish_objc() we could have

if (flag_gen_declaration)
  for (impent = imp_list; impent; impent = impent->next)
    dump_interface (gen_declaration_file, impent->imp_context);

and then the various runtimes wouldn't have to worry about dump_interface().  And dump_interface()
could be static again.


> +/* This routine encodes the attribute of the input PROPERTY according to following
> +   formula:
> +
> +Property attributes are stored as a comma-delimited C string. The simple attributes
> +readonly and copies are encoded as single characters. The parametrized attributes,
> +getter=name, setter=name, and ivar=name, are encoded as single characters, followed
> +by an identifier. Property types are also encoded as a parametrized attribute. The
> +characters used to encode these attributes are defined by the following enumeration:
> +
> +enum PropertyAttributes {
> +    kPropertyReadOnly = 'r',                    // property is read-only.
> +    kPropertyCopies = 'c',                      // property is a copy of the value last assigned
> +    kPropertyGetter = 'g',                      // followed by getter selector name
> +    kPropertySetter = 's',                      // followed by setter selector name
> +    kPropertyInstanceVariable = 'i'            // followed by instance variable  name
> +    kPropertyType = 't'                         // followed by old-style type encoding.
> +};

This looks good (it comes from the Apple branch).  Maybe you could reindent the comment
to match the GCC coding style though. :-)


> +  obstack_1grow (&util_obstack, 't');
> [...]
> +  if (PROPERTY_READONLY (property))
> +    obstack_grow (&util_obstack, ",r", 2);

I think newer Apple compilers use uppercase letters ("T", ",R", etc) so you may want to match
what they do.

As you're doing this, it may be worth also adding ",&" for retain, ",D" for dynamic, and ",N"
for non-atomic.


> Index: gcc/objc/objc-next-runtime-abi-02.c
> ===================================================================
> --- gcc/objc/objc-next-runtime-abi-02.c (revision 0)
> +++ gcc/objc/objc-next-runtime-abi-02.c (revision 0)
> [...]
> +/* The NeXT ABI2 is used for m64 implementations on Darwin/OSX machines.
> +
> +   This version is intended to match (logically) output of Apple's 4.2.1
> +   compiler.
> +
> +   References:
> +   FSF GCC branches/apple/trunk.
> +   objc4-371.2 Open Source release (Apple Computer). (objc-runtime-new.h)
> +   gcc_42-5664 Apple Local modifications to GCC (Apple Computer).
> +*/

I'd suggest removing the references as they'll be obsoleted quickly.  Free software
code ends up being around for a very long time; you're writing a comment that may easily
be around for 10 or 20 years, and it's unlikely that someone will bother keeping the
references up to date. ;-)

<Skipping the rest of the NeXT runtime code>


> Index: gcc/objc/objc-runtime-hooks.h
> ===================================================================
> --- gcc/objc/objc-runtime-hooks.h       (revision 0)
> +++ gcc/objc/objc-runtime-hooks.h       (revision 0)
> [...]
> +   "GNU" runtime selected by -fgnu-runtime (currently at API version 1).

I'd suggest saying "currently at ABI version 8"


> +typedef struct _objc_runtime_hooks_r
> +{

Do we want the initial underscore ?  I have the impression that GCC code
never uses it.


> +  /* Get an ivar ref. re the base.  */
> +  tree (*build_ivar_reference) (location_t, tree, tree);

It may be worth reviewing the comments in this file, and make them more readable
and explanatory. :-)


> +/* One per runtime at present.
> +   TODO: Make into some kind of configury-generated table.  */
> +extern bool objc_gnu_runtime_abi_01_init (objc_runtime_hooks *);
> +extern bool objc_next_runtime_abi_01_init (objc_runtime_hooks *);
> +extern bool objc_next_runtime_abi_02_init (objc_runtime_hooks *);

Hmm.  Not sure the suggestion in the TODO is the right approach,
but I agree we may want to do better ... let's discuss another time.


> Index: gcc/objc/objc-act.h
> ===================================================================
> --- gcc/objc/objc-act.h (revision 170210)
> +++ gcc/objc/objc-act.h (working copy)
> [...]
> +#define CLASS_HAS_EXCEPTION_ATTR(CLASS) ((CLASS)->type.lang_flag_0)

I think you want to use TYPE_LANG_FLAG_0 here, which is the same but
does the additional checks when building in checking mode.

Unfortunately TYPE_LANG_FLAG_0 seems to be also used by C though, and so
are TYPE_LANG_FLAG_1 and TYPE_LANG_FLAG_2.

And all TYPE_LANG_FLAG_x seems to be used by C++, with the exception of
TYPE_LANG_FLAG_2.

Not sure what you could do.  If we assume that Objective-C is the priority,
then we want no conflicts with C and we want to pick one between TYPE_LANG_FLAG_3
and TYPE_LANG_FLAG_6.

I'm not sure what to suggest.  Maybe you could store the 'objc_exception' attribute
in some other way ?  As a standard attribute maybe ?  Is the CLASS_HAS_EXCEPTION_ATTR
an optimization ?

In 4.7, I'd really like to understand if there is a way we can sort the "language flags"
out properly and have ObjC have its own.


> @@ -394,6 +396,10 @@ enum objc_tree_index
>      OCTI_GET_PROPERTY_STRUCT_DECL,
>      OCTI_SET_PROPERTY_STRUCT_DECL,
> 
> +    /* "V1" stuff.  */
> +    OCTI_V1_PROP_LIST_TEMPL,
> +    OCTI_V1_PROP_NAME_ATTR_CHAIN,
> +

Can you expand the comment ?  A sentence would be good.  :-)

> +/* V1 stuff.  */
> +#define objc_prop_list_ptr     objc_global_trees[OCTI_V1_PROP_LIST_TEMPL]
> +#define prop_names_attr_chain  objc_global_trees[OCTI_V1_PROP_NAME_ATTR_CHAIN]

Same.


> Index: gcc/objc/objc-gnu-runtime-abi-01.c
> ===================================================================
> --- gcc/objc/objc-gnu-runtime-abi-01.c  (revision 0)
> +++ gcc/objc/objc-gnu-runtime-abi-01.c  (revision 0)
>
> [...]
>
> +#ifndef TARGET_64BIT
> +#define TARGET_64BIT 0
> +#endif

Hmmm.  It seems a bit vague for describing the target. ;-)

It seems to later be used to add some padding to structures.  Are you sure
this padding would match the one used by the GNU runtime on all platforms ?
My understanding is that 64-bit doesn't tell you much if you want to know
what padding is required in structures; you'd need to know the size/alignment
of the fields used in the structure (long, void *, etc).

At the moment the runtime doesn't have any structure padding, yet the compiler
automatically adds them as appropriate for that platform.  The corresponding
structure types created by the compiler to generate the metadata also don't
have any explicit padding, but again the compiler will pad them as appropriate
for the platform, in the same way, resulting in the same padding used by compiler
and runtime, so we're fine. :-)

If you add padding explicitly, we need to make sure it is always correct (or that
the runtime has exactly the same explicit padding).  Can I suggest we don't make
any changes for GCC 4.6 ?  Maybe you could comment or #ifdef out your 64-bit padding
changes, and we'll look at them for 4.7 ?

Unless ... were you trying to fix a bug reported by someone ?


> +static tree
> +gnu_runtime_abi_01_super_superclassfield_id (void)
> +{
> +  if (!super_superclassfield_id)
> +    super_superclassfield_id = get_identifier ("super_class");
> +  return super_superclassfield_id;
> +}

I see you've tried to optimize things here by lazily creating these identifiers. :-)

If you want my personal opinion, I would create them all at startup and not bother
with the lazy creation and the caching.  Once upon a time, you could have argued that
you were saving the get_identifier call, and the cost of the conditional check was
small.  But branching is slow on modern CPUs; meaning the cost of the conditional check
is not necessarily as tiny as it used to be.  That means you're not sure if the
optimization speeds things up or slows them down, and as the maximum benefit you
can get is saving one get_identifier call per compiled unit (which is impossibly
tiny to measure), why bother with the complication ? ;-)

Anyhow, not an urgent change obviously; you can leave the code as it is for now.


> +  /* This is the type of all of the following functions
> +     bjc_getPropertyStruct() and objc_setPropertyStruct().  */

objc_getPropertyStruct()


> +static tree
> +gnu_runtime_abi_01_get_category_super_ref (location_t loc ATTRIBUTE_UNUSED,
> +                                          struct imp_entry *imp, bool inst_meth)
> +{
> +  tree super_name = CLASS_SUPER_NAME (imp->imp_template);
> +  tree super_class;
> +
> +  add_class_reference (super_name);
> +  super_class = (inst_meth ? objc_get_class_decl : objc_get_meta_class_decl);
> +/* assemble_external (super_class);*/

This assemble_external (super_class); was uncommented in the original code.  Any
reason to comment it out ?


Ok - I ran out of time again.  I still have to look at a few thousand lines at least.
I'll try to do it tomorrow morning (GMT).

Thanks