Re: PATCH: Improve plugin error handling

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Sun, Dec 2, 2012 at 7:28 PM, Alan Modra <amodra@gmail.com> wrote:
> On Fri, Nov 23, 2012 at 08:13:14AM -0800, H.J. Lu wrote:
>> +#ifdef HAVE_DLFCN_H
>> +static const char *
>> +dl_error (const char *plugin ATTRIBUTE_UNUSED)
>> +{
>> +  return dlerror ();
>> +}
>> +#else
>> +static const char *
>> +dl_error (const * char plugin)
>> +{
>> +  return plugin;
>> +}
>> +#endif
>> +
>
> I think the above would be better without the "plugin" arg, with the
> !HAVE_DLFCN_H case returning "".

I just renamed dl_error to dlerror and defined it for the
!HAVE_DLFCN_H case.

>
>> @@ -188,7 +202,7 @@ plugin_opt_plugin (const char *plugin)
>>    newplug->name = plugin;
>>    newplug->dlhandle = dlopen (plugin, RTLD_NOW);
>>    if (!newplug->dlhandle)
>> -    return set_plugin_error (plugin);
>> +    einfo (_("%P%F: error loading plugin: %s\n"), dl_error (plugin));
>
> I think this message should still print the plugin name.  ie.
>     einfo (_("%P%F: %s: error loading plugin: %s\n"), plugin, dl_error ());
>
>> @@ -806,13 +819,15 @@ plugin_load_plugins (void)
>>        if (!onloadfn)
>>       onloadfn = (ld_plugin_onload) dlsym (curplug->dlhandle, "_onload");
>>        if (!onloadfn)
>> -     return set_plugin_error (curplug->name);
>> +     einfo (_("%P%F: error loading plugin: %s\n"),
>> +            dl_error (curplug->name));
>
> Similarly here.
>
>>        set_tv_plugin_args (curplug, &my_tv[tv_header_size]);
>>        called_plugin = curplug;
>>        rv = (*onloadfn) (my_tv);
>>        called_plugin = NULL;
>>        if (rv != LDPS_OK)
>> -     return set_plugin_error (curplug->name);
>> +     einfo (_("%P%F: %s: error loading plugin: %d\n"),
>> +            curplug->name, rv);
>
> You've successfully loaded the plugin here, but hit some runtime error.
> Perhaps just s/error loading plugin/plugin error/.
>
> OK with those changes, and of course altering the testsuite to suit.
>

This is the patch I checked in.

-- 
H.J.
--
diff --git a/ld/ChangeLog b/ld/ChangeLog
index 4befee4..812a5ff 100644
--- a/ld/ChangeLog
+++ b/ld/ChangeLog
@@ -1,3 +1,22 @@
+2012-12-03  H.J. Lu  <hongjiu.lu@intel.com>
+
+	PR ld/14904
+	* ldmain.c (main): Don't check plugin_load_plugins return.
+
+	* lexsup.c (parse_args): Don't check plugin_opt_plugin return.
+
+	* plugin.c (dlerror): New.  Defined if HAVE_DLFCN_H isn't
+	defined.
+	(plugin_opt_plugin): Change return type to void.  Stop on
+	dlopen error and report error with dlerror ().
+	(plugin_load_plugins): Change return type to void.  Stop on
+	dlsym error and report error with dlerror ().  Don't use
+	set_plugin_error.
+	(plugin_call_cleanup): Issue an error for each plugin.
+
+	* plugin.h (plugin_opt_plugin): Change return type to void.
+	(plugin_load_plugins): Likewise.
+
 2012-11-30  Joern Rennecke <joern.rennecke@embecosm.com>

 	* scripttempl/epiphany_4x4.sc, emulparams/elf32epiphany_4x4.sh: Add.
diff --git a/ld/ldmain.c b/ld/ldmain.c
index 07f5f09..c23c554 100644
--- a/ld/ldmain.c
+++ b/ld/ldmain.c
@@ -308,8 +308,7 @@ main (int argc, char **argv)

 #ifdef ENABLE_PLUGINS
   /* Now all the plugin arguments have been gathered, we can load them.  */
