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

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

> Is there any realistic step (other than doing nothing, and not  
> applying the patch) that would reassure you?

Yes.  Testing, obviously, and code reviews.

One problem I have with this patch is that it was not written as a "stage 4" patch; it contains
all sort of changes, regardless of whether they are relevant to adding support the new Apple ABI
or not.  Lots of things merit discussion too.

I assume we'll have to accept the large refactoring of all the code generation, as it can't be
stripped out of the patch, but can we agree that for everything else we're trying to make the minimal
changes required to support the new Apple ABI ?


> Index: gcc/c-family/c.opt
> [...]
> +fobjc-abi-version=
> +ObjC ObjC++ Joined Report RejectNegative UInteger Var(flag_objc_abi)
> +Specify which ABI to use in ObjC* code and meta-data generation.

We agreed to avoid "ObjC*"

Is this flag needed for GCC 4.6 ?  It does nothing for the GNU runtime (it is indeed
inconsistent with the -fobjc-nonfragile-abi used by clang to switch between different
ABI versions of the GNU runtime), and my understanding is that on Apple you control
the ABI version using -m32 / -m64.  Do we still need the flag for GCC 4.6 ?

I actually like the flag, but if we add it I'd like to use the actual ABI number used in the runtime
structures (eg, currently 8 for the GNU runtime), as opposed to 1 or 2.  This needs
discussion so if we don't strictly need the flag, we may as well have the discussion for 4.7, and
avoid the flag for now.


> Index: gcc/objc/objc-next-metadata-tags.h
> ===================================================================
> --- gcc/objc/objc-next-metadata-tags.h  (revision 0)
> +++ gcc/objc/objc-next-metadata-tags.h  (revision 0)
> [...]
>
> +/* Objective-C* supports several runtime library variants:
> +
> +   "GNU" runtime selected by -fgnu-runtime (currently at API version 1).
> +   "NeXT" runtime (selected by -fnext-runtime) and installed on OSX/Darwin
> +   systems at API version 1 (for m32 code) and version 2 (for m64 code).
> +
> +   The runtimes require different data types/layouts, method call mechanisms
> +   and so on, and the purpose of this interface is to abstract such
> +   differences from the parser's perspective.  */

Avoid "Objective-C*".

Why is this comment in this file ?  This is a file for the "next" runtime, but the
comment just describes generally some runtime variants and refers to some interface.
Shouldn't the comment go in objc-runtime-hooks.h ?

> +/* ObjC* meta data attribute tag */
> +#define objc_meta      objc_rt_trees[OCTI_RT_OBJC_META]

Avoid ObjC*


> Index: gcc/objc/objc-gnu-runtime-abi-01-private.h
> ===================================================================
> --- gcc/objc/objc-gnu-runtime-abi-01-private.h  (revision 0)
> +++ gcc/objc/objc-gnu-runtime-abi-01-private.h  (revision 0)

Is there any particular reason why this header file exists at all ?  It contains a number of #defines
that are only used in objc-gnu-runtime-abi-01.c.  Maybe they should be moved at the top of that file ?
Similarly for objc-next-runtime-abi-01-private.h and objc-next-runtime-abi-02-private.h.


> +#define OBJCMETA(DECL,VERS,KIND)                                       \
> +  if (VERS)                                                            \
> +    DECL_ATTRIBUTES (DECL) = build_tree_list ((VERS), (KIND));

This is new, and it's for the GNU runtime.  What is the purpose of it ?  A comment
is required.  Can it wait until 4.7 ?  No changes to the GNU runtime please.


> Index: gcc/objc/objc-runtime-shared-support.c
> ===================================================================
> --- gcc/objc/objc-runtime-shared-support.c      (revision 0)
> +++ gcc/objc/objc-runtime-shared-support.c      (revision 0)
> [...]
> +extern GTY(()) tree objc_rt_trees[OCTI_RT_META_MAX];
> +
> +/* Rather than repeatedly looking up the identifiers, we save them here.  */
> +tree objc_rt_trees[OCTI_RT_META_MAX];

Variable declared twice ... any reason ?


