Re: [patch] Add plugin support for bfd

[Mike Frysinger <vapier at gentoo dot org>]

On Thursday 30 April 2009 13:25:56 Rafael Espindola wrote:
> > which leads to the next part -- this code should be checking for dlopen
> > support and erroring out if someone tries to enable plugins but they dont
> > have libdl support.
>
> Is there a canonical way to do this?

libtool already does dlopen checking, so i would just use $enable_dlopen.  
otherwise you'd have to do like:
	AC_CHECK_LIB(dl, dlopen)

> > also, if instead you updated one of the bfd in header files with like:
> > #define BFD_SUPPORTS_PLUGINS @...@
> > you shouldnt need to touch any other configure.in files as all the other
> > projects can key off this define ...
>
> The problem is that I need the value in the Makefiles, not just on the
> C source code:

not if the libtool think i suggested later is implemented

> >>+static enum ld_plugin_status
> >>+message (int level ATTRIBUTE_UNUSED,
> >>+       const char * format, ...)
> >
> > do you really need your own helper function for output ?
>
> Yes, part of the plugin API.

message() provides stdout only, and it doesnt provide any standard format.  
you should at least prefix messages with like:
printf("bfd_plugin: %s: ", plugin_name);

nothing worse than running a high level command and getting back a useless 
error message like:
error: blah
no idea *where* that error is coming from

> >>+  plugin_dir = concat (BINDIR, "/../lib/bfd-plugins", NULL);
> >
> > assuming/requiring the program being executed (ld in this case) is
> > relative to its library directory smells completely broken to me
>
> It is looking for its plugins, not a generic library. That was
> suggested by Joseph S. Myers. What would you suggest instead?

considering there is already precedence here with how ld works and its 
ldscripts dir, i would think you should mirror that.  or, the ld function 
set_scripts_dir() could be split apart so both ld and your plugin function 
could leverage it with a simple "find me a dir named 'FOO' in the normal 
library search paths".

> >>+++ b/binutils/Makefile.am
> >>+if PLUGINS
> >>+LIBDL = -ldl
> >>+endif
> >
> > this looks broken/backwards to me, as do the rest of the LDADD changes in
> > the Makefile.am files.  these programs arent using libdl, so why do they
> > need to link against it  binutils uses libtool for library creation, so
> > the -ldl should be in libbfd.la so that everyone else will get it
> > automatically and you dont need this stuff.  it will also let you drop
> > the changes for gprof/ld/gas/etc... that dont actually invoke any plugin
> > code directly.
>
> I remember having a link error, but I don't remember the details. Will
> try without it and see what breaks.

well, you have to add -ldl to the bfd link lines otherwise libtool wont record 
it ... you probably get a link failure already if you enable shared library 
support via the configure command line

> >>+++ b/binutils/ar.c
> >>+      bfd_plugin_set_plugin (argv[2]);
> >
> > there is no conditional compile here, so ar will fail to link if plugin
> > support is disabled ?  same for nm.c
>
> The function was added to bfd.c to avoid that. I can move it to
> plugin.c and add conditional compilation if you prefer.

yeah, i see that now.  either method should work fine, but it'll probably be 
confusing when people use the --plugin option and dont get back an error if 
plugin support is actually disabled ... or do you also have to set the bfd to 
the plugin bfd ?
-mike

Attachment: signature.asc
Description: This is a digitally signed message part.