On Tue, Mar 15, 2022 at 2:21 AM Mohamed Atef <mohamedatef1...@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 <ja...@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 <mohamedatef1...@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 <mohamedatef1...@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 >> >>