> +/* Utility routine to set global flags for a global, static declaration. */
> +
> +static void
> +objc_set_global_decl_fields (tree var)
> +{
> +  TREE_STATIC (var) = 1;
> +  DECL_INITIAL (var) = error_mark_node;  /* A real initializer is coming... */
> +  DECL_IGNORED_P (var) = 1;
> +  DECL_ARTIFICIAL (var) = 1;
> +  DECL_CONTEXT (var) = NULL_TREE;
> +#ifdef OBJCPLUS
> +  DECL_THIS_STATIC (var) = 1; /* squash redeclaration errors */
> +#endif
> +}
> +
> +/* Create a global, static declaration for variable NAME of a given TYPE.  The
> +   finish_var_decl() routine will need to be called on it afterwards.  */
> +
> +tree
> +start_var_decl (tree type, const char *name)
> +{
> +  tree var = build_decl (input_location,
> +                        VAR_DECL, get_identifier (name), type);
> +  objc_set_global_decl_fields (var);
> +  return var;
> +}

objc_set_global_decl_fields does not exist in the original code.  You added it but then
it's only called once, 10 lines below, where you copied the code from.  I'd revert to
the original code.


> +tree
> +add_objc_string (tree ident, string_section section)
> +{
> +  tree *chain, decl, type;
> +  char buf[256], ng = flag_next_runtime ? 'C':'G';
> +
> +  switch (section)
> +    {
> +    case class_names:
> +      chain = &class_names_chain;
> +      snprintf (buf, 256, "_OBJ%c_ClassName_%s", ng, IDENTIFIER_POINTER (ident));
> +      break;
> +    case meth_var_names:
> +      chain = &meth_var_names_chain;
> +      snprintf (buf, 256, "_OBJ%c_MethodOrVarName_%d", ng, meth_var_names_idx++);
> +      break;
> +    case meth_var_types:
> +      chain = &meth_var_types_chain;
> +      snprintf (buf, 256, "_OBJ%c_MethodOrVarType_%d", ng, meth_var_types_idx++);
> +      break;
> +    case prop_names_attr:
> +      chain = &prop_names_attr_chain;
> +      snprintf (buf, 256, "_OBJ%c_PropertyAttributeOrName_%d", ng,
> +               property_name_attr_idx++);
> +      break;
> +    default:
> +      gcc_unreachable ();
> +    }

This changes the symbols used with the GNU runtime from

 _OBJC_CLASS_NAME_12
 _OBJC_METH_VAR_NAME_33
 _OBJC_METH_VAR_TYPE_7

to

 _OBJG_ClassName_AClass
 _OBJG_MethodOrVarName_33
 _OBJG_MethodOrVarType_7

Any reason (suitable for stage 4) for this change ?  Are you fixing a regression for the GNU runtime ?
If you're just trying to make the names prettier (which is not for stage 4), I still wouldn't want "OBJG"
to be used, as it's not pretty. ;-)

By the way, class names longer than 240 characters will cause problems in your new code.  I'm not
sure what the maximum class name length is (or if there is one), but 240 doesn't sound that big.
At least you should be using BUFSIZE (ie, 1024) like everywhere else in gcc/objc/. ;-)


