Re: [PATCH 2/3] lto-plugin: make claim_file_handler thread-safe

[Richard Biener <richard.guenther@gmail.com>]

On Thu, Jun 16, 2022 at 9:01 AM Martin Liška <mliska@suse.cz> wrote:
>
> lto-plugin/ChangeLog:
>
>         * lto-plugin.c (plugin_lock): New lock.
>         (claim_file_handler): Use mutex for critical section.
>         (onload): Initialize mutex.
> ---
>  lto-plugin/lto-plugin.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
> index 00b760636dc..13118c4983c 100644
> --- a/lto-plugin/lto-plugin.c
> +++ b/lto-plugin/lto-plugin.c
> @@ -55,6 +55,7 @@ along with this program; see the file COPYING3.  If not see
>  #include <unistd.h>
>  #include <fcntl.h>
>  #include <sys/types.h>
> +#include <pthread.h>

Not sure if we support any non-pthread target for building the LTO
plugin, but it
seems we have

  # Among non-ELF, only Windows platforms support the lto-plugin so far.
  # Build it unless LTO was explicitly disabled.
  case $target in
    *-cygwin* | *-mingw*) build_lto_plugin=$enable_lto ;;

which suggests that at least build validating the above with --enable-lto

IIRC we have gthr-*.h in libgcc/, not sure if that's usable in a
host linker plugin.

>  #ifdef HAVE_SYS_WAIT_H
>  #include <sys/wait.h>
>  #endif
> @@ -157,6 +158,9 @@ enum symbol_style
>    ss_uscore,   /* Underscore prefix all symbols.  */
>  };
>
> +/* Plug-in mutex.  */
> +static pthread_mutex_t plugin_lock;
> +
>  static char *arguments_file_name;
>  static ld_plugin_register_claim_file register_claim_file;
>  static ld_plugin_register_all_symbols_read register_all_symbols_read;
> @@ -1262,15 +1266,18 @@ claim_file_handler (const struct ld_plugin_input_file *file, int *claimed)
>                               lto_file.symtab.syms);
>        check (status == LDPS_OK, LDPL_FATAL, "could not add symbols");
>
> +      pthread_mutex_lock (&plugin_lock);
>        num_claimed_files++;
>        claimed_files =
>         xrealloc (claimed_files,
>                   num_claimed_files * sizeof (struct plugin_file_info));
>        claimed_files[num_claimed_files - 1] = lto_file;
> +      pthread_mutex_unlock (&plugin_lock);
>
>        *claimed = 1;
>      }
>
> +  pthread_mutex_lock (&plugin_lock);
>    if (offload_files == NULL)
>      {
>        /* Add dummy item to the start of the list.  */
> @@ -1333,11 +1340,12 @@ claim_file_handler (const struct ld_plugin_input_file *file, int *claimed)
>         offload_files_last_lto = ofld;
>        num_offload_files++;
>      }
> +  pthread_mutex_unlock (&plugin_lock);
>
>    goto cleanup;
>
>   err:
> -  non_claimed_files++;
> +  __atomic_fetch_add (&non_claimed_files, 1, __ATOMIC_RELAXED);

is it worth "optimizing" this with yet another need for target specific support
(just use pthread_mutex here as well?)

>    free (lto_file.name);
>
>   cleanup:
> @@ -1415,6 +1423,12 @@ onload (struct ld_plugin_tv *tv)
>    struct ld_plugin_tv *p;
>    enum ld_plugin_status status;
>
> +  if (pthread_mutex_init (&plugin_lock, NULL) != 0)
> +    {
> +      fprintf (stderr, "mutex init failed\n");
> +      abort ();
> +    }
> +
>    p = tv;
>    while (p->tv_tag)
>      {
> --
> 2.36.1
>
>