-  if (plugin_load_plugins ())
-    einfo (_("%P%F: %s: error loading plugin\n"), plugin_error_plugin ());
+  plugin_load_plugins ();
 #endif /* ENABLE_PLUGINS */

   ldemul_set_symbols ();
diff --git a/ld/lexsup.c b/ld/lexsup.c
index c6baebe..be8a897 100644
--- a/ld/lexsup.c
+++ b/ld/lexsup.c
@@ -954,9 +954,7 @@ parse_args (unsigned argc, char **argv)
 	  break;
 #ifdef ENABLE_PLUGINS
 	case OPTION_PLUGIN:
-	  if (plugin_opt_plugin (optarg))
-	    einfo (_("%P%F: %s: error loading plugin\n"),
-		   plugin_error_plugin ());
+	  plugin_opt_plugin (optarg);
 	  break;
 	case OPTION_PLUGIN_OPT:
 	  if (plugin_opt_plugin_arg (optarg))
diff --git a/ld/plugin.c b/ld/plugin.c
index 8902ef4..da99e77 100644
--- a/ld/plugin.c
+++ b/ld/plugin.c
@@ -155,6 +155,14 @@ dlclose (void *handle)

 #endif /* !defined (HAVE_DLFCN_H) && defined (HAVE_WINDOWS_H)  */

+#ifndef HAVE_DLFCN_H
+static const char *
+dlerror (void)
+{
+  return "";
+}
+#endif
+
 /* Helper function for exiting with error status.  */
 static int
 set_plugin_error (const char *plugin)
@@ -178,7 +186,7 @@ plugin_error_plugin (void)
 }

 /* Handle -plugin arg: find and load plugin, or return error.  */
-int
+void
 plugin_opt_plugin (const char *plugin)
 {
   plugin_t *newplug;
@@ -188,7 +196,7 @@ plugin_opt_plugin (const char *plugin)
   newplug->name = plugin;
   newplug->dlhandle = dlopen (plugin, RTLD_NOW);
   if (!newplug->dlhandle)
-    return set_plugin_error (plugin);
+    einfo (_("%P%F: %s: error loading plugin: %s\n"), plugin, dlerror ());

   /* Chain on end, so when we run list it is in command-line order.  */
   *plugins_tail_chain_ptr = newplug;
@@ -197,7 +205,6 @@ plugin_opt_plugin (const char *plugin)
   /* Record it as current plugin for receiving args.  */
   last_plugin = newplug;
   last_plugin_args_tail_chain_ptr = &newplug->args;
-  return 0;
 }

 /* Accumulate option arguments for last-loaded plugin, or return
@@ -771,7 +778,7 @@ plugin_active_plugins_p (void)
 }

 /* Load up and initialise all plugins after argument parsing.  */
-int
+void
 plugin_load_plugins (void)
 {
   struct ld_plugin_tv *my_tv;
@@ -780,7 +787,7 @@ plugin_load_plugins (void)

   /* If there are no plugins, we need do nothing this run.  */
   if (!curplug)
-    return 0;
+    return;

   /* First pass over plugins to find max # args needed so that we
      can size and allocate the tv array.  */
@@ -806,13 +813,14 @@ plugin_load_plugins (void)
       if (!onloadfn)
 	onloadfn = (ld_plugin_onload) dlsym (curplug->dlhandle, "_onload");
       if (!onloadfn)
-	return set_plugin_error (curplug->name);
+        einfo (_("%P%F: %s: error loading plugin: %s\n"),
+	       curplug->name, dlerror ());
       set_tv_plugin_args (curplug, &my_tv[tv_header_size]);
       called_plugin = curplug;
       rv = (*onloadfn) (my_tv);
       called_plugin = NULL;
       if (rv != LDPS_OK)
-	return set_plugin_error (curplug->name);
+	einfo (_("%P%F: %s: plugin error: %d\n"), curplug->name, rv);
       curplug = curplug->next;
     }

