Re: [patch] libgompd: Add thread handles

[Jakub Jelinek <jakub@redhat.com>]

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 <ahmedsayedmousse@gmail.com>

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