On Mon, Jun 06, 2022 at 05:13:24PM +0200, Mohamed Sayed via Gcc-patches wrote:
> > 2022-06-06 Mohamed Sayed <[email protected]>
> >
> >
> > * 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
