Re: PATCH: Improve plugin error handling

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

On Fri, Nov 23, 2012 at 8:13 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> Hi,
>
> This patch improves plugin error handling:
>
> 1. Use dlerror to report error whenerve possible.
> 2. Report plugin error code on error.
> 3. Only issue cleanup error during cleanup.
>
> OK to install?

I opened a bug to show the problem:

http://sourceware.org/bugzilla/show_bug.cgi?id=14904

OK to install?

> Thanks.
>
>
> H.J.
> ---
> ld/
>
> 2012-11-22  H.J. Lu  <hongjiu.lu@intel.com>
>
>         * ldmain.c (main): Don't check plugin_load_plugins return.
>
>         * lexsup.c (parse_args): Don't check plugin_opt_plugin return.
>
>         * plugin.c (dl_error): New.
>         (plugin_opt_plugin): Change return type to void.  Stop on
>         dlopen error and report error with dl_error ().
>         (plugin_load_plugins): Change return type to void.  Stop on
>         dlsym error and report error with dl_error ().  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.
>
> ld/testsuite/
>
> 2012-11-22  H.J. Lu  <hongjiu.lu@intel.com>
>
>         * ld-plugin/plugin-2.d: Update expected error message.
>         * ld-plugin/plugin-4.d: Likewise.
>
> 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..a11ccde 100644
> --- a/ld/plugin.c
> +++ b/ld/plugin.c
> @@ -155,6 +155,20 @@ dlclose (void *handle)
>
>  #endif /* !defined (HAVE_DLFCN_H) && defined (HAVE_WINDOWS_H)  */
>
> +#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
> +
>  /* Helper function for exiting with error status.  */
>  static int
>  set_plugin_error (const char *plugin)
> @@ -178,7 +192,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 +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));
>
>    /* Chain on end, so when we run list it is in command-line order.  */
>    *plugins_tail_chain_ptr = newplug;
> @@ -197,7 +211,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 +784,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 +793,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 +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));
>        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);
>        curplug = curplug->next;
>      }
>
> @@ -825,8 +840,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 +942,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/ld-plugin/plugin-2.d b/ld/testsuite/ld-plugin/plugin-2.d
> index 0ce111f..983f1cf 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.*: error loading plugin: 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\)
>  #...



-- 
H.J.