This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Add plugin interface to LD [1/4] Infrastructure.


On 09/22/2010 10:31 PM, Dave Korn wrote:
> - I added a --enable-plugins command to ld's configure before I remembered
> that there's one in the binutils/ subdir (for nm and ar) as well.  Should I
> perhaps change it to --enable-ld-plugins (or something else) so that it can be
> controlled separately?  Should I rewrite it to use config/plugins.m4?

I honestly don't have any idea.  I didn't realize that we had a plugin
interface to bfd.  I almost suggested renaming the existing option to
--enable-bfd-plugins, but I don't see that actually reducing the 
confusion at all.

Personally I think the least amount of confusion would be generated
by having the ld plugin be non-optional.  That way for some minimal
binutils version gcc can simply assume its availability.

> - Is it OK to put an unguarded "#include <stdarg.h>" in ldmisc.h?

We assume C90, so technically ok, but why do you want it in
that header file?  Probably sysdep.h would be better for
consistency.

[ Ignoring most of the configury and makefile hackery ]

> +/* Alias to shorten static function prototype lines.  */
> +#define PLUGAPIFUNC static enum ld_plugin_status

That just seems odd.  I'd prefer formatting like

static enum ld_plugin_status message
  (int level, const char *format, ...);

and I'd prefer even more if those declarations weren't needed.
I.e. move the uses of those functions to the end of the file.

> +  /* Allocate tv array and initialise constant part.  */
> +  my_tv = xmalloc ((max_args + 1 + set_tv_header (NULL)) * sizeof *my_tv);
> +  set_tv_header (my_tv);

I think this interface to set_tv_header is unnecessarily ugly.
Move tv_header_tags and tv_header_size out of set_tv_header
and you no longer need those silly NULL calls.

> +  newplug->dlhandle = dlopen (plugin, RTLD_NOW);

While I like avoiding libtool as much as possible, don't we
have to use lt_dlopen on some targets?  I suppose cygwin
already does the appropriate mapping to LoadLibrary...

> +/* Always use this macro when invoking a plugin function.  */
> +#define INVOKE_PLUGIN_FN(plugin, retval, fn, args)	\
> +	called_plugin = plugin;				\
> +	retval = (*fn) args;				\
> +	called_plugin = NULL;

While this does manage called_plugin effectively, this makes
the actual invocations unnecessarily ugly.  Do you expect
more code to go here, or couldn't we just expand this in
place in the 4 places its used?

> +      {
> +	char *newfmt = xmalloc (strlen (format) + 3);
> +	newfmt[0] = '%';
> +	newfmt[1] = (level == LDPL_FATAL) ? 'F' : 'X';
> +	strcpy (&newfmt[2], format);
> +	vfinfo (stderr, newfmt, args, TRUE);
> +      }

Probably better as

  newfmt = concat ((level == LDPL_FATAL ? "%F" : "%X"),
                   format, NULL);

Also missing a free.


r~


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]