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 06/10/2010 21:57, Richard Henderson wrote:
> 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 wasn't aware of it either until I started on this, but looking back I
see we've had it for quite some time!

> 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.

  I was thinking mostly in terms of having the ability to disable it as a
get-out-of-jail-free card if it broke anything.  I'm pretty confident that
none of the regular code paths will be affected if the user simply doesn't use
the -plugin option, but some platforms might not have dlopen, so I'll have to
add some AC_FUNC/AC_HEADERS tests for those, and then I'll still need some way
of disabling the plugin code from building on platforms that don't support the
dl-* interfaces, so since I'm going to have all the mechanism anyway, why not
provide command-line control over it?

  I could leave the option there but default it to on for platforms where the
dl-* headers and funcs are found, or I could remove the command-line option
altogether and make it depend only on the presence of the libs and headers;
please let me know which you'd prefer.

>> - 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.

  I just put it there because that was what depended on it.  I'll move it to
sysdep.h as you suggest.

>> +/* 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.

  OK, will do.

>> +  /* 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.

  OK, will do.

>> +  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...

  I copied what both GOLD and BFD do here.  As far as I know, the main thing
that using libltdl would give us is dlpreopening, which in theory would allow
linking plugins statically into ld on platforms that don't support DSOs; but
IIUC, that would require the plugin to be built as part of the ld build and
modifications to the ld makefile to link it against the plugin, which I don't
think is going to be a significant use case; I think most plugins will be
built as part of separate projects, like GCC's lto-plugin.

  So, I'd rather just punt on that.  We can add it later if it turns out to
actually be needed for anything?

>> +/* 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?

  Well, I just didn't like cut'n'pasting the same code repeatedly, but I guess
if we don't expect a whole load more interfaces to be added to the API in
future then I'll just expand it inline.

>> +      {
>> +	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.

  Right, thanks, will fix.

    cheers,
      DaveK


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