Re: [PATCH] libgompd: Add ompd_get/rel_display_control_vars

[Jakub Jelinek <jakub@redhat.com>]

On Fri, May 27, 2022 at 02:04:11AM +0200, Mohamed Atef wrote:
> libgomp/ChangeLog
> 
> 2022-05-27  Mohamed Atef  <mohamedatef1698@gmail.com>
> 
> * libgompd.map (ompd_get_display_control_vars,
> ompd_rel_display_control_vars): New global symbol versions.
> * env.c: (gompd_buffer, gompd_env_buff_size): New Variables.
> (dump_icvs): New function.
> (initialize_env): call dump_icvs.
> * ompd-icv.c: (ompd_get_display_control_vars): New function.
> (ompd_rel_display_control_vars): New function.

I don't like this solution, I know LLVM libomp does it this way,
but that adds complexity to the libgomp library to make it easier
for libgompd, and even worse further bloats .data section of
every OpenMP program even when debugging isn't enabled (and not just a tiny
bit, but by sizeof (char *) + sizeof (unsigned long) + 500 bytes, which is
significant).

This really should be done on the libgompd side, instead of copying
the gompd_buffer you've added, it should instead copy
gomp_global_icv and whatever else is necessary to print it, then
do that in libgompd.

> diff --git a/libgomp/env.c b/libgomp/env.c
> index 243c6267ef9..173c9271303 100644
> --- a/libgomp/env.c
> +++ b/libgomp/env.c
> @@ -91,6 +91,8 @@ unsigned long gomp_places_list_len;
>  uintptr_t gomp_def_allocator = omp_default_mem_alloc;
>  int gomp_debug_var;
>  int gompd_enabled;
> +char *gompd_buffer;
> +unsigned long gompd_env_buff_size;
>  unsigned int gomp_num_teams_var;
>  int gomp_nteams_var;
>  int gomp_teams_thread_limit_var;
> @@ -1453,6 +1455,187 @@ omp_display_env (int verbose)
>  }
>  ialias (omp_display_env)
>  
> +/* This function dumps all global ICVs into a buffer
> +   in the form "icv-name=icv-value\n", so that OMPD can read the
> +   buffer and display all icvs.  */
> +
> +static void
> +dump_icvs (void)
> +{
> +  static char temp_buffer[500];
> +  char temp_num_str[20];
> +  strcat (temp_buffer, "OMP_DYNAMIC=");
> +  strcat (temp_buffer, gomp_global_icv.dyn_var ? "TRUE\n" : "FALSE\n");
> +  strcat (temp_buffer, "OMP_NESTED=");
> +  strcat (temp_buffer,
> +          gomp_global_icv.max_active_levels_var > 1 ? "TRUE\n" : "FALSE\n");
> +  strcat (temp_buffer, "OMP_NUM_THREADS=");
> +  sprintf (temp_num_str, "%lu\n", gomp_global_icv.nthreads_var);
> +  strcat (temp_buffer, temp_num_str);
> +  strcat (temp_buffer, "OMP_SCHEDULE=");

This is terribly inefficient even when done that way on the libgompd side.
You really don't want every further strcat to walk all the already
previously stored bytes to find the end, and you don't want to hardcode
the size of the buffer, see
https://www.gnu.org/prep/standards/standards.html#Writing-Robust-Programs

So, the buffer should be dynamically allocated, and you should at all times
know how many bytes have been already stored.  One possibility is to
call a printing function twice, once with say NULL as the buffer which will
be a mode in which it just computes the length of each addition and just
returns the total length, then allocate and then pass non-NULL buffer
into which it will actually store it.
Or you could allocate keep some extra bytes in the buffer (longer than
say longest name of an env variable name, = char, largest representable
long long number, \n char plus a few bytes extra) and keep checking before
adding any var that is guaranteed to fit into those extra bytes whether
there are at least those number of bytes left in the buffer and reallocate
otherwise.  But for vars that can have arbitrarily long results (e.g. vars
with various lists), such checking needs to be done before adding any
partial result that can fit).

	Jakub