Re: [PATCH] libgomp : OMPD implementation

[Mohamed Atef <mohamedatef1698@gmail.com>]

On Tue, Mar 15, 2022 at 2:21 AM Mohamed Atef <mohamedatef1698@gmail.com>
wrote:

> 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
>>
>> >  #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?
>
    or should i add this definition  to the omp-tools.h file as it has the
standard definitions?

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