From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qv1-xf36.google.com (mail-qv1-xf36.google.com [IPv6:2607:f8b0:4864:20::f36]) by sourceware.org (Postfix) with ESMTPS id D379B3870870 for ; Tue, 21 Jun 2022 07:57:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D379B3870870 Received: by mail-qv1-xf36.google.com with SMTP id q4so7107413qvq.8 for ; Tue, 21 Jun 2022 00:57:06 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=bGDQ+wIfKkuxOFpK+mhk4ygvtrUhk2NfjhQgHikGo20=; b=wtgZFSqzDGGn/sf780qG3mNWD8rTcxkyT3nebxKea3QpzN8WwuoCMUofJJ5PBTXgnl eDefBTh88Zv7wIK4kM3HrD87BXYCI5inA6lgEx7D6qrCpCNV3uYaM+bbngoksdJMU71F 3sa5UuUl+Uhvq3304YTE8Pa/iWMFpwDwVeUrn9ha2bfjQ50enCFkv/f7HXVqL3nVURCK xj4XfdSS0fJ8k3vNMIfLf3UwGRK1qOXfw53rzrVLN3acO+RFsC42l/agoUyM7n0f2PSv NFTaLlvIC3UrwemK6Hg3nI8yGJeb/y6/iHkiNG4CSNsORb7lPGtOrPW7BTmzFURFmOcZ qlvw== X-Gm-Message-State: AJIora8kJY6izFUAtmSNfmmVPYwkRZUzBCEC3fGep+YTDaeBZDS4cmdV sOqYVzHQ30QC9+eUlMOfPTyhGM8hvBWmwjKIjEg= X-Google-Smtp-Source: AGRyM1ulsM2WHNDIkuDVCUZVZ96icMBcupLmzeaBMzLkJ/Xz+g3OoYBO2nECr7IZj8sNlTuzbckFUFhYorDdJyJUcNY= X-Received: by 2002:a05:6214:4117:b0:467:cc51:3f4d with SMTP id kc23-20020a056214411700b00467cc513f4dmr21851060qvb.40.1655798226310; Tue, 21 Jun 2022 00:57:06 -0700 (PDT) MIME-Version: 1.0 References: <803a0290-3909-b9c5-2461-b1740a00c63a@suse.cz> In-Reply-To: From: Richard Biener Date: Tue, 21 Jun 2022 09:56:55 +0200 Message-ID: Subject: Re: [PATCH 2/3] lto-plugin: make claim_file_handler thread-safe To: =?UTF-8?Q?Martin_Li=C5=A1ka?= , Rainer Orth , "Joseph S. Myers" Cc: Jakub Jelinek , GCC Patches Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-7.6 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: Tue, 21 Jun 2022 07:57:08 -0000 On Mon, Jun 20, 2022 at 12:20 PM Martin Li=C5=A1ka wrote: > > On 6/20/22 11:32, Richard Biener wrote: > > On Thu, Jun 16, 2022 at 9:01 AM Martin Li=C5=A1ka wrot= e: > >> > >> 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 > >> #include > >> #include > >> +#include > > > > 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=3D$enable_lto ;; > > > > which suggests that at least build validating the above with --enable-l= to > > 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 > >> #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_inp= ut_file *file, int *claimed) > >> lto_file.symtab.syms); > >> check (status =3D=3D LDPS_OK, LDPL_FATAL, "could not add symbol= s"); > >> > >> + 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); > >> > >> *claimed =3D 1; > >> } > >> > >> + 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); > >> > >> 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_HEA= DER for the gthr.h stuff using $target_thread_file (aka --enable-threads=3DXYZ)= , 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 fo= r 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) !=3D 0) > >> + { > >> + fprintf (stderr, "mutex init failed\n"); > >> + abort (); > >> + } > >> + > >> p =3D tv; > >> while (p->tv_tag) > >> { > >> -- > >> 2.36.1 > >> > >>