> +static tree
> +init_module_descriptor (tree type, long vers)
> +{
> [...]
> +  /* symtab = { ..., _OBJC_SYMBOLS, ... } */
> +
> +  ltyp = build_pointer_type (xref_tag (RECORD_TYPE,
> +                                      get_identifier (UTAG_SYMTAB)));
> +  if (UOBJC_SYMBOLS_decl)
> +    expr = convert (ltyp, build_unary_op (loc,
> +                          ADDR_EXPR, UOBJC_SYMBOLS_decl, 0));
> +  else
> +    expr = convert (ltyp, null_pointer_node);

The original code doesn't have the ltyp and convert here.  Can you explain (and maybe document)
why you added it ?


> +void
> +build_module_descriptor (long vers, tree attr)
> [...]
> 
> +  /* Create an instance of "_objc_module".  */
> +  UOBJC_MODULES_decl = start_var_decl (objc_module_template,
> +                       flag_next_runtime ? "_OBJC_Module" :  "_OBJG_Module");

This used to be _OBJC_MODULES for the GNU runtime.  Any reason why you're changing the GNU runtime symbols
everywhere, giving them such (unattractive) _OBJG_ names ?



> +/* Used by NeXT ABI=0..2 */
> +void
> +build_next_selector_translation_table (void)
> [...]
> +      if (warn_selector)

This used to be 

 if (warn_selector && objc_implementation_context)

I assume you know what you're doing and that -Wselector still works with the Apple runtime.


> Index: gcc/objc/Make-lang.in
> ===================================================================
> --- gcc/objc/Make-lang.in       (revision 170111)
> +++ gcc/objc/Make-lang.in       (working copy)
> [...]
>
> +START_HDRS = $(CONFIG_H) $(SYSTEM_H) coretypes.h $(TM_H) $(TREE_H) $(C_TREE_H) \
> +  c-lang.h langhooks.h c-family/c-objc.h objc/objc-act.h
>
> [...]
>
> -objc/objc-lang.o : objc/objc-lang.c \
> -   $(CONFIG_H) $(SYSTEM_H) coretypes.h $(TM_H) $(TREE_H) $(C_TREE_H) \
> -   $(GGC_H) langhooks.h $(LANGHOOKS_DEF_H) $(C_COMMON_H) gtype-objc.h \
> -   c-objc-common.h c-family/c-objc.h objc/objc-act.h
> +objc/objc-lang.o : objc/objc-lang.c $(START_HDRS) \
> +   $(GGC_H) $(LANGHOOKS_DEF_H) $(C_COMMON_H) gtype-objc.h \
> +   c-objc-common.h

If I read this correctly, you added a dependency of objc-lang.c to c-lang.h.  Is that
wanted ?


> Index: gcc/objc/objc-next-runtime-abi-01.c
> ===================================================================
> --- gcc/objc/objc-next-runtime-abi-01.c (revision 0)
> +++ gcc/objc/objc-next-runtime-abi-01.c (revision 0)
> [...]
>

It may be worth adding a comment at the top of the file explaining which ABIs
are implemented in this file, and what they mean in practice.  My understanding
is that this implements the NeXT ABI "0" (ie, the old 32-bit one) and "1" (ie, the old
32-bit one plus properties and protocol optional methods).

>
> +/* We need a way to convey what kind of meta-data are represented by a given
> +   variable, since each type is expected (by the runtime) to be found in a
> +   specific named section.  The solution must be usable with LTO.
> +
> +   The scheme used for NeXT ABI 0/1 (partial matching of variable names) is not
> +   satisfactory for LTO & ABI-2.  We now tag ObjC meta-data with identification
> +   attributes in the front end.  The back-end may choose to act on these as it
> +   requires.  */
> +
> +static void
> +next_runtime_abi_01_init_metadata_attributes (void)
> +{
> +  if (!objc_meta)
> +    objc_meta = get_identifier ("OBJC1META");
> +
> +  if (!meta_base)
> +    meta_base = get_identifier ("V1_BASE");
> [...]

This code is all new, yet it's for NeXT ABI v0/1.  I suppose you're fixing LTO with the
NeXT ABI v0/1 here ?


> +static void next_runtime_01_initialize (void)
> +{
> +  tree type;
> +
> +#ifdef OBJCPLUS
> +      /* For all objc ABIs -fobjc-call-cxx-cdtors is on by default. */
> +      if (!global_options_set.x_flag_objc_call_cxx_cdtors)
> +        global_options.x_flag_objc_call_cxx_cdtors = 1;
> +#endif

The comment needs updating; this is most likely "For all NeXT objc ABIs".


> +static tree
> +next_runtime_abi_01_class_decl (tree klass)
> +{
> +  tree decl;
> +  char buf[256];
> +  snprintf (buf, 256, "_OBJC_Class_%s",
> +           IDENTIFIER_POINTER (CLASS_NAME (klass)));
> +  decl = start_var_decl (objc_class_template, buf);
> +  OBJCMETA (decl, objc_meta, meta_class);
> +  return decl;
> +}

You are changing the symbol names here (for NeXT ABI 0 and 1, not the new one!).  Why ?

Are you changing them to follow changes in the new Apple compilers (eg, clang) ?
If not, I recommend you stick to using the same symbols as the Apple compilers emit.  That
guarantees that our compiler is more likely to be compatible with whatever Apple does in the
future.  You just want to produce identical object files, so they'll always work identically.

The same applies to next_runtime_abi_01_metaclass_decl(), next_runtime_abi_01_category_decl(),
next_runtime_abi_01_protocol_decl() and build_class_reference_decl().

(running out of time, I'll have to skip the other few thousand lines of NeXT runtime support
code)


> Index: gcc/objc/objc-act.c
> ===================================================================
> --- gcc/objc/objc-act.c (revision 170111)
> +++ gcc/objc/objc-act.c (working copy)

> +#include "langhooks.h"
> +#include "c-family/c-objc.h"
> +#include "objc-act.h"
>
>  #include "c-family/c-common.h"
> -#include "c-family/c-objc.h"
>  #include "c-family/c-pragma.h"
>  #include "c-family/c-format.h"
>  #include "flags.h"
> -#include "langhooks.h"
> -#include "objc-act.h"
>  #include "input.h"

Why moving the headers around ?


> -#define OBJC_VOID_AT_END       void_list_node

Why was this #define removed ?  By the way, I don't mind removing it, I just don't
see the point of sprinkling random changes around at the end of stage 4.


> -/* (Decide if these can ever be validly changed.) */
> -#define OBJC_ENCODE_INLINE_DEFS        0
> -#define OBJC_ENCODE_DONT_INLINE_DEFS   1

Any reason why these #defines were removed ?


> -struct imp_entry *imp_list = 0;
> +imp_entry_t *imp_list = 0;

This is due to the renaming of "struct imp_entry" to "imp_entry_t".  Any reason to do
this ?


> bool
> objc_init (void)
> {
> [...]

Summarizing, here you changed the code to call the runtime-specific initialization functions.  Ok.

But then, you also changed the code to run the generate_struct_value_by_array() before the runtime
initialization instead of afterwards.

And, you moved the flag_gen_declarations implementation from objc_init () to objc_write_global_declarations ().
You also skip finish_objc() (and the flag_gen_declarations) if flag_syntax_only or pch_file.

If these have to go in, can we at least clarify the logic there ?  Are we trying to fix some bugs ?
Maybe add a few lines of comments about why things are that way ?


> -static tree
> +tree
> get_objc_string_decl (tree ident, enum string_section section)
> {
> [...]
>
> -  gcc_unreachable ();
> +  /* We didn't find the entry.  */
>   return NULL_TREE;
> }

I guess the gcc_unreachable is no longer unreachable ?  In what condition would it be reached ?


> @@ -5407,6 +3841,7 @@ void
> objc_begin_catch_clause (tree decl)
> {
>   tree compound, type, t;
> +  bool ellipsis = false;
> [...]
> +        /* ... but allow the runtime to differentiate between ellipsis and the
> +           case of @catch (id xyz).  */
> +        ellipsis = true;

So here you are trying to have the compiler compile @catch(...) and @catch(id x) differently.
My understanding is that that only matters if you are in ObjC++ and can mix ObjC and C++
exceptions.  Presumably on Apple 64-bit that works for you ?  Is that why we're making this
change ?

> -  else if (objc_is_object_id (TREE_TYPE (type)))
> -    type = NULL;
> +  else if (POINTER_TYPE_P (type) && objc_is_object_id (TREE_TYPE (type)))
> +    /* @catch (id xyz) or @catch (...) but we note this for runtimes that
> +       identify 'id'.  */
> +    ;

I'm not sure I understand this change.  You've already recorded if it's a @catch(...) as opposed
to a @catch(id x) in the "ellipsis" variable, and that will be passed to the runtime.  Presumably
the rest of the flow could be identical (and we don't have to worry about breaking something) ?

But it isn't.  The POINTER_TYPE_P(type) change does not seem that it would do anything (any reason
you added it ?), but the change from "type = NULL;" to ";" means that "@catch (id x)" is now
compiled differently.  We used to generate a NULL type for the catch, with the GNU runtime
interprets as a catchall, while now we generate something else.  Or, do we ?  I'm confused.
If we generate something else, exactly, what do we generate ?

I'm at line 6,441 of your patch - halfway - now I'm afraid have to go.  I'll pick it up later.

---

All I can say is that if this patch is so important for you Apple guys, let's apply it.  But I want
to discuss the details.  Unfortunately, if the time is short and we want to ensure the quality of
the result, the only things we can do it look at the code and try to spot problems, and test it.

I also understand that by requesting too many changes we may actually end up with worse, less tested
software.  But I still can't think of anything better than looking at the code and discussing it.

Mike suggested privately to try compile some serious projects with the GNU runtime and compare
the output.  That is a great, helpful idea.  I'll see if I can arrange to do that during the weekend.

Anyway, I'd appreciate if other people were to look at the code too.

Thanks