@@ -825,8 +833,6 @@ plugin_load_plugins (void)
   plugin_callbacks.notice = &plugin_notice;
   link_info.notice_all = TRUE;
   link_info.callbacks = &plugin_callbacks;
-
-  return 0;
 }

 /* Call 'claim file' hook for all plugins.  */
@@ -929,14 +935,12 @@ plugin_call_cleanup (void)
 	  rv = (*curplug->cleanup_handler) ();
 	  called_plugin = NULL;
 	  if (rv != LDPS_OK)
-	    set_plugin_error (curplug->name);
+	    info_msg (_("%P: %s: error in plugin cleanup: %d (ignored)\n"),
+		      curplug->name, rv);
 	  dlclose (curplug->dlhandle);
 	}
       curplug = curplug->next;
     }
-  if (plugin_error_p ())
-    info_msg (_("%P: %s: error in plugin cleanup (ignored)\n"),
-	      plugin_error_plugin ());
 }

 /* To determine which symbols should be resolved LDPR_PREVAILING_DEF
diff --git a/ld/plugin.h b/ld/plugin.h
index dc32295..a227575 100644
--- a/ld/plugin.h
+++ b/ld/plugin.h
@@ -32,8 +32,8 @@ extern bfd_boolean no_more_claiming;
    to include the plugin-api.h header in order to use this file.  */
 struct ld_plugin_input_file;

-/* Handle -plugin arg: find and load plugin, or return error.  */
-extern int plugin_opt_plugin (const char *plugin);
+/* Handle -plugin arg: find and load plugin.  */
+extern void plugin_opt_plugin (const char *plugin);

 /* Accumulate option arguments for last-loaded plugin, or return
    error if none.  */
@@ -44,7 +44,7 @@ extern int plugin_opt_plugin_arg (const char *arg);
 extern bfd_boolean plugin_active_plugins_p (void);

 /* Load up and initialise all plugins after argument parsing.  */
-extern int plugin_load_plugins (void);
+extern void plugin_load_plugins (void);

 /* Return name of plugin which caused an error in any of the above.  */
 extern const char *plugin_error_plugin (void);
diff --git a/ld/testsuite/ChangeLog b/ld/testsuite/ChangeLog
index 493d4ba..fd49cd6 100644
--- a/ld/testsuite/ChangeLog
+++ b/ld/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2012-12-03  H.J. Lu  <hongjiu.lu@intel.com>
+
+	PR ld/14904
+	* ld-plugin/plugin-2.d: Update expected error message.
+	* ld-plugin/plugin-4.d: Likewise.
+
 2012-11-30  Roland McGrath  <mcgrathr@google.com>

 	* ld-elf/ehdr_start.s: Put reference in .rodata section, not .data.
diff --git a/ld/testsuite/ld-plugin/plugin-2.d
b/ld/testsuite/ld-plugin/plugin-2.d
index 0ce111f..d0190a7 100644
--- a/ld/testsuite/ld-plugin/plugin-2.d
+++ b/ld/testsuite/ld-plugin/plugin-2.d
@@ -18,5 +18,5 @@ Hello from testplugin.
 .*: LDPT_OPTION 'failonload'
 .*: LDPT_NULL value        0x0 \(0\)
 #...
-.*ld.*:.*ldtestplug.*: error loading plugin
+.*ld.*:.*ldtestplug.*: plugin error: 3
 #...
diff --git a/ld/testsuite/ld-plugin/plugin-4.d
b/ld/testsuite/ld-plugin/plugin-4.d
index e17565e..9f25fc6 100644
--- a/ld/testsuite/ld-plugin/plugin-4.d
+++ b/ld/testsuite/ld-plugin/plugin-4.d
@@ -20,5 +20,5 @@ Hello from testplugin.
 .*: LDPT_NULL value        0x0 \(0\)
 #...
 hook called: cleanup.
-.*ld.*:.*ldtestplug.*: error in plugin cleanup \(ignored\)
+.*ld.*:.*ldtestplug.*: error in plugin cleanup: 3 \(ignored\)
 #...