Re: [plugin/objc] pluginize objc mangling...

[Diego Novillo <dnovillo at google dot com>]

On Sat, Aug 22, 2009 at 11:03, Matt Rice <ratmice@gmail.com> wrote:
> in objc-act.c there is this comment here which isn't visible in my patch fuzz..
>
> /* This is the default way of generating a method name. Â*/
> /* I am not sure it is really correct.
> Â Perhaps there's a danger that it will make name conflicts
> Â if method names contain underscores. -- rms. Â*/
> #ifndef OBJC_GEN_METHOD_LABEL
> .....
>
>
> this will happen, in the case of
>
> @interface Foo
> - (void) bar_baz_;
> - (void) bar:(id)a baz:(id)b;
> @end
>
> this will create 2 symbols
> _i_Foo__bar_baz
>
> @interface Foo_Bar
> - (void) foo;
> @end
>
> @interface Foo(Bar)
> - (void) foo;
> @end
>
> will create
> _i_Foo_Bar_foo;
>
> both of these are manufactured... not really appearing in the wild afaik
> one real annoyance though is that because of the conversion of ':' to
> _ and the allowance of _'s in method names, these mangled symbols
> cannot be demangled into a more human readable fashion.
>
> the existing OBJC_GEN_METHOD_LABEL allows for targets in
> gcc/gcc/config/* to define their own name mangling macro,
>
> the attached patch attempts to extend that macro for use by plugins.
> and i've implemented a plugin which replaces the underscores with dots
> which the gnu assembler accepts and doesn't suffer from the issues
> that default OBJC_GEN_METHOD_LABEL macro does.
>
> (i'm kinda assuming that there is some assembler out there in use
> which does not accept dots in symbol names which gcc can also use)
> does this assumption hold, or is compatibility the only thing holding
> us back from fixing these?
>
> none the less, changing it in gcc would require us to patch anything
> which understands the existing mangling scheme (gdb and oprofile at
> the very least, i believe these could be made to work with 2 mangling
> schemes at the expense of potentially mangling twice if we pick the
> wrong scheme.)
>
> Âso i guess the patch allows me to get what i want without having to
> maintain a platform in gcc/config or breaking backwards compatibility
> for everybody else...
>
> at the very least i can use this to assess the extent of the damage,
> whether it is applied or not... let me know what you guys think.

Well, this is interesting.  There are some performance
implications that I think are minimal in this patch, but if we
start converting target macros into callbacks, it may be
noticeable.

I am OK with this patch, but I would like to get the opinion from
Mike and Stan.  Do you folks mind if OBJC_GEN_METHOD_LABEL gets
converted into a function pointer overridable by a plugin?

Some minor formatting issues below.

Thanks.  Diego.

> Index: gcc/objc/objc-plugin.h
> ===================================================================
> --- gcc/objc/objc-plugin.h	(revision 0)
> +++ gcc/objc/objc-plugin.h	(revision 0)
> @@ -0,0 +1,10 @@
> +#ifndef GCC_OBJC_PLUGIN_H
> +#define GCC_OBJC_PLUGIN_H
> +
> +typedef void (*objc_mangle_func)(char **buf, int is_instance, const char *class_name,
> +                           const char *category_name, const char *selector_name,
> +                           int method_slot);
> +
> +extern objc_mangle_func objc_gen_method_label;
> +
> +#endif
> Index: gcc/objc/Make-lang.in
> ===================================================================
> --- gcc/objc/Make-lang.in	(revision 151010)
> +++ gcc/objc/Make-lang.in	(working copy)
> @@ -36,6 +36,8 @@
>  # - the compiler proper (eg: cc1plus)
>  # - define the names for selecting the language in LANGUAGES.
>
> +OBJC_PLUGIN_HEADERS := objc-plugin.h
> +
>  #
>  # Define the names for selecting Objective-C in LANGUAGES.
>  objc: cc1obj$(exeext)
> @@ -79,7 +81,7 @@
>     $(EXPR_H) $(TARGET_H) $(C_TREE_H) $(DIAGNOSTIC_H) toplev.h $(FLAGS_H) \
>     objc/objc-act.h input.h $(FUNCTION_H) output.h debug.h langhooks.h \
>     $(LANGHOOKS_DEF_H) $(HASHTAB_H) $(C_PRAGMA_H) gt-objc-objc-act.h \
> -   $(GIMPLE_H) c-lang.h
> +   $(GIMPLE_H) c-lang.h objc/objc-plugin.h
>
>  objc.srcextra:
>
> @@ -98,7 +100,18 @@
>  objc.man:
>  objc.srcinfo:
>  objc.srcman:
> -objc.install-plugin:
> +objc.install-plugin: installdirs
> +# We keep the directory structure for files in config and .def files. All
> +# other files are flattened to a single directory.
> +	headers="$(OBJC_PLUGIN_HEADERS)"; \
> +	for file in $$headers; do \
> +	  path=$(srcdir)/objc/$$file; \
> +	  dest=$(plugin_includedir)/objc/$$file; \
> +	  echo $(INSTALL_DATA) $$path $(DESTDIR)$$dest; \
> +	  dir=`dirname $$dest`; \
> +	  $(mkinstalldirs) $(DESTDIR)$$dir; \
> +	  $(INSTALL_DATA) $$path $(DESTDIR)$$dest; \
> +	done
>
>  objc.tags: force
>  	cd $(srcdir)/objc; etags -o TAGS.sub *.c *.h; \
> Index: gcc/objc/objc-act.c
> ===================================================================
> --- gcc/objc/objc-act.c	(revision 151010)
> +++ gcc/objc/objc-act.c	(working copy)
> @@ -60,6 +60,7 @@
>  #include "flags.h"
>  #include "langhooks.h"
>  #include "objc-act.h"
> +#include "objc-plugin.h"
>  #include "input.h"
>  #include "except.h"
>  #include "function.h"
> @@ -111,6 +112,17 @@
>    } while (0)
>  #endif
>
> +static void default_objc_gen_method_label(char **buf, int is_instance,
> +			   const char *class_name,
> +			   const char *category_name, const char *selector_name,
> +			   int method_slot __attribute__((unused)))
> +{
> +  OBJC_GEN_METHOD_LABEL(*buf, is_instance, class_name, category_name,
> +			selector_name, method_slot);

Space before '('.

> +}
> +
> +objc_mangle_func objc_gen_method_label = default_objc_gen_method_label;
> +
>  /* These need specifying.  */
>  #ifndef OBJC_FORWARDING_STACK_OFFSET
>  #define OBJC_FORWARDING_STACK_OFFSET 0
> @@ -8691,8 +8703,8 @@
>    buf = (char *) alloca (50 + strlen (sel_name) + strlen (class_name)
>  			 + (cat_name ? strlen (cat_name) : 0));
>
> -  OBJC_GEN_METHOD_LABEL (buf, TREE_CODE (method) == INSTANCE_METHOD_DECL,
> -			 class_name, cat_name, sel_name, method_slot);
> +  (*objc_gen_method_label)(&buf, TREE_CODE(method) == INSTANCE_METHOD_DECL,
> +			class_name, cat_name, sel_name, method_slot);

Likewise.

>
>    method_id = get_identifier (buf);
>
> Index: gcc/objc/ChangeLog
> ===================================================================
> --- gcc/objc/ChangeLog	(revision 151010)
> +++ gcc/objc/ChangeLog	(working copy)
> @@ -1,3 +1,18 @@
> +2009-08-22  Matt Rice  <ratmice@gmail.com>
> +
> +	* objc-plugin.h: New file, declare objc_mangle_func type and
> +	objc_gen_method_label function pointer.
> +	* objc-act.c: Include objc-plugin.h
> +	(default_objc_gen_method_label): New function wrapping
> +	OBJC_GEN_METHOD_LABEL macro.
> +	(objc_gen_method_label): Default to default_objc_gen_method_label.
> +	(really_start_method): Call objc_gen_method_label instead of
> +	OBJC_GEN_METHOD_LABEL.
> +	* Make-lang.in (objc.install-plugin): Implement plugin header
> +	installation.
> +	(OBJC_PLUGIN_HEADERS): Add objc-plugin.h
> +	(objc/objc-act.o): Add objc/objc-plugin.h dependency.
> +
>  2009-08-20  Richard Guenther  <rguenther@suse.de>
>
>  	* objc-act.c: Include c-lang.h