Re: [patch] Add plugin support for bfd

[Mike Frysinger <vapier at gentoo dot org>]

>+AM_CFLAGS = $(WARN_CFLAGS) $(LFS_CFLAGS) -DBINDIR='"$(bindir)"'

-D flags are CPPFLAGS, not CFLAGS, and thus should go in AM_CPPFLAGS

> cpu-i386.lo: cpu-i386.c $(INCDIR)/filenames.h $(INCDIR)/hashtab.h
>+cpu-plugin.lo: cpu-plugin.c $(INCDIR)/filenames.h $(INCDIR)/hashtab.h
> cpu-i860.lo: cpu-i860.c $(INCDIR)/filenames.h $(INCDIR)/hashtab.h

shouldnt the .lo dependency list be kept sorted ?  "plugin" does not come 
after "i386" and before "i860".

>+AC_ARG_ENABLE([plugins],
>+[  --enable-plugins        linker plugins],

should really start using AS_HELP_STRING() in AC_ARG_ENABLE() and 
AC_ARG_WITH()

>+[plugins=no])

any reason to not enable plugins by default ?  i guess since it requires 
dlopen and/or -ldl, that's a good reason to disable by default.

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.

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

>+  enable_targets="$enable_targets plugin";

you dont need semicolons for assignments

>+++ b/bfd/cpu-plugin.c
>+/* BFD support for plugins.
>+   Copyright 1992, 1994, 1995, 1996, 1998, 2000, 2001, 2002, 2004, 2007
>+   Free Software Foundation, Inc.

outdated copyright info ... same goes for all new files you added

>+static enum ld_plugin_status
>+message (int level ATTRIBUTE_UNUSED,
>+	 const char * format, ...)

do you really need your own helper function for output ?

>+// Register a claim-file handler.

afaik, binutils does not use/allow C99 comments, so use /* ... */

>+extern char *program_name __attribute__ ((weak));

you declare it weak, yet assume it exists ... that's bad

>+      fprintf (stderr, "%s\n", dlerror());

there are error/warning functions already in bfd you should be using

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

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

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

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