[PATCH] Plugin API clarification.

[PATCH] Plugin API clarification.

[Dave Korn]

[-- Attachment #1: Type: text/plain, Size: 1175 bytes --]


    Hi list,

  Following on from the fix of GCC PR46291, Cary clarified for me that the
plugin interface is meant to retain control of the file descriptor that it
passes to the claim_file handler.  In LD, that means we should close it after
the handler returns, regardless whether it claimed the file or not.

  While I was there, I fixed some formatting problems, and I added a tiny
optimisation that saves us from having to open the fd and create a dummy bfd
only to throw them both away a moment later when there aren't any plugins that
could possibly claim it anyway.

ld/ChangeLog:

	* plugin.h (plugin_active_plugins_p): New prototype.
	(is_ir_dummy_bfd): Delete prototype.
	* plugin.c: Fix formatting issues.
	(is_ir_dummy_bfd): Make static.
	(plugin_active_plugins_p): New function.
	* ldfile.c (ldfile_try_open_bfd): Use it to save work if no plugins
	are loaded.  Always close file descriptor after claim handler returns.
	* ldmain.c (add_archive_element): Likewise.


  Built+tested on i686-pc-cygwin without regressions.  Am bootstrapping
GCC+lto-plugin and will verify it against that after it's finished building.
OK if no problems?

    cheers,
      DaveK


[-- Attachment #2: plugin-api-clarification.diff --]
[-- Type: text/x-c, Size: 5681 bytes --]

Index: ld/plugin.h
===================================================================
RCS file: /cvs/src/src/ld/plugin.h,v
retrieving revision 1.1
diff -p -u -r1.1 plugin.h
--- ld/plugin.h	14 Oct 2010 01:31:31 -0000	1.1
+++ ld/plugin.h	4 Nov 2010 02:59:01 -0000
@@ -33,6 +33,10 @@ extern int plugin_opt_plugin (const char
    error if none.  */
 extern int plugin_opt_plugin_arg (const char *arg);
 
+/* Return true if any plugins are active this run.  Only valid
+   after options have been processed.  */
+extern bfd_boolean plugin_active_plugins_p (void);
+
 /* Load up and initialise all plugins after argument parsing.  */
 extern int plugin_load_plugins (void);
 
@@ -56,9 +60,6 @@ extern int plugin_call_cleanup (void);
    add_symbols hook has been called so that it can be read when linking.  */
 extern bfd *plugin_get_ir_dummy_bfd (const char *name, bfd *template);
 
-/* Check if the BFD passed in is an IR dummy object file.  */
-extern bfd_boolean is_ir_dummy_bfd (const bfd *abfd);
-
 /* Notice-symbol bfd linker callback hook.  */
 extern bfd_boolean plugin_notice (struct bfd_link_info *info,
 		const char *name, bfd *abfd, asection *section,
Index: ld/plugin.c
===================================================================
RCS file: /cvs/src/src/ld/plugin.c,v
retrieving revision 1.5
diff -p -u -r1.5 plugin.c
--- ld/plugin.c	25 Oct 2010 06:22:50 -0000	1.5
+++ ld/plugin.c	4 Nov 2010 02:59:01 -0000
@@ -172,13 +172,15 @@ plugin_error_p (void)
 }
 
 /* Return name of plugin which caused an error if any.  */
-const char *plugin_error_plugin (void)
+const char *
+plugin_error_plugin (void)
 {
   return error_plugin ? error_plugin : _("<no plugin>");
 }
 
 /* Handle -plugin arg: find and load plugin, or return error.  */
-int plugin_opt_plugin (const char *plugin)
+int
+plugin_opt_plugin (const char *plugin)
 {
   plugin_t *newplug;
 
@@ -201,7 +203,8 @@ int plugin_opt_plugin (const char *plugi
 
 /* Accumulate option arguments for last-loaded plugin, or return
    error if none.  */
-int plugin_opt_plugin_arg (const char *arg)
+int
+plugin_opt_plugin_arg (const char *arg)
 {
   plugin_arg_t *newarg;
 
@@ -241,8 +244,8 @@ plugin_get_ir_dummy_bfd (const char *nam
   return abfd;
 }
 
-/* Check if the BFD is an IR dummy.  */
-bfd_boolean
+/* Check if the BFD passed in is an IR dummy object file.  */
+static bfd_boolean
 is_ir_dummy_bfd (const bfd *abfd)
 {
   size_t namlen = strlen (abfd->filename);
@@ -611,8 +614,17 @@ set_tv_plugin_args (plugin_t *plugin, st
   tv->tv_u.tv_val = 0;
 }
 
+/* Return true if any plugins are active this run.  Only valid
+   after options have been processed.  */
+bfd_boolean
+plugin_active_plugins_p (void)
+{
+  return plugins_list != NULL;
+}
+
 /* Load up and initialise all plugins after argument parsing.  */
-int plugin_load_plugins (void)
+int
+plugin_load_plugins (void)
 {
   struct ld_plugin_tv *my_tv;
   unsigned int max_args = 0;
Index: ld/ldfile.c
===================================================================
RCS file: /cvs/src/src/ld/ldfile.c,v
retrieving revision 1.57
diff -p -u -r1.57 ldfile.c
--- ld/ldfile.c	29 Oct 2010 12:10:36 -0000	1.57
+++ ld/ldfile.c	4 Nov 2010 02:59:01 -0000
@@ -312,7 +312,8 @@ success:
      bfd_object that it sets the bfd's arch and mach, which
      will be needed when and if we want to bfd_create a new
      one using this one as a template.  */
-  if (bfd_check_format (entry->the_bfd, bfd_object))
+  if (bfd_check_format (entry->the_bfd, bfd_object)
+	&& plugin_active_plugins_p ())
     {
       int fd = open (attempt, O_RDONLY | O_BINARY);
       if (fd >= 0)
@@ -330,6 +331,8 @@ success:
 	  if (plugin_call_claim_file (&file, &claimed))
 	    einfo (_("%P%F: %s: plugin reported error claiming file\n"),
 	      plugin_error_plugin ());
+	  /* fd belongs to us, not the plugin; but we don't need it.  */
+	  close (fd);
 	  if (claimed)
 	    {
 	      /* Discard the real file's BFD and substitute the dummy one.  */
@@ -340,10 +343,9 @@ success:
 	    }
 	  else
 	    {
-	      /* If plugin didn't claim the file, we don't need the fd or the
-	         dummy bfd.  Can't avoid speculatively creating it, alas.  */
+	      /* If plugin didn't claim the file, we don't need the dummy
+	         bfd.  Can't avoid speculatively creating it, alas.  */
 	      bfd_close_all_done (file.handle);
-	      close (fd);
 	      entry->claimed = FALSE;
 	    }
 	}
Index: ld/ldmain.c
===================================================================
RCS file: /cvs/src/src/ld/ldmain.c,v
retrieving revision 1.144
diff -p -u -r1.144 ldmain.c
--- ld/ldmain.c	14 Oct 2010 01:31:31 -0000	1.144
+++ ld/ldmain.c	4 Nov 2010 02:59:01 -0000
@@ -809,7 +809,7 @@ add_archive_element (struct bfd_link_inf
      BFD, but we still want to output the original BFD filename.  */
   orig_input = *input;
 #ifdef ENABLE_PLUGINS
-  if (bfd_my_archive (abfd) != NULL)
+  if (bfd_my_archive (abfd) != NULL && plugin_active_plugins_p ())
     {
       /* We must offer this archive member to the plugins to claim.  */
       int fd = open (bfd_my_archive (abfd)->filename, O_RDONLY | O_BINARY);
@@ -831,6 +831,8 @@ add_archive_element (struct bfd_link_inf
 	  if (plugin_call_claim_file (&file, &claimed))
 	    einfo (_("%P%F: %s: plugin reported error claiming file\n"),
 	      plugin_error_plugin ());
+	  /* fd belongs to us, not the plugin; but we don't need it.  */
+	  close (fd);
 	  if (claimed)
 	    {
 	      /* Substitute the dummy BFD.  */
@@ -843,7 +845,6 @@ add_archive_element (struct bfd_link_inf
 	    {
 	      /* Abandon the dummy BFD.  */
 	      bfd_close_all_done (file.handle);
-	      close (fd);
 	      input->claimed = FALSE;
 	    }
 	}

Re: [PATCH] Plugin API clarification.

[Dave Korn]

On 04/11/2010 03:35, Dave Korn wrote:

> ld/ChangeLog:
> 
> 	* plugin.h (plugin_active_plugins_p): New prototype.
> 	(is_ir_dummy_bfd): Delete prototype.
> 	* plugin.c: Fix formatting issues.
> 	(is_ir_dummy_bfd): Make static.
> 	(plugin_active_plugins_p): New function.
> 	* ldfile.c (ldfile_try_open_bfd): Use it to save work if no plugins
> 	are loaded.  Always close file descriptor after claim handler returns.
> 	* ldmain.c (add_archive_element): Likewise.
> 
> 
>   Built+tested on i686-pc-cygwin without regressions.  Am bootstrapping
> GCC+lto-plugin and will verify it against that after it's finished building.
> OK if no problems?

  It finished and I did, and it was all correct.  May I commit?

    cheers,
      DaveK

Re: [PATCH] Plugin API clarification.

[Alan Modra]

On Fri, Nov 05, 2010 at 04:54:41AM +0000, Dave Korn wrote:
> > 	* plugin.h (plugin_active_plugins_p): New prototype.
> > 	(is_ir_dummy_bfd): Delete prototype.
> > 	* plugin.c: Fix formatting issues.
> > 	(is_ir_dummy_bfd): Make static.
> > 	(plugin_active_plugins_p): New function.
> > 	* ldfile.c (ldfile_try_open_bfd): Use it to save work if no plugins
> > 	are loaded.  Always close file descriptor after claim handler returns.
> > 	* ldmain.c (add_archive_element): Likewise.
> > 
> > 
> >   Built+tested on i686-pc-cygwin without regressions.  Am bootstrapping
> > GCC+lto-plugin and will verify it against that after it's finished building.
> > OK if no problems?
> 
>   It finished and I did, and it was all correct.  May I commit?

OK.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] Plugin API clarification.

[Dave Korn]

On 05/11/2010 05:41, Alan Modra wrote:
> On Fri, Nov 05, 2010 at 04:54:41AM +0000, Dave Korn wrote:
>>> 	* plugin.h (plugin_active_plugins_p): New prototype.
>>> 	(is_ir_dummy_bfd): Delete prototype.
>>> 	* plugin.c: Fix formatting issues.
>>> 	(is_ir_dummy_bfd): Make static.
>>> 	(plugin_active_plugins_p): New function.
>>> 	* ldfile.c (ldfile_try_open_bfd): Use it to save work if no plugins
>>> 	are loaded.  Always close file descriptor after claim handler returns.
>>> 	* ldmain.c (add_archive_element): Likewise.
>>>
>>>
>>>   Built+tested on i686-pc-cygwin without regressions.  Am bootstrapping
>>> GCC+lto-plugin and will verify it against that after it's finished building.
>>> OK if no problems?
>>   It finished and I did, and it was all correct.  May I commit?
> 
> OK.

  Done, thanks.

    cheers,
      DaveK