Message ID | d653dfac-0fd4-eb94-469d-874731d89a59@suse.cz |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 1BF60385736F for <patchwork@sourceware.org>; Thu, 16 Jun 2022 07:01:42 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id C5F633858C56 for <gcc-patches@gcc.gnu.org>; Thu, 16 Jun 2022 07:01:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C5F633858C56 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.cz Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id CE02421C85 for <gcc-patches@gcc.gnu.org>; Thu, 16 Jun 2022 07:01:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1655362880; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=h19JkkU3KWJivqigsWVT2ppUkv594g8X7QukBGVDAog=; b=WDZxoZ3kaF4linIeciugx1m9e8xSrMNxqbScqLk9RmSIK3sc/DFvwih111dOyi5cmCw38t upeIiqnMHX5/y2NtM8YZPo03VY6CqfGOgDPYgc5vP+kaYXwUt90Of7htu37scs+wojgByQ VGfkJHf5tXb/H5ldD8BTjj9z5fQ9g/Y= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1655362880; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=h19JkkU3KWJivqigsWVT2ppUkv594g8X7QukBGVDAog=; b=2G5S1xPSz+/tBAtbcKesjLQEL85HazlSDoOoyXe8zzLNRB5jkub0EPeriaRP+wIGOWDdhP +VWn0vTx8Tom9RBA== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id BFF3413A70 for <gcc-patches@gcc.gnu.org>; Thu, 16 Jun 2022 07:01:20 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id ofj5LUDVqmLQDgAAMHmgww (envelope-from <mliska@suse.cz>) for <gcc-patches@gcc.gnu.org>; Thu, 16 Jun 2022 07:01:20 +0000 Message-ID: <d653dfac-0fd4-eb94-469d-874731d89a59@suse.cz> Date: Thu, 16 Jun 2022 09:01:20 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.10.0 From: =?utf-8?q?Martin_Li=C5=A1ka?= <mliska@suse.cz> Subject: [PATCH 2/3] lto-plugin: make claim_file_handler thread-safe To: gcc-patches@gcc.gnu.org References: <803a0290-3909-b9c5-2461-b1740a00c63a@suse.cz> Content-Language: en-US In-Reply-To: <803a0290-3909-b9c5-2461-b1740a00c63a@suse.cz> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
[1/3] lto-plugin: support LDPT_GET_SYMBOLS_V3
|
|
Commit Message
Martin Liška
June 16, 2022, 7:01 a.m. UTC
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(-)
Comments
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 > >
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 >> >>
On Mon, Jun 20, 2022 at 12:20 PM Martin Liška <mliska@suse.cz> wrote: > > 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? Can you at least add a configure check for pthread.h and maybe disable locking when not found or erroring out? I figure we have GCC_AC_THREAD_HEADER for the gthr.h stuff using $target_thread_file (aka --enable-threads=XYZ), but as said that's for the target and I don't see any host uses. We might also add an explicit list of hosts (*-linux*?) where we enable thread support for lto-plugin, providing opt-in (so you'd have to wrap the mutex taking or if-def it out). I think you also need to link lto-plugin with -pthread, no? On linux it might work omitting that but I'm not sure other libc have serial pthread stubs in their libc. BFD ld definitely doesn't link against pthread so dlopening lto-plugin will fail (also not all libc might like initializing threads from a dlopen _init). Richard. > 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 > >> > >>
On 6/21/22 09:56, Richard Biener wrote: > On Mon, Jun 20, 2022 at 12:20 PM Martin Liška <mliska@suse.cz> wrote: >> >> 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? > > Can you at least add a configure check for pthread.h and maybe disable > locking when not found or erroring out? I figure we have GCC_AC_THREAD_HEADER All right, let's error out then. > for the gthr.h stuff using $target_thread_file (aka --enable-threads=XYZ), > but as said that's for the target and I don't see any host uses. We might also > add an explicit list of hosts (*-linux*?) where we enable thread support for > lto-plugin, providing opt-in (so you'd have to wrap the mutex taking or > if-def it out). > > I think you also need to link lto-plugin with -pthread, no? Yep. Please see the updated patch. > On linux > it might work omitting that but I'm not sure other libc have serial pthread > stubs in their libc. BFD ld definitely doesn't link against pthread so > dlopening lto-plugin will fail (also not all libc might like > initializing threads > from a dlopen _init). What initializing threads do you mean? Martin > > Richard. > >> 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 >>>> >>>>
> Am 21.06.2022 um 10:43 schrieb Martin Liška <mliska@suse.cz>: > > On 6/21/22 09:56, Richard Biener wrote: >>> On Mon, Jun 20, 2022 at 12:20 PM Martin Liška <mliska@suse.cz> wrote: >>> >>> 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? >> >> Can you at least add a configure check for pthread.h and maybe disable >> locking when not found or erroring out? I figure we have GCC_AC_THREAD_HEADER > > All right, let's error out then. > >> for the gthr.h stuff using $target_thread_file (aka --enable-threads=XYZ), >> but as said that's for the target and I don't see any host uses. We might also >> add an explicit list of hosts (*-linux*?) where we enable thread support for >> lto-plugin, providing opt-in (so you'd have to wrap the mutex taking or >> if-def it out). >> >> I think you also need to link lto-plugin with -pthread, no? > > Yep. > > Please see the updated patch. > Please use -pthread instead of -lpthread Otherwise looks OK to me. >> On linux >> it might work omitting that but I'm not sure other libc have serial pthread >> stubs in their libc. BFD ld definitely doesn't link against pthread so >> dlopening lto-plugin will fail (also not all libc might like >> initializing threads >> from a dlopen _init). > > What initializing threads do you mean? It’s target dependent what kind of global state init need to be done when any pthread facility is used. Richard > Martin > >> >> Richard. >> >>> 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 >>>>> >>>>> > <0001-lto-plugin-make-claim_file_handler-thread-safe.patch>
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> #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); 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) {