On Tue, Jun 23, 2020 at 07:25:22PM -0400, y2s1982 via Gcc-patches wrote:
> This patch adds some unit tests for omp-tools.h header. It also adds some
> simple
> functions related to OMPD API versions. It also partially defines the OMPD
> initialization function.
>
> 2020-06-23 Tony Sim <y2s1...@gmail.com>
>
> libgomp/ChangeLog:
>
> * Makefile.am (toolexeclib_LTLIBRARIES and related): Add libgompd.la.
Please spell out all the changes. I.e.
* Makefile.am (toolexeclib_LTLIBRARIES): Add libgompd.la.
(libgompd_la_LDFLAGS, libgompd_la_DEPENDENCIES, libgompd_la_LINK,
libgompd_la_SOURCES): Set.
> @@ -67,6 +67,12 @@ libgomp_la_SOURCES = alloc.c atomic.c barrier.c critical.c
> env.c error.c \
> oacc-mem.c oacc-async.c oacc-plugin.c oacc-cuda.c priority_queue.c \
> affinity-fmt.c teams.c allocator.c oacc-profiling.c oacc-target.c
>
> +libgompd_la_LDFLAGS = $(libgomp_version_info) $(libgomp_version_script) \
> + $(lt_host_flags)
> +libgompd_la_DEPENDENCIES = $(libgomp_version_dep)
> +libgompd_la_LINK = $(LINK) $(libgomp_la_LDFLAGS)
You actually want to use libgompd_la_LDFLAGS, libgompd_version_dep,
libgompd_version_script, libgompd_version_info etc. (and set those variables
next to the libgomp ones).
And introduce libgompd.map version script which will have the exported
symbols in for now OMPD_5.0 symbol version.
So
OMPD_5.0 {
global:
...;
local:
*;
};
> +libgompd_la_SOURCES = libgompd.c
Not sure if you want to call the source file libgompd.c, that would make
sense only if you want to have all of OMPD implemented in a single file.
If you need multiple, I'd suggest ompd-*.c where the * would be something
short/descriptive to name a set of related functions.
> +++ b/libgomp/libgompd.c
> @@ -0,0 +1,46 @@
> +#include <string.h>
> +#include <stdlib.h>
> +#include "omp-tools.h"
> +#include "libgompd.h"
> +//#include "plugin-suffix.h"
So, what actually includes that header? Otherwise it can't compile.
> +
> +ompd_rc_t ompd_get_api_version(ompd_word_t *version)
> +{
> + *version = OMPD_VERSION;
> + return ompd_rc_ok;
> +}
Formatting. The GNU Coding Conventions say it should be
ompd_rc_t
ompd_get_api_version (ompd_word_t *version)
{
*version = OMPD_VERSION;
return ompd_rc_ok;
}
(i.e. function name should be at the start of line for easy grepping,
there should be a single space before (, no space after the ( or
before ) as you have in other functions, indentation level is 2
(see indent program defaults).
> +
> +ompd_rc_t ompd_get_version_string(const char **string)
> +{
> + string = str(OMPD_VERSION);
> + return ompd_rc_ok;
> +}
This doesn't do anything outside of the function. You need
*string =
and put there something more descriptive than just the version,
perhaps
*string = "GNU OpenMP Runtime implementing OpenMP 5.0 " str (OMPD_VERSION);
I don't see a str macro defined, and it should have a different name, str is
too generic.
libgomp.h defines (conditionally):
# define ialias_str1(x) ialias_str2(x)
# define ialias_str2(x) #x
which does what you want, but you need it unconditionally and call it some
other way (stringify and stringify1?, define right above the function?).
> +
> +ompd_rc_t ompd_initialize ( ompd_word_t api_version, const ompd_callbacks_t
> *callbacks )
> +{
> + /* initialized flag */
> + static int ompd_initialized = 0;
> +
> + if (ompd_initialized)
> + return ompd_rc_error;
> +
> + /* compute library name and locations */
> + const char *prefix = "libgompd.";
> + const char *suffix = SONAME_SUFFIX (1);
> + size_t prefix_len, suffix_len;
> + prefix_len = strlen(prefix);
> + suffix_len = strlen(suffix);
Formatting (in addition to what has been said above, e.g. space before (.
But, this doesn't really belong here anyway, ompd_dll_locations needs to be
initialized not in ompd_initialize, but far before that.
Either it should be just a const variable, which I think with
const char *ompd_dll_locations[2] = { "libgompd" SONAME_SUFFIX (1), NULL };
is possible, or some library constructor needs to initializa it and then
pass through (or call) the mandated breakpoint so that the debugger can be
sure it is initialized.
Then the debugger will use the variable, load the libgompd library and only
after that actually call ompd_initialize.
> --- /dev/null
> +++ b/libgomp/testsuite/libgomp.ompd/header-1.c
> @@ -0,0 +1,6 @@
> +/* Test that the omp-tools.h will compile successfully. */
> +
> +/* { dg-do run } */
Should be just dg-do compile, that is all you care about in this case.
> +#include "omp-tools.h"
> +
> +int main(void){ return 0; }
Please fix up formatting,
int
main ()
{
return 0;
}
Ditto other tests.
Jakub
