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

["Martin Liška" <mliska@suse.cz>]

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

On 6/20/22 11:32, Richard Biener wrote:
> 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

Verified that it's fine.

> 
> 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?)

Sure.

May I install the patch with the change?

Cheers,
Martin

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

[-- Attachment #2: 0001-lto-plugin-make-claim_file_handler-thread-safe.patch --]
[-- Type: text/x-patch, Size: 2478 bytes --]

From 12fb5f8fbb283313f9d5bcadb24c45904128804d Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Mon, 16 May 2022 14:18:41 +0200
Subject: [PATCH 1/2] lto-plugin: make claim_file_handler thread-safe

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, 16 insertions(+)

diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
index 00b760636dc..2d95b7b3803 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>
 #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,14 @@ 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:
+  pthread_mutex_lock (&plugin_lock);
   non_claimed_files++;
+  pthread_mutex_unlock (&plugin_lock);
   free (lto_file.name);
 
  cleanup:
@@ -1415,6 +1425,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