Re: [PATCH] libgomp : OMPD implementation

[Jakub Jelinek <jakub@redhat.com>]

Hi!

Sorry for the delay, GCC is currently in stage 4, which means a lot of us
concentrate on fixing GCC 12 so that it can be released soon and projects
that are clearly GCC 13 material are much lower priority.

On Wed, Feb 16, 2022 at 11:04:13PM +0200, Mohamed Atef via Gcc-patches wrote:
> --- a/libgomp/ChangeLog
> +++ b/libgomp/ChangeLog
> @@ -1,3 +1,30 @@

ChangeLog entries should be posted in the patches and eventually should be
in the commit message, but the patch shouldn't change ChangeLog files
directly, we have nightly scripts that handle that.

> +2022-02-16  Mohamed Atef  <mohamedatef1698@gmail.com>
> +
> +        * Makefile.am (toolexeclib_LTLIBRARIES): Add libgompd.la.

For ChangeLog entries, https://gcc.gnu.org/contribute.html specifies
how it should be formatted.
E.g. there should be a tab instead of 8 spaces at the start of the lines
(except for the date/name/email line), the above has spaces.

> +        (libgompd_la_LDFLAGS, libgompd_la_DEPENDENCIES, libgompd_la_LINK,
> +        libgompd_la_SOURCES, libgompd_version_dep, libgompd_version_script,
> +        libgompd.ver-sun, libgompd.ver, libgompd_version_info): Defined. 

No trailing whitespace.  I think it would be better to use : New.

> +        * Makefile.in: Regenerate.
> +        * aclocal.m4: Regenerate.
> +        * config/darwin/plugin-suffix.h: Removed ().
> +        * config/hpux/plugin-suffix.h: Removed ().
> +        * config/posix/plugin-suffix.h: Removed ().

These don't say what has changed, and the tense is wrong.  So should be
like:
	* config/posix/plugin-suffix.h (SONAME_SUFFIX): Remove ()s.
or so.

> +        * configure: Regenerate.
> +        * env.c: (#include "ompd-support.h") : Added.

The above correctly mentions just the filename and not a particular thing
in it for the added include, but the rest is incorreclty formatted.
Should be:
	* env.c: Include ompd-support.h
or so.

> +        (initialize_env) : Call ompd_load().

There should be no space in between ) and :.  Just say
	(initialize_env): Call ompd_load.
No need for the ()s in there.

> +        * parallel.c:(#include "ompd-support.h"): Added.

See above.

> +        (GOMP_parallel) : Call ompd_bp_parallel_begin and ompd_bp_parallel_end.
> +        * libgomp.map: Add OMP_5.0.3 symobl versions.
> +        * libgompd.map: New file.
> +        * omp-tools.h.in : New file.
> +        * omp-types.h.in : New file.
> +        * ompd-support.h : New file.
> +        * ompd-support.c : New file.
> +        * ompd-helper.h : New file.

See above for the superfluous spaces before :.

> +        * ompd-helper.c: New file.
> +        * ompd-init.c: New file.
> +        * testsuite/Makfile.in: Regenerate.

Typo, Makefile.in instead of Makfile.in (the checking script would prevent
commit because of this).

> --- a/libgomp/Makefile.am
> +++ b/libgomp/Makefile.am
...
> +libgompd_version_info = -version-info $(libtool_VERSION)
>  libgomp_la_LDFLAGS = $(libgomp_version_info) $(libgomp_version_script) \
>          $(lt_host_flags)
> +libgompd_la_LDFLAGS = $(libgompd_version_info) $(libgompd_version_script) \
> +				$(lt_host_flags)
>  libgomp_la_DEPENDENCIES = $(libgomp_version_dep)
> +libgompd_la_DEPENDENCIES = $(libgompd_version_dep)
>  libgomp_la_LINK = $(LINK) $(libgomp_la_LDFLAGS)
> -
> +libgompd_la_LINK = $(LINK) $(libgompd_la_LDFLAGS)

Please preserve the empty line above libgomp_la_SOURCES.

> --- a/libgomp/Makefile.in
> +++ b/libgomp/Makefile.in
> @@ -1,7 +1,7 @@
> -# Makefile.in generated by automake 1.15.1 from Makefile.am.
> +# Makefile.in generated by automake 1.16.1 from Makefile.am.

Please make sure you regenerate files with the exact autoconf/automake
versions as used previously, we don't want the generated files
jump backwards/forwards whenever somebody regenerates them.
You can e.g. build automake 1.15.1 and autoconf 2.69 if you don't have
those, make install DESTDIR=/your/home/automake-1.15.1 etc.
and then just regenerate with those in $PATH.
> --- a/libgomp/env.c
> +++ b/libgomp/env.c
> @@ -1483,6 +1484,8 @@ initialize_env (void)
>  	= thread_limit_var > INT_MAX ? UINT_MAX : thread_limit_var;
>      }
>    parse_int_secure ("GOMP_DEBUG", &gomp_debug_var, true);
> +  if(gomp_debug_var)
> +  		ompd_load();

