This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] Add plugin interface to LD [1/4] Infrastructure.
- From: Richard Henderson <rth at redhat dot com>
- To: Dave Korn <dave dot korn dot cygwin at gmail dot com>
- Cc: "binutils at sourceware dot org" <binutils at sourceware dot org>
- Date: Wed, 06 Oct 2010 13:57:44 -0700
- Subject: Re: [PATCH] Add plugin interface to LD [1/4] Infrastructure.
- References: <4C9AE5CA.80707@gmail.com> <4C9AE625.8030904@gmail.com>
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~