Hello,
Thank you for the tips. I have some follow up questions. :)
On Wed, Jun 24, 2020 at 5:25 AM Jakub Jelinek <ja...@redhat.com> wrote:
> > 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.
>
Okay. Thank you for the clarification.
> +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:
> *;
> };
>
Oh I wasn't sure how everything worked yet, so I thought to borrow libomp
scripts as a placeholder for now.
I will change them to libompd scripts.
As for mapping, could you point me to an example that has global: and
local: object as above?
>
> > +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.
>
Okay. I will start splitting them up.
>
> > +++ 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.
>
I forgot to uncomment that. I put that in there to see other compilation
errors. I am still having trouble generating compilation errors using make.
I tried creating a separate directory outside of the source, calling
../gcc/configure, and trying make libgomp.
It did not generate any output other than "make: Nothing to do be for
'../gcc/libgomp'". I wasn't sure what else to try.
> > +
> > +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).
>
Thank you. I will try harder to follow the formatting. When you say
indentation level, do you mean the number of spaces?
>
> > +
> > +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.
>
Thank you for the clarification. Should I change the declaration and put
the definition of ompd_dll_locations variable in the omp-tools.h?
>
> > --- /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.
>
Thank you.
>
> > +#include "omp-tools.h"
> > +
> > +int main(void){ return 0; }
>
> Please fix up formatting,
> int
> main ()
> {
> return 0;
> }
>
> Ditto other tests.
>
Okay.
>
> Jakub
>
> Thank you as always.
Cheers,
Tony Sim
