Re: [PATCH]: libgompd add parallel handle functions

[Jakub Jelinek <jakub@redhat.com>]

On Mon, Jun 06, 2022 at 05:13:24PM +0200, Mohamed Sayed via Gcc-patches wrote:
> > 2022-06-06  Mohamed Sayed  <mohamedsayed22198@gmail.com>
> >
> >
> > * Makefile.am: (libgompd_la_SOURCES): Add ompd-parallel.c.

The ChangeLog formatting is wrong.  The above line should be:
	* Makefile.am (libgompd_la_SOURCES): Add ompd-parallel.c.
i.e. tab, * space filename and if there is something in the
file that is being modified, followed by space ( name )
All this then followed by : and description.

> > * Makefile.in: Regenerate.
> > * libgompd.map: Add ompd_get_curr_parallel_handle,
> > ompd_get_enclosing_parallel_handle, ompd_rel_parallel_handle
> > and ompd_parallel_handle_compare symbol versions.

We aren't very consistent on the map files, either it should be
	* libgompd.map (ompd_get_curr_parallel_handle,
	ompd_get_enclosing_parallel_handle, ompd_rel_parallel_handle,
	ompd_parallel_handle_compare): Export at OMPD_5.1.
or
	* libgompd.map (OMPD_5.1): Export ompd_get_curr_parallel_handle,
	ompd_get_enclosing_parallel_handle, ompd_rel_parallel_handle and
	ompd_parallel_handle_compare.

> > * ompd-support.h:() : Add gompd_access (gomp_team_state, team) and
> > gompd_access (gomp_team, prev_ts).

This is totally wrong.
You are modifying the GOMPD_FOREACH_ACCESS macro I think.
So you should say
	* ompd-support.h (GOMPD_FOREACH_ACCESS): Add
	gompd_access (gomp_team_state, team) and
	gompd_access (gomp_team, prev_ts).

> +    return ompd_rc_stale_handle;
> +
> +  CHECK (parallel_handle->ah);
> +
> +  ompd_address_space_context_t *context = parallel_handle->ah->context;  
> +  ompd_address_t symbol_address = parallel_handle->th;
> +  ompd_address_t temp_symbol_address = {OMPD_SEGMENT_UNSPECIFIED, 0};
> +  ompd_word_t temp_offset;
> +  ompd_rc_t ret;
> +
> +  /* get prev_ts offset.  */
> +  GET_VALUE (context, NULL, "gompd_access_gomp_team_prev_ts",
> +  	     temp_offset, temp_offset, target_sizes.sizeof_long_long, 1, ret,
> +             temp_symbol_address);

Formatting.  You can see the two lines are badly indented, each one in
a different wrong way.  The indenting rule is that the lines after the first
one in this case should start aligned below context on the GET_VALUE
line, and each 8 spaces should be replaced by a tab.

> +
> +  symbol_address.address += temp_offset;
> +
> +  /* get team offset.  */
> +  GET_VALUE (context, NULL, "gompd_access_gomp_team_state_team",
> +  	     temp_offset, temp_offset, target_sizes.sizeof_long_long, 1, ret,
> +             temp_symbol_address);

Likewise.
> +
> +  symbol_address.address += temp_offset;
> +
> +  ret = callbacks->read_memory (context, NULL, &symbol_address,
> +                                target_sizes.sizeof_pointer,
> +                                &symbol_address.address);
> +  CHECK_RET (ret); 
> +  ret = callbacks->alloc_memory (sizeof (ompd_parallel_handle_t),
> +                                 (void **) (enc_par_handle));

Why the ()s around enc_par_handle?
Anyway, seems like an aliasing violation, if the callback stores
through void **, then you should just pass it address of a local
void * variable and *enc_par_handle = (ompd_parallel_handle_t) var;

> +ompd_rc_t
> +ompd_rel_parallel_handle (ompd_parallel_handle_t *parallel_handle)
> +{
> +  if (parallel_handle == NULL)
> +    return ompd_rc_stale_handle;
> +
> +  ompd_rc_t ret = callbacks->free_memory ((void *) (parallel_handle));

Likewise (both aliasing and the unnecessary ()s.
>  #define GOMPD_SIZES(gompd_size) \
>    gompd_size (gomp_thread) \
>    gompd_size (gomp_task_icv) \
> -  gompd_size (gomp_task)
> +  gompd_size (gomp_task) 
> +  

Why?

	Jakub