Re: [PATCH] libgomp : OMPD implementation

[Mohamed Atef <mohamedatef1698@gmail.com>]

Hi Jakub,
i will fix all the issues and i will use the script on the website.
On Mon, Mar 14, 2022 at 6:23 PM Jakub Jelinek <jakub@redhat.com> wrote:

> 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.
>
Never mind, thank you anyway.

>
> 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.
>
done.

> > --- 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
>
> do macros also should have a space between the name and the '('

> >  #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.
>
> from OpenMP specs.
"The OMPD library does not need to be reentrant. The tool must ensure that
only one thread enters
the OMPD library at a time. The OMPD library must not install signal
handlers or otherwise
interfere with the tool’s signal configuration."


> > +/*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?
>
I didn't find OMP_DEBUG in the list of environment variables.
 but fixed.

> 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
>
So should I remove them from the omp-tools.h file?

> 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.
>
> done, what about ompd_thread_bp_begin, and ompd_thread_bp_end : where
should i put them?

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