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