libgomp: Add OMPD Address Space Information functions.

This patch adds Address Space Information function implementations as
defined in section 5.5.4 of OpenMP API Specification 5.0.

2020-07-08  Tony Sim  <y2s1982@gmail.com>

libgomp/ChangeLog:

	* Makefile.am (libgompd_la_OBJECTS): Add ompd-addr.c.
	* Makefile.in: Regenerate.
	* ompd-addr.c: New file.

---
 libgomp/Makefile.am |  2 +-
 libgomp/Makefile.in |  5 +--
 libgomp/ompd-addr.c | 88 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 92 insertions(+), 3 deletions(-)
 create mode 100644 libgomp/ompd-addr.c

On Wed, Jul 08, 2020 at 03:30:35PM -0400, y2s1982 wrote:
> +ompd_rc_t
> +ompd_get_omp_version (ompd_address_space_handle_t *address_space,
> +		      ompd_word_t *omp_version)
> +{
> +  if (omp_version == NULL)
> +    return ompd_rc_bad_input;
> +  if (address_space == NULL)
> +    return ompd_rc_stale_handle;
> +
> +  /* _OPENMP macro is defined to have yyyymm integer.  */
> +  ompd_size_t macro_length = sizeof (int);
> +
> +  ompd_rc_t ret = ompd_rc_ok;
> +
> +  struct ompd_address_t addr;
> +  ret = gompd_callbacks.symbol_addr_lookup (address_space->context, NULL,
> +					    "openmp_version", &addr, NULL);

This can't be right.  There is no openmp_version variable in libgomp.so.1
(and I don't think we should add it).
As I said multiple times before, you should add one (read-only) data
variable to libgomp.so.1 that will encode a lot of information that OMPD
needs and the version should be in there.

> +ompd_rc_t
> +ompd_get_omp_version_string (ompd_address_space_handle_t *address_space,
> +			     const char **string)
> +{
> +  if (string == NULL)
> +    return ompd_rc_bad_input;
> +
> +  if (address_space == NULL)
> +    return ompd_rc_stale_handle;
> +
> +  ompd_word_t omp_version;
> +  ompd_rc_t ret = ompd_get_omp_version (address_space, &omp_version);
> +  if (ret != ompd_rc_ok)
> +    return ret;
> +
> +  char *tmp = "GNU OpenMP Runtime implementing OpenMP 5.0 "
> +	    ompd_stringify (omp_version);

This will append "omp_version" to the string literal, won't it?
That is not what you want in there, you instead want the value.

	Jakub

Hello Jakub,

Thank you again for detailed feedback. I had few questions.

On Wed, Jul 8, 2020 at 4:42 PM Jakub Jelinek <jakub@redhat.com> wrote:

> On Wed, Jul 08, 2020 at 03:30:35PM -0400, y2s1982 wrote:
> > +ompd_rc_t
> > +ompd_get_omp_version (ompd_address_space_handle_t *address_space,
> > +                   ompd_word_t *omp_version)
> > +{
> > +  if (omp_version == NULL)
> > +    return ompd_rc_bad_input;
> > +  if (address_space == NULL)
> > +    return ompd_rc_stale_handle;
> > +
> > +  /* _OPENMP macro is defined to have yyyymm integer.  */
> > +  ompd_size_t macro_length = sizeof (int);
> > +
> > +  ompd_rc_t ret = ompd_rc_ok;
> > +
> > +  struct ompd_address_t addr;
> > +  ret = gompd_callbacks.symbol_addr_lookup (address_space->context,
> NULL,
> > +                                         "openmp_version", &addr, NULL);
>
> This can't be right.  There is no openmp_version variable in libgomp.so.1
> (and I don't think we should add it).
> As I said multiple times before, you should add one (read-only) data
> variable to libgomp.so.1 that will encode a lot of information that OMPD
> needs and the version should be in there.
>

I do remember, though I obviously understood wrongly. Sorry about that.
I had assumed it might have something to do with ICV but didn't realize it
would also
apply to other variables. In all honesty, I was looking for _OPENMP macro;
I assumed
such information would be stored somewhere already and thought
symbol_addr_lookup() would find it somehow. I saw mentions of it on
ChangeLog,
testsuits, and in one string, but I couldn't find the actual macro. As for
openmp_version,
I (wrongly) made the assumption looking at the omp_lib.h.in. I should learn
more
about .in file's syntax and what they do.

To place a variable in libgomp.so.1, should I define a related struct and
declare a global
extern variable of the struct in omp.h and define it in some related .c
file?
Can I then simply use the name of the declared variable as the name (where
"openmp_version" currently  is) to find the struct? As for the value for
_OPENMP version,
where can I find it, or should OMPD maintain its own values for it?


> > +ompd_rc_t
> > +ompd_get_omp_version_string (ompd_address_space_handle_t *address_space,
> > +                          const char **string)
> > +{
> > +  if (string == NULL)
> > +    return ompd_rc_bad_input;
> > +
> > +  if (address_space == NULL)
> > +    return ompd_rc_stale_handle;
> > +
> > +  ompd_word_t omp_version;
> > +  ompd_rc_t ret = ompd_get_omp_version (address_space, &omp_version);
> > +  if (ret != ompd_rc_ok)
> > +    return ret;
> > +
> > +  char *tmp = "GNU OpenMP Runtime implementing OpenMP 5.0 "
> > +         ompd_stringify (omp_version);
>
> This will append "omp_version" to the string literal, won't it?
> That is not what you want in there, you instead want the value.
>

Oh I see. Thanks for pointing that out. I am still learning how macro
expansion works.

Cheers,

Tony

>
>         Jakub
>
>

On Wed, Jul 08, 2020 at 07:53:23PM -0400, y2s1982 . wrote:
> I do remember, though I obviously understood wrongly. Sorry about that.
> I had assumed it might have something to do with ICV but didn't realize it
> would also
> apply to other variables. In all honesty, I was looking for _OPENMP macro;
> I assumed
> such information would be stored somewhere already and thought
> symbol_addr_lookup() would find it somehow. I saw mentions of it on
> ChangeLog,
> testsuits, and in one string, but I couldn't find the actual macro. As for
> openmp_version,

openmp_version is a Fortran PARAMETER, which is (for integral variables) something
like
enum { openmp_version = ... };
or C++ const int openmp_version = ...
if only non-ODR used, i.e. replaced by the value whenever used.

You can always readelf -Wa .libs/libgomp.so.1 or nm -D .libs/libgomp.so.1 to
see what is actually present and exported.

> I (wrongly) made the assumption looking at the omp_lib.h.in. I should learn
> more
> about .in file's syntax and what they do.
> 
> To place a variable in libgomp.so.1, should I define a related struct and
> declare a global
> extern variable of the struct in omp.h and define it in some related .c
> file?

Definitely not in omp.h, that header is for OpenMP users.  Private
implementation stuff should not be there.  But it can be in libgomp.h.

> Can I then simply use the name of the declared variable as the name (where
> "openmp_version" currently  is) to find the struct? As for the value for
> _OPENMP version,
> where can I find it, or should OMPD maintain its own values for it?

_OPENMP is a predefined macro by C/C++ (and Fortran uses that omp_lib.f90
file).  And it is only predefined if preprocessing with -fopenmp.
I guess you can define some private structure, especially for the header of
that variable (where you add perhaps some magic 32-bit number, some version
number of the layout of the variable, this _OPENMP version, size of the
whole variable and others as needed).  What I'd suggest is that it then
contains some array of self-describing further values once you'll need it.
And it should be compact if possible (ideal would be e.g. sleb128/uleb128
encoding of numbers like in DWARF; e.g. one way to do it compact would be
have a generator program that would include omp.h and libgomp.h and stdio.h,
would use offsetof/sizeof and whatever and write it on stdout as initializer
of a const char GOMP_library_description[] = { 0x12, 0x7a, 0x2b, ... };
which then would be compiled and linked into libgomp.so.1.
And that you don't try to read data from that variable all the time, but
instead do it once from ompd_process_init, where you'd look up that
variable, read and parse that variable, store what you found into the
process handle and punt if anything is unexpected in there.
Once you need more information in there, it would be nice to macroize
that info, e.g. have a macro when you need size of some internal structure,
or offsetof of its member, or size of the member, or when some pointer needs
to be dereferenced to achieve something.

But perhaps for start just use a struct and parse the information from
there, and only when we have clearer picture on what kind of information we
need we can extend it.

	Jakub