Formatting.  Again, https://gcc.gnu.org/contribute.html contains
the rules or just watch how does surrounding code look like.
The errors are:
1) space in between if and ( is missing
2) ompd_load should be indented 2 further columns from if (and
   any consecutive 8 spaces at the start of lines replaced with
   tab in *.c/*.C files)
3) space between ompd_load and ( is missing

>  #ifndef HAVE_SYNC_BUILTINS
>    gomp_mutex_init (&gomp_managed_threads_lock);
>  #endif
> diff --git a/libgomp/libgomp.map b/libgomp/libgomp.map
> index 2ac58094169..5c57b1a2d08 100644
> --- a/libgomp/libgomp.map
> +++ b/libgomp/libgomp.map
> @@ -226,6 +226,16 @@ OMP_5.1 {
>  	omp_get_teams_thread_limit_;
>  } OMP_5.0.2;
>  
> +OMP_5.0.3 {
> +  global:
> +  	ompd_dll_locations;
> +  	ompd_dll_locations_valid;
> +  	ompd_bp_parallel_begin;
> +  	ompd_bp_parallel_end;
> +  local:
> +  	*;
> +}OMP_5.0.2;

Again, please watch how the surrounding code looks like.
The first 2 added lines are correct, the next 4 have 2 spaces
followed by tab when they should start with just tab, local: *; lines
shouldn't be there, that is just in the very first symver and nowhere else.
And there should be a space between } and OMP_5.0.2;

> --- /dev/null
> +++ b/libgomp/libgompd.map
> @@ -0,0 +1,12 @@
> +OMPD_5.1 {
> +  global:
> +	ompd_initialize;
> +	ompd_get_api_version;
> +	ompd_get_version_string;
> +	ompd_process_initialize;
> +	ompd_device_initialize;
> +	ompd_rel_address_space_handle;
> +	ompd_finalize;
> +  local:
> +  	*;
> +};

The *; local has incorrect indentation, the rest is ok.

> --- /dev/null
> +++ b/libgomp/omp-tools.h.in

> +void ompd_dll_locations_valid(void);

Space before (.
> +
> +typedef __UINT64_TYPE__ ompd_size_t;
> +typedef __UINT64_TYPE__ ompd_wait_id_t;
> +
> +typedef __UINT64_TYPE__ ompd_addr_t;
> +typedef __INT64_TYPE__ ompd_word_t;
> +typedef __UINT64_TYPE__ ompd_seg_t;
> +
> +typedef struct ompd_address_t
> +{
> +	ompd_seg_t segment;
> +	ompd_addr_t address;

These should be indented by 2 spaces instead of a tab.
> +} ompd_address_t;
> +
> +typedef struct ompd_frame_info_t
> +{
> +	ompd_address_t frame_address;
> +	ompd_word_t frame_flag;

Ditto.

> +typedef struct _ompd_task_handle ompd_task_handle_t;
> +
> +typedef enum ompd_scope_t
> +{
> +	ompd_scope_global		    = 1,
> +	ompd_scope_address_space = 2,
> +	ompd_scope_thread		    = 3,
> +	ompd_scope_parallel		 = 4,
> +	ompd_scope_implicit_task = 5,
> +	ompd_scope_task          = 6

Likewise.  And just space = space before the values.
Several times in the file.

> +
> +typedef ompd_rc_t (*ompd_callback_get_thread_context_for_thread_id_fn_t) 

No trailing whitespace.

> +	(ompd_address_space_context_t *, ompd_thread_id_t,
> +	ompd_size_t, const void *, ompd_thread_context_t **);

I'd indent the ( just by 2 spaces, and ompd_size_t should be indented
below ompd_address_space_context_t, i.e. not below (.
Again many times.
> +
> +#endif /* _OMP_TOOLS_H */
> \ No newline at end of file

Please make sure newly added files are newline terminated.

> diff --git a/libgomp/ompd-helper.c b/libgomp/ompd-helper.c
> new file mode 100644
> index 00000000000..033990073a5
> --- /dev/null
> +++ b/libgomp/ompd-helper.c
> @@ -0,0 +1,47 @@
> +/* Copyright (C) 2021 Free Software Foundation, Inc.

It is 2022 now.

> +   <http://www.gnu.org/licenses/>.  */
> +
> +
> +
> +
> +

Please avoid the excessive vertical whitespace, 2 lines is ok,
more than that is not.

> +#include "ompd-helper.h"
> +
> +ompd_device_type_sizes_t target_sizes;
> +
> +ompd_rc_t
> +get_sizes(ompd_address_space_context_t *context)

Space before ( etc., incorrect indentation, what I said above.

> +{
> +	if(context == NULL)
> +		return ompd_rc_bad_input;

> +	static int inited = 0;

Just use bool and false (and include <stdbool.h> somewhere).
Also, can that be called in parallel by multiple threads?
If so, such inited checking is racy.

> +/*This file contains the implementation of functions defined in
> +	section 5.5.1, 5.5.2.  */

Space after /* missing, and section should be below This.

> +ompd_rc_t
> +ompd_initialize(ompd_word_t api_version,
> +	const ompd_callbacks_t *callbacks_table)

Space before ( and const ompd_ should be below ompd_word_t.

> +	static const char tmp_string[] = 

= should be on the next line already.

> +		"GNU OpenMP runtime implementing OMPD version "
> +			stringize(VERSION) " Debugging library.";

stringize (VERSION) should be below that first ", i.e.
  static const char tmp_string[]
    = "GNU OpenMP runtime implementing OMPD version "
      stringize (VERSION) " Debugging library";
The . at the end makes no sense, it isn't a sentence.

> +	//naive way to read from memory

We use just C comments in libgomp, missing capital at the start
and . at the end, so
  /* Native way to read from memory.  */

> +	ret = callbacks->symbol_addr_lookup(context, NULL, "ompd_state",
> +			&symbol_addr, NULL);

Arguments on next line should be below arguments on the first line, so
one column after (.
> +
> +	ret = callbacks->read_memory(context, NULL, &symbol_addr,
> +			target_sizes.sizeof_long_long, &ompd_state);
> +
> +	ret = callbacks->device_to_host(context, &ompd_state,
> +			target_sizes.sizeof_long_long, 1, &ompd_state);
> +
> +	ret = callbacks->alloc_memory(sizeof(ompd_address_space_handle_t),
> +			(void **)(handle));

Space between )(.  Also, shouldn't ret != ompd_rc_ok be checked after
every call?  Otherwise you only checkit from the last one.
> +	
> +
> +	if(ret != ompd_rc_ok)
> +		return ret;
> +
> +	if(handle == NULL)
> +		return ompd_rc_error;
> +
> +	(*handle)->context = context;
> +	(*handle)->kind = OMPD_DEVICE_KIND_HOST;
> +	return ret;
> +}
> +
> +
> +
> +/*OMPD will not support GPUs for now. */
> +
> +ompd_rc_t
> +ompd_device_initialize(ompd_address_space_handle_t *process_handle,
> +	ompd_address_space_context_t *device_context, ompd_device_t kind,
> +	ompd_size_t sizeof_id, void *id,
> +	ompd_address_space_handle_t **device_handle)
> +
> +{
> +	if(device_context == NULL)
> +		return ompd_rc_bad_input;
> +
> +	return ompd_rc_unsupported;
> +}
> +
> +
> +
> +ompd_rc_t
> +ompd_rel_address_space_handle(ompd_address_space_handle_t *handle)
> +{
> +	if(handle == NULL)
> +		return ompd_rc_stale_handle;
> +
> +	ompd_rc_t ret = callbacks->free_memory((void *)handle);
> +	return ret;
> +}
> diff --git a/libgomp/ompd-support.c b/libgomp/ompd-support.c
> new file mode 100644
> index 00000000000..8458e1f5d5f
> --- /dev/null
> +++ b/libgomp/ompd-support.c
> @@ -0,0 +1,108 @@
> +/* Copyright (C) 2021 Free Software Foundation, Inc.
> +   Contributed by Mohamed Atef <mohamedatef1698@gmail.com>.
> +   This file is part of the GNU Offloading and Multi Processing Library
> +   (libgomp).
> +   Libgomp is free software; you can redistribute it and/or modify it
> +   under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3, or (at your option)
> +   any later version.
> +   Libgomp is distributed in the hope that it will be useful, but WITHOUT ANY
> +   WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
> +   FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> +   more details.
> +   Under Section 7 of GPL version 3, you are granted additional
> +   permissions described in the GCC Runtime Library Exception, version
> +   3.1, as published by the Free Software Foundation.
> +   You should have received a copy of the GNU General Public License and
> +   a copy of the GCC Runtime Library Exception along with this program;
> +   see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include "ompd-support.h"
> +
> +
> +
> +const char **ompd_dll_locations = NULL;
> +__UINT64_TYPE__ ompd_state;
> +
> +void 
> +ompd_load()
> +{
> +	static int ompd_initialized = 0;
> +	if(ompd_initialized)
> +		return;
> +	fprintf(stderr, "OMP OMPD active\n");

I think you don't want such printouts, perhaps use gomp_debug so that
it is only printed if user asks for it.
Also, ompd_load is not a symbol specified in the standard, so we should
call it differently, the omp_* and ompd_* prefixes should be reserved
for symbols from the standard.
And, now I see you call ompd_load guarded with gomp_debug_var, shouldn't
that be guarded with a var set from parsing "OMP_DEBUG" env var rather
than the implementation specific "GOMP_DEBUG" which means something
completely else?
Why do you need that ompd_initialized stuff?  env.c calls it just once.

> +	static const char *tmp_ompd_dll_locations[2] 
> +		= {"libgompd" SONAME_SUFFIX(1) , NULL};

No space before , and = should be 2 columns to the right from static
which should be 2 columns to the right from { (this is everywhere in the
patch).

> +	ompd_state |= OMPD_ENABLED;
> +	ompd_initialized = 1;
> +	ompd_dll_locations = (const char **)malloc(2 * sizeof(const char *));

If you'd need to actually allocate something, you'd want to use
gomp_malloc and similar APIs that will fail miserably if memory can't be
allocated.  But another option is just to have a static 2 elements array
that holds it and just set ompd_dll_locations to the address of that array's
first element.  After all, you clearly have one like that,
just don't call it tmp_ompd_dll_locations but say ompd_dll_locations_array
or something similar.  If you for whatever reason need to allocate it
from heap, on the other side don't make that array static, we really want
as few dynamic relocations in the binary and as small .data and .rodata
as possible.

> +void __attribute__ ((noinline))
> +ompd_dll_locations_valid()
> +{

I'd strongly prefer these not to be separate functions at least
on the common platforms.  At least when they are called just once
instead of multiple times.
So have in the headers some macros that depending on e.g. whether
it is __ELF__ define those as:
#ifdef __ELF__
#define ompd_dll_locations_valid() \
  __asm__ __volatile__ (".globl ompd_dll_locations_valid\n\t"
			"ompd_dll_locations_valid:");
#else
extern void ompd_dll_locations_valid (void);
#endif
and use these out of line functions only if the macros aren't defined.
> @@ -173,10 +174,14 @@ GOMP_parallel (void (*fn) (void *), void *data, unsigned num_threads,
>  	       unsigned int flags)
>  {
>    num_threads = gomp_resolve_num_threads (num_threads, 0);
> +  if(ompd_state)
> +    ompd_bp_parallel_begin();

This shouldn't be conditional on anything (especially if it is just asm
volatile with a label rather than a function call, because it doesn't cost
anything in that case except for the (required) .dynsym entry).

But, I think this spot is also not the best one, because it isn't the only
one invoked for #pragma omp parallel.  GOMP_parallel_loop_static_start,
GOMP_parallel_loop_nonmonotonic_guided etc., there are many.
Better to stick it into gomp_team_start and gomp_team_end.

Please adjust your other patch with all the issues raised above too.

	Jakub