On 6/21/22 09:56, Richard Biener wrote: > On Mon, Jun 20, 2022 at 12:20 PM Martin Liška <mli...@suse.cz> wrote: >> >> On 6/20/22 11:32, Richard Biener wrote: >>> On Thu, Jun 16, 2022 at 9:01 AM Martin Liška <mli...@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 >>>> >>>>
From f1e2f84dbfdac5c7aee7036e78841cb33c3bad50 Mon Sep 17 00:00:00 2001 From: Martin Liska <mli...@suse.cz> Date: Mon, 16 May 2022 14:18:41 +0200 Subject: [PATCH] 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/config.h.in | 3 +++ lto-plugin/configure | 17 +++++++++++++++-- lto-plugin/configure.ac | 7 +++++++ lto-plugin/lto-plugin.c | 20 ++++++++++++++++++++ 4 files changed, 45 insertions(+), 2 deletions(-) diff --git a/lto-plugin/config.h.in b/lto-plugin/config.h.in index ddbde7619d5..029e782f1ee 100644 --- a/lto-plugin/config.h.in +++ b/lto-plugin/config.h.in @@ -9,6 +9,9 @@ /* Define to 1 if you have the <memory.h> header file. */ #undef HAVE_MEMORY_H +/* Define to 1 if pthread.h is present. */ +#undef HAVE_PTHREAD_H + /* Define to 1 if you have the <stdint.h> header file. */ #undef HAVE_STDINT_H diff --git a/lto-plugin/configure b/lto-plugin/configure index b820accfd65..6d452063cfb 100755 --- a/lto-plugin/configure +++ b/lto-plugin/configure @@ -5643,6 +5643,9 @@ ac_compiler_gnu=$ac_cv_c_compiler_gnu +# The plug-in depends on pthreads +LDFLAGS="-lpthread" + # Check whether -static-libgcc is supported. saved_LDFLAGS="$LDFLAGS" LDFLAGS="$LDFLAGS -static-libgcc" @@ -6010,6 +6013,16 @@ else fi +# Check for thread headers. +ac_fn_c_check_header_mongrel "$LINENO" "pthread.h" "ac_cv_header_pthread_h" "$ac_includes_default" +if test "x$ac_cv_header_pthread_h" = xyes; then : + +$as_echo "#define HAVE_PTHREAD_H 1" >>confdefs.h + +fi + + + case `pwd` in *\ * | *\ *) { $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: Libtool does not cope well with whitespace in \`pwd\`" >&5 @@ -12081,7 +12094,7 @@ else lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2 lt_status=$lt_dlunknown cat > conftest.$ac_ext <<_LT_EOF -#line 12084 "configure" +#line 12097 "configure" #include "confdefs.h" #if HAVE_DLFCN_H @@ -12187,7 +12200,7 @@ else lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2 lt_status=$lt_dlunknown cat > conftest.$ac_ext <<_LT_EOF -#line 12190 "configure" +#line 12203 "configure" #include "confdefs.h" #if HAVE_DLFCN_H diff --git a/lto-plugin/configure.ac b/lto-plugin/configure.ac index bc5b618a495..aafabd4fa7b 100644 --- a/lto-plugin/configure.ac +++ b/lto-plugin/configure.ac @@ -13,6 +13,9 @@ AC_PROG_CC AC_SYS_LARGEFILE ACX_PROG_CC_WARNING_OPTS([-Wall], [ac_lto_plugin_warn_cflags]) +# The plug-in depends on pthreads +LDFLAGS="-lpthread" + # Check whether -static-libgcc is supported. saved_LDFLAGS="$LDFLAGS" LDFLAGS="$LDFLAGS -static-libgcc" @@ -87,6 +90,10 @@ AM_CONDITIONAL(LTO_PLUGIN_USE_SYMVER, [test "x$lto_plugin_use_symver" != xno]) AM_CONDITIONAL(LTO_PLUGIN_USE_SYMVER_GNU, [test "x$lto_plugin_use_symver" = xgnu]) AM_CONDITIONAL(LTO_PLUGIN_USE_SYMVER_SUN, [test "x$lto_plugin_use_symver" = xsun]) +# Check for thread headers. +AC_CHECK_HEADER(pthread.h, + [AC_DEFINE(HAVE_PTHREAD_H, 1, [Define to 1 if pthread.h is present.])]) + AM_PROG_LIBTOOL ACX_LT_HOST_FLAGS AC_SUBST(target_noncanonical) diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c index 00b760636dc..635e126946b 100644 --- a/lto-plugin/lto-plugin.c +++ b/lto-plugin/lto-plugin.c @@ -40,7 +40,11 @@ along with this program; see the file COPYING3. If not see #ifdef HAVE_CONFIG_H #include "config.h" +#if !HAVE_PTHREAD_H +#error POSIX threads are mandatory dependency #endif +#endif + #if HAVE_STDINT_H #include <stdint.h> #endif @@ -55,6 +59,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 +162,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 +1270,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 +1344,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 +1429,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