From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x333.google.com (mail-wm1-x333.google.com [IPv6:2a00:1450:4864:20::333]) by sourceware.org (Postfix) with ESMTPS id 7DEBC385AE41 for ; Fri, 24 Jun 2022 08:37:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7DEBC385AE41 Received: by mail-wm1-x333.google.com with SMTP id c130-20020a1c3588000000b0039c6fd897b4so2912150wma.4 for ; Fri, 24 Jun 2022 01:37:03 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:content-transfer-encoding:from:mime-version :subject:date:message-id:references:cc:in-reply-to:to; bh=vhUIKhcmQjAWZh8oGc/xKj0/5UrtWsqCBQQtCzZiMrs=; b=ps6fl1xZTWraAhcX1yv3paVaAqr83XWtuq6H/PeJAW/36mXhmqeIk7MDiJSXZCzlbS XePOaLwErF/I9wetq6kkpRU1nZ9FE4zQIUOgElg04QUBnxIEWMaCeyOgAWiYDMgGba5r CGZO6nK4ICYBABnNpgMHJy1NVGpmeB22SKnqw4P9lM7BsX5l5K1+M2xSPOjqMdjLBTSx eAC+NRtm5slJ+L9YvU6NZov8vkBilUbZ+HZ7K8TH28JSTewb0qpoVfA4fuaDaSTCSVOh fSVEyX2DoMEVRX9HS+pgkaZzzx0AgfeGeTKNnDvzs7VH5TQrST4RLKGqaNum1atSaIcb GDvg== X-Gm-Message-State: AJIora/8yQjmw9dMZQsGU63ZBMDAcP0s3RvxTyTN+OMmzvY1QVeoqesC 2dC2H/GDbnY7UKORA0qgtbUq7Ybsr69/aQ== X-Google-Smtp-Source: AGRyM1ssEhw8Bh16SGZgkHvWqWltvPsXtQKZM3tCne0bOKzU+8eQ58fGBWbVJ/wDAuXn3mrxevoaMQ== X-Received: by 2002:a05:600c:3c83:b0:39c:9039:852c with SMTP id bg3-20020a05600c3c8300b0039c9039852cmr2463498wmb.187.1656059821781; Fri, 24 Jun 2022 01:37:01 -0700 (PDT) Received: from smtpclient.apple ([2a02:3038:10:55be:c892:f305:5fb7:c6a8]) by smtp.gmail.com with ESMTPSA id az33-20020a05600c602100b003a03b4cb7cfsm1298452wmb.38.2022.06.24.01.37.01 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 24 Jun 2022 01:37:01 -0700 (PDT) Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable From: Richard Biener Mime-Version: 1.0 (1.0) Subject: Re: [PATCH 2/3] lto-plugin: make claim_file_handler thread-safe Date: Fri, 24 Jun 2022 10:37:00 +0200 Message-Id: References: Cc: Rainer Orth , "Joseph S. Myers" , Jakub Jelinek , GCC Patches In-Reply-To: To: =?utf-8?Q?Martin_Li=C5=A1ka?= X-Mailer: iPhone Mail (19F77) X-Spam-Status: No, score=-10.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Jun 2022 08:37:05 -0000 > Am 21.06.2022 um 10:43 schrieb Martin Li=C5=A1ka : >=20 > =EF=BB=BFOn 6/21/22 09:56, Richard Biener wrote: >>> On Mon, Jun 20, 2022 at 12:20 PM Martin Li=C5=A1ka wrot= e: >>>=20 >>> On 6/20/22 11:32, Richard Biener wrote: >>>> On Thu, Jun 16, 2022 at 9:01 AM Martin Li=C5=A1ka wrot= e: >>>>>=20 >>>>> lto-plugin/ChangeLog: >>>>>=20 >>>>> * 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(-) >>>>>=20 >>>>> 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 n= ot see >>>>> #include >>>>> #include >>>>> #include >>>>> +#include >>>>=20 >>>> Not sure if we support any non-pthread target for building the LTO >>>> plugin, but it >>>> seems we have >>>>=20 >>>> # 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=3D$enable_lto ;; >>>>=20 >>>> which suggests that at least build validating the above with --enable-l= to >>>=20 >>> Verified that it's fine. >>>=20 >>>>=20 >>>> IIRC we have gthr-*.h in libgcc/, not sure if that's usable in a >>>> host linker plugin. >>>>=20 >>>>> #ifdef HAVE_SYS_WAIT_H >>>>> #include >>>>> #endif >>>>> @@ -157,6 +158,9 @@ enum symbol_style >>>>> ss_uscore, /* Underscore prefix all symbols. */ >>>>> }; >>>>>=20 >>>>> +/* 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_inp= ut_file *file, int *claimed) >>>>> lto_file.symtab.syms); >>>>> check (status =3D=3D LDPS_OK, LDPL_FATAL, "could not add symbols= "); >>>>>=20 >>>>> + pthread_mutex_lock (&plugin_lock); >>>>> num_claimed_files++; >>>>> claimed_files =3D >>>>> xrealloc (claimed_files, >>>>> num_claimed_files * sizeof (struct plugin_file_info))= ; >>>>> claimed_files[num_claimed_files - 1] =3D lto_file; >>>>> + pthread_mutex_unlock (&plugin_lock); >>>>>=20 >>>>> *claimed =3D 1; >>>>> } >>>>>=20 >>>>> + pthread_mutex_lock (&plugin_lock); >>>>> if (offload_files =3D=3D NULL) >>>>> { >>>>> /* Add dummy item to the start of the list. */ >>>>> @@ -1333,11 +1340,12 @@ claim_file_handler (const struct ld_plugin_inp= ut_file *file, int *claimed) >>>>> offload_files_last_lto =3D ofld; >>>>> num_offload_files++; >>>>> } >>>>> + pthread_mutex_unlock (&plugin_lock); >>>>>=20 >>>>> goto cleanup; >>>>>=20 >>>>> err: >>>>> - non_claimed_files++; >>>>> + __atomic_fetch_add (&non_claimed_files, 1, __ATOMIC_RELAXED); >>>>=20 >>>> is it worth "optimizing" this with yet another need for target specific= support >>>> (just use pthread_mutex here as well?) >>>=20 >>> Sure. >>>=20 >>> May I install the patch with the change? >>=20 >> 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_H= EADER >=20 > All right, let's error out then. >=20 >> for the gthr.h stuff using $target_thread_file (aka --enable-threads=3DXY= Z), >> but as said that's for the target and I don't see any host uses. We migh= t also >> add an explicit list of hosts (*-linux*?) where we enable thread support f= or >> lto-plugin, providing opt-in (so you'd have to wrap the mutex taking or >> if-def it out). >>=20 >> I think you also need to link lto-plugin with -pthread, no? >=20 > Yep. >=20 > Please see the updated patch. >=20 Please use -pthread instead of -lpthread=20 Otherwise looks OK to me. >> On linux >> it might work omitting that but I'm not sure other libc have serial pthre= ad >> 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). >=20 > What initializing threads do you mean? It=E2=80=99s target dependent what kind of global state init need to be done= when any pthread facility is used. Richard=20 > Martin >=20 >>=20 >> Richard. >>=20 >>> Cheers, >>> Martin >>>=20 >>>>=20 >>>>> free (lto_file.name); >>>>>=20 >>>>> cleanup: >>>>> @@ -1415,6 +1423,12 @@ onload (struct ld_plugin_tv *tv) >>>>> struct ld_plugin_tv *p; >>>>> enum ld_plugin_status status; >>>>>=20 >>>>> + if (pthread_mutex_init (&plugin_lock, NULL) !=3D 0) >>>>> + { >>>>> + fprintf (stderr, "mutex init failed\n"); >>>>> + abort (); >>>>> + } >>>>> + >>>>> p =3D tv; >>>>> while (p->tv_tag) >>>>> { >>>>> -- >>>>> 2.36.1 >>>>>=20 >>>>>=20 > <0001-lto-plugin-make-claim_file_handler-thread-safe.patch>