From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 4160A382CBA4 for ; Tue, 7 Jun 2022 09:56:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 4160A382CBA4 Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-103-bObQJRDgNG6jv7UTBHH26Q-1; Tue, 07 Jun 2022 05:56:08 -0400 X-MC-Unique: bObQJRDgNG6jv7UTBHH26Q-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 00EA81C03364; Tue, 7 Jun 2022 09:56:08 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.11]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A9BE91415100; Tue, 7 Jun 2022 09:56:07 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.17.1/8.17.1) with ESMTPS id 2579u4dC2092091 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Tue, 7 Jun 2022 11:56:05 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 2579u46a2092090; Tue, 7 Jun 2022 11:56:04 +0200 Date: Tue, 7 Jun 2022 11:56:03 +0200 From: Jakub Jelinek To: Ahmed Sayed Mousse Cc: gcc-patches@gcc.gnu.org Subject: Re: [patch] libgompd: Add thread handles Message-ID: Reply-To: Jakub Jelinek References: MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.85 on 10.11.54.7 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-4.8 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_NONE, TXREP, T_FILL_THIS_FORM_SHORT, 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, 07 Jun 2022 09:56:12 -0000 On Tue, Jun 07, 2022 at 12:21:25AM +0200, Ahmed Sayed Mousse via Gcc-patches wrote: > This patch is the initial implementation of OpenMP-API specs book section > 20.5.5 with title "Thread Handles" > > libgomp/ChangeLog > > 2022-05-06 Ahmed Sayed Two spaces should separate the date and name and name and email. > > * Makefile.am (libgompd_la_SOURCES): Add ompd-threads.c. > > * Makefile.in: Regenerate. > No empty lines in between (and all ChangeLog lines start with a tab (I assume your mailer ate that). > * ompd-support.h ( gompd_thread_initial_tls_bias ): New Variable. No spaces after ( or before ) > * ompd-support.c ( gompd_thread_initial_tls_bias ): New Variable. > > ( gompd_load ): ( gompd_thread_initial_tls_bias ): Initialized with > &gomp_tls_data - pthread_self (). It is just gompd_load you are changing, so it should be: (gompd_load): Initialize gompd_thread_initial_tls_bias. or so. > --- a/libgomp/ompd-support.c > +++ b/libgomp/ompd-support.c > @@ -36,6 +36,10 @@ > const char **ompd_dll_locations = NULL; > __UINT64_TYPE__ gompd_state; > > +#if (defined HAVE_TLS || defined USE_EMUTLS) > +__UINT64_TYPE__ gompd_thread_initial_tls_bias; In reality it isn't these conditions, but #ifdef GOMP_NEEDS_THREAD_HANDLE that determines if there is a TLS bias possible. But the point of those gompd_sizeof* and gompd_access* vars was to make libgompd slightly more independent from the exact libgomp version, otherwise one could just use sizeof and offsetof values directly in libgompd. So, even using similar ifdefs on the libgompd side looks wrong, the var should be there unconditionally and just use some special value (e.g. -1 which isn't a possible TLS bias because the struct has some alignment requirements) to say that the TLS bias can't be used and one needs to use struct gomp_thread's handle member instead. Also, as I mentioned yesterday, using __UINT64_TYPE__ for everything is very vasteful, use the right type for each information. As for TLS bias, in reality it will be up to +- a few hundreds of bytes, worst case kilobytes, but in theory it could be on 64-bit targets even larger than 4GB, but on 32-bit arches it can't, so size_t would be the right type. Except I think the interfaces don't cover size_t size, but long would be a usable replacement (not the same thing size-wise on Windows, but Windows will always GOMP_NEEDS_THREAD_HANDLE). > +#endif > + > void > gompd_load (void) > { > @@ -61,7 +65,11 @@ gompd_load (void) > = (__UINT64_TYPE__) & (((struct gomp_thread *) NULL)->handle); > __UINT64_TYPE__ gompd_sizeof_gomp_thread_handle > = sizeof (((struct gomp_thread *) NULL)->handle); There is a preexisting bug above: #ifdef GOMP_NEEDS_THREAD_HANDLE __UINT64_TYPE__ gompd_access_gomp_thread_handle = (__UINT64_TYPE__) & (((struct gomp_thread *) NULL)->handle); __UINT64_TYPE__ gompd_sizeof_gomp_thread_handle = sizeof (((struct gomp_thread *) NULL)->handle); just defines automatic variables in the function and sets them to those values. They need to be global vars, ideally const initialized at file scope. But, as the field is sometimes present and sometimes it is not, I think best would be to initialize it to offsetof/sizeof #ifdef GOMP_NEEDS_THREAD_HANDLE and otherwise to 0 and 0. Then we even don't need a magic value or when TLS bias can't be used and instead always GET_VALUE of gompd_sizeof_gomp_thread_handle, if it is 0, then use TLS bias, otherwise load the handle. Again, comment more about the already committed patch now, besides trying to shrink the values from __UINT64_TYPE__ to probably short int and making them const and initialized at file scope initializers and using offsetof, there is a big question when do we expect OMPD to work. Seems the gompd_{sizeof,access}* symbols aren't exported from the library, so they are present (say on ELF) just in .symtab/.strtab sections and debug info. Those sections can be stripped or stripped to file, so that would mean OMPD would work only if the libgomp.so.1 library is not stripped or has separate debug info installed. Also, if one builds the library with LTO, I think the linker with the compiler will happily remove all those symbols, as nothing uses them. To fix this latter thing, one can just add __attribute__((used)) to all those vars. But if we want to make those work somehow even without debug info and .symtab/.strtab sections around, I think we want to force the symbols into .dynsym/.dynstr too (i.e. export in libgomp.map). Exporting dozens of such symbols would be quite costly though. So if we go that route, I think it would be best if we had just 1-2 of such variables with data for libgompd (probably 2 where one is const and can be in .rodata and the other for vars that might need changing). As most if not all of the const data can be represented in unsigned short, I think it should be an array of const unsigned short, with macros that say what each element means and those macros we'd just keep frozen as an unchangeable ABI between the libgomp and libgompd libraries. Ideally, [0] element of the array would be some kind of version which libgompd initialization can check and punt if the version is unexpected (in theory in the future libgompd could support multiple versions etc.). > + #elif (defined HAVE_TLS || defined USE_EMUTLS) > + gompd_thread_initial_tls_bias = (__UINT64_TYPE__) \ > + (&gomp_tls_data - pthread_self ()); I think you should add casts, (char *) &gomp_tls_data - (char *) pthread_self () But, sure, this difference is not constant and so needs to be in the other, .data variable if the above notes are incorporated into a later patch, for now just that the gompd_thread_initial_tls_bias var can't be const. > +ompd_rc_t > +ompd_get_thread_in_parallel (ompd_parallel_handle_t *parallel_handle, int > + thread_num, ompd_thread_handle_t **thread_handle) > +{ ... > + ompd_word_t team_size_var; > + ret = ompd_get_icv_from_scope ((void *) parallel_handle, ompd_scope_parallel, > + gompd_icv_team_size_var, &team_size_var); > + if (ret != ompd_rc_ok) > + return ret; > + if (thread_num < 0 || thread_num >= team_size_var) > + return ompd_rc_bad_input; Does ompd_get_icv_from_scope with gompd_icv_team_size_var DTRT? I.e. read the nthreads var from struct gomp_team if the parallel handle represents a parallel with corresponding struct gomp_team, otherwise (if it represents an implicit parallel, return 1)? See how omp_get_team_size (0) aka omp_get_num_threads () is implemented on the library side. Jakub