[PATCH] libgomp: Add helper functions for memory handling.

[PATCH] libgomp: Add helper functions for memory handling.

[y2s1982]

This patch adds few helper functions aims to improve readability of
fetching addresses, sizes, and values. It also proposes a syntax for
querying these information through the callback functions, similar to
that of LLVM implementation. The syntax format is <query type>_<varaible
name>, or <query type>_<variable type>_<member type>. '_' is the
delimiter between fields. '<query type>', as currently defined in the
enum, is either gompd_query_address or gompd_query_size: the first
handles address or offset queries while the second handles the size of
the variable/member. '<variable type>' refers to the variable type, and
'<member type>' refers to the data type of the member of the variable.
This code is incomplete: in particular, it currently lacks CUDA support,
as well as segment handling, and inlining of the functions.

This updated patch includes some bug fixes.

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

libgomp/ChangeLog:

	* Makefile.am (libgompd_la_SOURCES): Add ompd-helper.c.
	* Makefile.in: Regenerate.
	* libgompd.h (FOREACH_QUERYTYPE): Add macro for query types.
	(query_type): Add enum for query types.
	(gompd_getQueryStringSize): Add declaration.
	(gompd_getQueryString): Add declaration.
	(gompd_getAddress): Add declaration.
	(gompd_getSize): Add declaration.
	(gompd_getValue): Add declaration.
	(gompd_getVariableAddress): Add declaration.
	(gompd_getVariableSize): Add declaration.
	(gompd_getVariableValue): Add declaration.
	(gompd_getMemberAddress): Add declaration.
	(gompd_getMemberSize): Add declaration.
	(gompd_getMemberValue): Add declaration.
	* ompd-helper.c: New file.

---
 libgomp/Makefile.am   |   2 +-
 libgomp/Makefile.in   |   5 +-
 libgomp/libgompd.h    |  53 ++++++++++
 libgomp/ompd-helper.c | 222 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 279 insertions(+), 3 deletions(-)
 create mode 100644 libgomp/ompd-helper.c

diff --git a/libgomp/Makefile.am b/libgomp/Makefile.am
index fe0a92122ea..d126bc655fc 100644
--- a/libgomp/Makefile.am
+++ b/libgomp/Makefile.am
@@ -90,7 +90,7 @@ 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_SOURCES = ompd-lib.c ompd-proc.c
+libgompd_la_SOURCES = ompd-lib.c ompd-proc.c ompd-helper.c
 
 include $(top_srcdir)/plugin/Makefrag.am
 
diff --git a/libgomp/Makefile.in b/libgomp/Makefile.in
index f74d5f3ac8e..fdd488ca98e 100644
--- a/libgomp/Makefile.in
+++ b/libgomp/Makefile.in
@@ -235,7 +235,7 @@ am_libgomp_la_OBJECTS = alloc.lo atomic.lo barrier.lo critical.lo \
 	$(am__objects_1)
 libgomp_la_OBJECTS = $(am_libgomp_la_OBJECTS)
 libgompd_la_LIBADD =
-am_libgompd_la_OBJECTS = ompd-lib.lo ompd-proc.lo
+am_libgompd_la_OBJECTS = ompd-lib.lo ompd-proc.lo ompd-helper.lo
 libgompd_la_OBJECTS = $(am_libgompd_la_OBJECTS)
 AM_V_P = $(am__v_P_@AM_V@)
 am__v_P_ = $(am__v_P_@AM_DEFAULT_V@)
@@ -593,7 +593,7 @@ libgomp_la_SOURCES = alloc.c atomic.c barrier.c critical.c env.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 $(am__append_4)
-libgompd_la_SOURCES = ompd-lib.c ompd-proc.c
+libgompd_la_SOURCES = ompd-lib.c ompd-proc.c ompd-helper.c
 
 # Nvidia PTX OpenACC plugin.
 @PLUGIN_NVPTX_TRUE@libgomp_plugin_nvptx_version_info = -version-info $(libtool_VERSION)
@@ -817,6 +817,7 @@ distclean-compile:
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/oacc-plugin.Plo@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/oacc-profiling.Plo@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/oacc-target.Plo@am__quote@
+@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/ompd-helper.Plo@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/ompd-lib.Plo@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/ompd-proc.Plo@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/ordered.Plo@am__quote@
diff --git a/libgomp/libgompd.h b/libgomp/libgompd.h
index 495995e00d3..9d34803797a 100644
--- a/libgomp/libgompd.h
+++ b/libgomp/libgompd.h
@@ -30,12 +30,18 @@
 #define LIBGOMPD_H 1
 
 #include "omp-tools.h"
+#include <string.h>
 
 #define ompd_stringify(x) ompd_str2(x)
 #define ompd_str2(x) #x
 
 #define OMPD_VERSION 201811
 
+#define FOREACH_QUERYTYPE(TYPE)\
+	TYPE (gompd_query_address)\
+	TYPE (gompd_query_size)\
+
+
 extern ompd_callbacks_t gompd_callbacks;
 
 typedef struct _ompd_aspace_handle {
@@ -47,4 +53,51 @@ typedef struct _ompd_aspace_handle {
   ompd_size_t ref_count;
 } ompd_address_space_handle_t;
 
+typedef enum gompd_query_type {
+#define GENERATE_ENUM(ENUM) ENUM,
+  FOREACH_QUERYTYPE (GENERATE_ENUM)
+#undef GENERATE_ENUM
+} query_type;
+
+ompd_rc_t gompd_getQueryStringSize (size_t *, query_type, const char*,
+				    const char *);
+
+ompd_rc_t gompd_getQueryString (char **, query_type, const char*, const char *);
+
+ompd_rc_t gompd_getAddress (ompd_address_space_context_t *,
+			    ompd_thread_context_t *, ompd_address_t *,
+			    const char *, const char *, ompd_addr_t);
+
+ompd_rc_t gompd_getSize (ompd_address_space_context_t *,
+			 ompd_thread_context_t *, ompd_size_t *, const char *,
+			 const char *);
+
+ompd_rc_t gompd_getValue (ompd_address_space_context_t *,
+			  ompd_thread_context_t *, void *, ompd_address_t *,
+			  const char *, const char *);
+
+ompd_rc_t gompd_getVariableAddress (ompd_address_space_context_t *,
+				    ompd_thread_context_t *, ompd_address_t *,
+				    const char *, ompd_addr_t);
+
+ompd_rc_t gompd_getVariableSize (ompd_address_space_context_t *,
+				 ompd_thread_context_t *, ompd_size_t *,
+				 const char *);
+
+ompd_rc_t gompd_getVariableValue (ompd_address_space_context_t *,
+				  ompd_thread_context_t *,
+				  void *, ompd_address_t *, const char *);
+
+ompd_rc_t gompd_getMemberAddress (ompd_address_space_context_t *,
+				  ompd_thread_context_t *, ompd_address_t *,
+				  const char *, const char *, ompd_addr_t);
+
+ompd_rc_t gompd_getMemberSize (ompd_address_space_context_t *,
+			       ompd_thread_context_t *, ompd_size_t *,
+			       const char *, const char *);
+
+ompd_rc_t gompd_getMemberValue (ompd_address_space_context_t *,
+				ompd_thread_context_t *, void *,
+				ompd_address_t *, const char *, const char *);
+
 #endif /* LIBGOMPD_H */
diff --git a/libgomp/ompd-helper.c b/libgomp/ompd-helper.c
new file mode 100644
index 00000000000..3274a9acd43
--- /dev/null
+++ b/libgomp/ompd-helper.c
@@ -0,0 +1,222 @@
+/* Copyright (C) 2020 Free Software Foundation, Inc.
+   Contributed by Yoosuk Sim <y2s1982@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 PURoffsetE.  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/>.  */
+
+/* This file defines custom helper functions for OMPD functions.  */
+
+#include "libgompd.h"
+
+static const char *gompd_query_type_string[] = {
+#define GENERATE_STRING(STRING) ompd_stringify (STRING),
+  FOREACH_QUERYTYPE (GENERATE_STRING)
+#undef GENERATE_STRING
+};
+
+ompd_rc_t
+gompd_getQueryStringSize (size_t *size, query_type type,
+			  const char* variableType, const char *memberType)
+{
+  const char *sep = "_";
+  const char **typeString = &gompd_query_type_string[type];
+
+  *size = strlen (*typeString) + strlen (sep) + strlen (variableType);
+  if (memberType)
+    *size += strlen (sep) + strlen (memberType);
+
+  return ompd_rc_ok;
+}
+ompd_rc_t
+gompd_getQueryString (char **queryString, query_type type,
+		      const char* variableType, const char *memberType)
+{
+  if (!type || !variableType)
+    return ompd_rc_bad_input;
+
+  const char *sep = "_";
+  const char **typeString = &gompd_query_type_string[type];
+
+  size_t offset = 0;
+  strcpy (*queryString, *typeString);
+  offset += strlen (*typeString);
+  strcpy (*queryString + offset, sep);
+  offset += strlen (sep);
+  strcpy (*queryString + offset, variableType);
+  if (memberType)
+    {
+      offset += strlen (variableType);
+      strcpy (*queryString + offset, sep);
+      offset += strlen (sep);
+      strcpy (*queryString + offset, memberType);
+    }
+
+  return ompd_rc_ok;
+}
+
+ompd_rc_t
+gompd_getAddress (ompd_address_space_context_t *ac, ompd_thread_context_t *tc,
+		  ompd_address_t *addr, const char *variableType,
+		  const char *memberType, ompd_addr_t seg)
+{
+  if (!ac)
+    return ompd_rc_stale_handle;
+  ompd_rc_t ret = ompd_rc_ok;
+  char *queryString = NULL;
+  size_t querySize = 0;
+  ret = gompd_getQueryStringSize (&querySize, gompd_query_address, variableType,
+				  memberType);
+  if (ret != ompd_rc_ok)
+    return ret;
+  ret = gompd_callbacks.alloc_memory (querySize + 1, (void *) queryString);
+  if (ret != ompd_rc_ok)
+    return ret;
+  ret = gompd_getQueryString (&queryString, gompd_query_address, variableType,
+			      memberType);
+  if (ret != ompd_rc_ok)
+    return ret;
+
+  if (!memberType)
+    {
+      ret = gompd_callbacks.symbol_addr_lookup (ac, tc, queryString, addr,
+						NULL);
+      if (ret != ompd_rc_ok)
+	return ret;
+    }
+  else if (addr)
+    {
+      ompd_size_t offset;
+      ompd_address_t offsetAddr;
+      ret = gompd_callbacks.symbol_addr_lookup (ac, tc, queryString,
+						&offsetAddr, NULL);
+      if (ret != ompd_rc_ok)
+	return ret;
+      ret = gompd_callbacks.read_memory (ac, tc, &offsetAddr,
+					 sizeof (ompd_size_t),
+					 (void *) &offset);
+      if (ret != ompd_rc_ok)
+	return ret;
+      addr->address += offset;
+    }
+  else
+    {
+      return ompd_rc_bad_input;
+    }
+
+  return gompd_callbacks.free_memory (queryString);
+}
+
+ompd_rc_t
+gompd_getSize (ompd_address_space_context_t *ac, ompd_thread_context_t *tc,
+	       ompd_size_t *size, const char *variableType,
+	       const char *memberType)
+{
+  if (!ac)
+    return ompd_rc_stale_handle;
+  ompd_rc_t ret;
+  ompd_address_t sizeAddr;
+  char *queryString = NULL;
+  ret = gompd_getQueryString (&queryString, gompd_query_size, variableType,
+			      memberType);
+
+  if (ret != ompd_rc_ok)
+    return ret;
+
+  ret = gompd_callbacks.symbol_addr_lookup (ac, tc, queryString, &sizeAddr,
+					    NULL);
+  if (ret != ompd_rc_ok)
+    return ret;
+
+  return gompd_callbacks.read_memory (ac, tc, &sizeAddr, sizeof (ompd_size_t),
+				     (void *) size);
+}
+
+ompd_rc_t gompd_getValue (ompd_address_space_context_t *ac,
+			  ompd_thread_context_t *tc, void *value,
+			  ompd_address_t *addr,
+			  const char *variableType, const char *memberType)
+{
+  if (!ac)
+    return ompd_rc_stale_handle;
+  if (!addr)
+    return ompd_rc_bad_input;
+
+  ompd_rc_t ret;
+  ompd_size_t size;
+  ret = gompd_getSize (ac, tc, &size, variableType, memberType);
+  if (ret != ompd_rc_ok)
+    return ret;
+  return gompd_callbacks.read_memory (ac, tc, addr, size, value);
+}
+
+ompd_rc_t
+gompd_getVariableAddress (ompd_address_space_context_t *ac,
+			  ompd_thread_context_t *tc, ompd_address_t *addr,
+			  const char *variableType, ompd_addr_t seg)
+{
+  return gompd_getAddress (ac, tc, addr, variableType, NULL, seg);
+}
+
+ompd_rc_t
+gompd_getVariableSize (ompd_address_space_context_t *ac,
+		       ompd_thread_context_t * tc, ompd_size_t *size,
+		       const char *variableType)
+{
+  return gompd_getSize (ac, tc, size, variableType, NULL);
+}
+
+ompd_rc_t
+gompd_getVariableValue (ompd_address_space_context_t *ac,
+			ompd_thread_context_t *tc, void *value,
+			ompd_address_t *variableAddress,
+			const char *variableType)
+{
+  return gompd_getValue (ac, tc, value, variableAddress, variableType, NULL);
+}
+
+ompd_rc_t
+gompd_getMemberAddress (ompd_address_space_context_t *ac,
+			ompd_thread_context_t *tc, ompd_address_t *variableAddr,
+			const char *variableType, const char *memberType,
+			ompd_addr_t seg)
+{
+  if (!variableAddr)
+    return ompd_rc_bad_input;
+  return gompd_getAddress (ac, tc, variableAddr, variableType, memberType, seg);
+}
+
+ompd_rc_t
+gompd_getMemberSize (ompd_address_space_context_t *ac,
+		     ompd_thread_context_t *tc, ompd_size_t *size,
+		     const char *variableType, const char *memberType)
+{
+  return gompd_getSize (ac, tc, size, variableType, memberType);
+}
+
+ompd_rc_t
+gompd_getMemberValue (ompd_address_space_context_t *ac,
+		      ompd_thread_context_t *tc, void *value,
+		      ompd_address_t *addr, const char *variableType,
+		      const char *memberType)
+{
+  return gompd_getValue (ac, tc, value, addr, variableType, memberType);
+}
-- 
2.27.0

Re: [PATCH] libgomp: Add helper functions for memory handling.

[Jakub Jelinek]

On Sat, Jul 25, 2020 at 11:03:27AM -0400, y2s1982 via Gcc-patches wrote:
> This patch adds few helper functions aims to improve readability of
> fetching addresses, sizes, and values. It also proposes a syntax for
> querying these information through the callback functions, similar to
> that of LLVM implementation. The syntax format is <query type>_<varaible
> name>, or <query type>_<variable type>_<member type>. '_' is the
> delimiter between fields. '<query type>', as currently defined in the
> enum, is either gompd_query_address or gompd_query_size: the first
> handles address or offset queries while the second handles the size of
> the variable/member. '<variable type>' refers to the variable type, and
> '<member type>' refers to the data type of the member of the variable.
> This code is incomplete: in particular, it currently lacks CUDA support,
> as well as segment handling, and inlining of the functions.

That assumes on the libgomp.so.1 side you want to add all the magic symbols
to the dynamic! symbol table.  We do not want that, it is a big waste of
system resources that way.
Rather than that, as I said several times, there should be a single
variable, perhaps with generated content, with a compact format of data on
which the two libraries agree on and libgompd should parse and use
information from that single variable.

So I'm afraid pretty much nothing from this patch is really usable.

> +#define FOREACH_QUERYTYPE(TYPE)\
> +	TYPE (gompd_query_address)\
> +	TYPE (gompd_query_size)\
> +
Why the final \ ?  There should be space before the \ too.
> +
>  extern ompd_callbacks_t gompd_callbacks;
>  
>  typedef struct _ompd_aspace_handle {
> @@ -47,4 +53,51 @@ typedef struct _ompd_aspace_handle {
>    ompd_size_t ref_count;
>  } ompd_address_space_handle_t;
>  
> +typedef enum gompd_query_type {
> +#define GENERATE_ENUM(ENUM) ENUM,
> +  FOREACH_QUERYTYPE (GENERATE_ENUM)
> +#undef GENERATE_ENUM
> +} query_type;

It is unclear what you want to achieve through this, right now it is
a very fancy way to say
typedef enum gompd_query_type {
  gompd_query_address,
  gompd_query_size,
} query_type;

> +
> +ompd_rc_t gompd_getQueryStringSize (size_t *, query_type, const char*,
> +				    const char *);

GCC is not a CamelCaseShop, so please don't use such names.
Appropriate names would be gompd_get_query_string_size etc.

	Jakub

Re: [PATCH] libgomp: Add helper functions for memory handling.

[y2s1982]

Hello Jakub,

Thanks for the reply. I apparently need further clarification.

On Mon, Jul 27, 2020 at 12:36 PM Jakub Jelinek <jakub@redhat.com> wrote:

> On Sat, Jul 25, 2020 at 11:03:27AM -0400, y2s1982 via Gcc-patches wrote:
> > This patch adds few helper functions aims to improve readability of
> > fetching addresses, sizes, and values. It also proposes a syntax for
> > querying these information through the callback functions, similar to
> > that of LLVM implementation. The syntax format is <query type>_<varaible
> > name>, or <query type>_<variable type>_<member type>. '_' is the
> > delimiter between fields. '<query type>', as currently defined in the
> > enum, is either gompd_query_address or gompd_query_size: the first
> > handles address or offset queries while the second handles the size of
> > the variable/member. '<variable type>' refers to the variable type, and
> > '<member type>' refers to the data type of the member of the variable.
> > This code is incomplete: in particular, it currently lacks CUDA support,
> > as well as segment handling, and inlining of the functions.
>
> That assumes on the libgomp.so.1 side you want to add all the magic symbols
> to the dynamic! symbol table.  We do not want that, it is a big waste of
> system resources that way.
> Rather than that, as I said several times, there should be a single
> variable, perhaps with generated content, with a compact format of data on
> which the two libraries agree on and libgompd should parse and use
> information from that single variable.
>

I do know you have said this several times, and I thought I understood it,
but it seems I am wrong each time. I just want to clarify my understanding
and what I had intended on doing on this and would like further explanation
on what you would like to see happen more specifically so that I may make
less mistakes.

My assumption in this patch was that, as the ompd_callback_symbol_addr_fn_t
callback function takes 2 context addresses and string query as input, and
ompd_address_t address as an output, I should give it:
  - the context addresses, which I assumed would contain a single data in
compact form generated from the tool's side having size, offset, and
address information,
  - and a string, which is basically a query that needs to be interpreted
by the callback function to determine which part of the data it needs to
use to generate to the returning pointer.
I wasn't sure what role the filename input played in this.
This seemed to fit what you meant by having a single compact data that
contains all the information while resolving my ongoing mystery as to what
the callback was expecting to have as the string identifying the symbol. To
further clarify, I assumed the callback would do string manipulation and
comparison to identify which information is being requested, refer to the
single data contained in appropriate context, and return the address
information.

I am more than willing to scrap my bad idea for a better one. I am
sincerely interested in learning better ways to implement and improve
myself. I just need to know more specifics of the design, particularly:
- where the compact data is stored (I assumed context, which means it might
not be defined in the library side, but should it be in the handle or
global to library?),
- how the information necessary for the compact data is gathered (if it is
done on the library side, should I just use the ompd_callback_sizeof_fn_t
to obtain primitive sizes, and from there, just build the offset
information for each variable members? How about variable address?)
- how the ompd_callback_symbol_addr_fn_t callback function would likely use
it given its arguments (input of 2 contexts, 1 file name, 1 string to
output 1 address),
- what are the expected strings for the callback that would correspond to
each variable (here, I assume, the types would be gomp_thread,
gompd_thread_pool, gomp_team, etc.) or each member of said variables,
(these, at least I expected, should be documented and agreed on between
library and tool),
among other things.


>
> So I'm afraid pretty much nothing from this patch is really usable.
>
> > +#define FOREACH_QUERYTYPE(TYPE)\
> > +     TYPE (gompd_query_address)\
> > +     TYPE (gompd_query_size)\
> > +
> Why the final \ ?  There should be space before the \ too.
> > +
> >  extern ompd_callbacks_t gompd_callbacks;
> >
> >  typedef struct _ompd_aspace_handle {
> > @@ -47,4 +53,51 @@ typedef struct _ompd_aspace_handle {
> >    ompd_size_t ref_count;
> >  } ompd_address_space_handle_t;
> >
> > +typedef enum gompd_query_type {
> > +#define GENERATE_ENUM(ENUM) ENUM,
> > +  FOREACH_QUERYTYPE (GENERATE_ENUM)
> > +#undef GENERATE_ENUM
> > +} query_type;
>
> It is unclear what you want to achieve through this, right now it is
> a very fancy way to say
> typedef enum gompd_query_type {
>   gompd_query_address,
>   gompd_query_size,
> } query_type;
>

I wanted to have a consistent way to create an enum and a corresponding
array string that translates the enum to a string. I also wasn't sure if I
covered all the types the query should handle as the idea was still being
discussed. The FOREACH_QUERYTYPE macro was later used again in
ompd-helper.c to generate an array of strings. It was placed there instead
of the header because gcc was kept complaining that the variable was not
used while compiling other .c files that included the header.


>
> > +
> > +ompd_rc_t gompd_getQueryStringSize (size_t *, query_type, const char*,
> > +                                 const char *);
>
> GCC is not a CamelCaseShop, so please don't use such names.
> Appropriate names would be gompd_get_query_string_size etc.
>
Apologies. Thank you for the guidance.

>
>         Jakub
>
> Thank you for the detailed information. I look forward to having more
clarity in these aspects.

Cheers,

Tony Sim

Re: [PATCH] libgomp: Add helper functions for memory handling.

[Jakub Jelinek]

On Mon, Jul 27, 2020 at 01:59:01PM -0400, y2s1982 via Gcc-patches wrote:
> I do know you have said this several times, and I thought I understood it,
> but it seems I am wrong each time. I just want to clarify my understanding
> and what I had intended on doing on this and would like further explanation
> on what you would like to see happen more specifically so that I may make
> less mistakes.
> 
> My assumption in this patch was that, as the ompd_callback_symbol_addr_fn_t
> callback function takes 2 context addresses and string query as input, and
> ompd_address_t address as an output, I should give it:
>   - the context addresses, which I assumed would contain a single data in
> compact form generated from the tool's side having size, offset, and
> address information,
>   - and a string, which is basically a query that needs to be interpreted
> by the callback function to determine which part of the data it needs to
> use to generate to the returning pointer.

The standard of course can't go into details on what exactly it is, because
it is heavily dependent on what object format is used, etc.
But for ELF, the intent is that the symbol addr callback does pretty much
what dlsym does, i.e. perform a (dynamic) symbol lookup like the dynamic
linker normally performs.  It likely can't use the dynamic linker directly,
for running processes that would mean having to perform an inferior call to
the libdl library, and for core files there is no running process to do it
in, but it knows all the currently loaded libraries, their search order and
so can in that order go one library by one and search their dynamic symbol
tables (if you do nm -D on a library or readelf -Ws, you'll see what is in
there), relocate the addresses in there for shared libraries depending on
their load bias (difference between the addresses they are loaded at and
linked to) and return those.  If filename is supplied, then it would perform
the lookup primarily in the given shared library and only then fallback to
others.

> I wasn't sure what role the filename input played in this.
> This seemed to fit what you meant by having a single compact data that
> contains all the information while resolving my ongoing mystery as to what
> the callback was expecting to have as the string identifying the symbol. To
> further clarify, I assumed the callback would do string manipulation and
> comparison to identify which information is being requested, refer to the
> single data contained in appropriate context, and return the address
> information.
> 
> I am more than willing to scrap my bad idea for a better one. I am
> sincerely interested in learning better ways to implement and improve
> myself. I just need to know more specifics of the design, particularly:
> - where the compact data is stored (I assumed context, which means it might
> not be defined in the library side, but should it be in the handle or
> global to library?),
> - how the information necessary for the compact data is gathered (if it is
> done on the library side, should I just use the ompd_callback_sizeof_fn_t
> to obtain primitive sizes, and from there, just build the offset
> information for each variable members? How about variable address?)
> - how the ompd_callback_symbol_addr_fn_t callback function would likely use
> it given its arguments (input of 2 contexts, 1 file name, 1 string to
> output 1 address),
> - what are the expected strings for the callback that would correspond to
> each variable (here, I assume, the types would be gomp_thread,
> gompd_thread_pool, gomp_team, etc.) or each member of said variables,
> (these, at least I expected, should be documented and agreed on between
> library and tool),
> among other things.

So, there are multiple things you might need to know about the libgomp
shared library's internal ABI in order to implement the OMPD library that
can handle it.
One thing are addresses of internal variables.

Say if OMPD wants to query the value of some ICV, those have different
scopes, some of them are per-process, others per-device, others per-thread,
others per-task.  Say the cancel-var is per process and stored in
gomp_cancel_var which is a bool variable (in env.c).
If you look for that symbol:
readelf -Ws libgomp.so.1 | grep gomp_cancel_var
   478: 0000000000041704     1 OBJECT  LOCAL  DEFAULT   24 gomp_cancel_var
you'll see that there is such a symbol, but it is (intentionally) not
exported and not present in .dynsym, so the symbol lookup callback can't
find it.
So, if OMPD needs to get at the address of that, it again should look at
the single exported variable with all the data and find in it the address
of the variable.  Now, addresses are the only thing which need extra care,
it is something where one needs the help of the assembler and linker.
One way to do it is to have
void *gomp_internal_addresses[] = {
  &gomp_cancel_var,
  ...
};
table and export that.  It is a fallback for when the target can't do
something better, but it will require dynamic relocations, so if possible,
it would be better to use something like:
ptrdiff_t gomp_internal_addr_offsets[] = {
  (char *)&gomp_cancel_var - (char *)&gomp_internal_addr_offsets[0],
  ...
};
which doesn't need dynamic relocations.  All other details, sizeof
something, offsetof (something, field), how many times one needs to
dereference something, or e.g. for the ICVs also the information whether
the ICV lives in a per-process variable (which), or is e.g. at some offset
in the gomp_thread or where exactly, is something I'd highly suggest
to initially just hardcode in the ompd source with some specific comment
above it, so that you can then very quickly find everything that initially
non-portably is hardcoded in libgompd.so.1 and thus e.g. it can't support
-m32 vs. -m64, and only after you get a better picture of what exactly you
need you start putting it into the compact data section.

So, say you start with libgompd.so.1 sources doing:
          /* INFO */
  addr += offsetof (struct blah, field);
and once you have big part of OMPD implemented, you go through everything
you have such comments on, and instead create ompd-info-generate.c
to contain some macros that in the end will store the offsetof value in the
output and then on the libgompd.so.1 part read that back.

	Jakub

[PATCH] libgomp: Add helper functions for memory handling.

[y2s1982]

This patch adds few helper functions aims to improve readability of
fetching addresses, sizes, and values. It also proposes a syntax for
querying these information through the callback functions, similar to
that of LLVM implementation. The syntax format is <query type>_<varaible
name>, or <query type>_<variable type>_<member type>. '_' is the
delimiter between fields. '<query type>', as currently defined in the
enum, is either gompd_query_address or gompd_query_size: the first
handles address or offset queries while the second handles the size of
the variable/member. '<variable type>' refers to the variable type, and
'<member type>' refers to the data type of the member of the variable.
This code is incomplete: in particular, it currently lacks CUDA support,
as well as segment handling, and inlining of the functions.

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

libgomp/ChangeLog:

	* Makefile.am (libgompd_la_SOURCES): Add ompd-helper.c.
	* Makefile.in: Regenerate.
	* libgompd.h (FOREACH_QUERYTYPE): Add macro for query types.
	(query_type): Add enum for query types.
	(gompd_getQueryStringSize): Add declaration.
	(gompd_getQueryString): Add declaration.
	(gompd_getAddress): Add declaration.
	(gompd_getSize): Add declaration.
	(gompd_getValue): Add declaration.
	(gompd_getVariableAddress): Add declaration.
	(gompd_getVariableSize): Add declaration.
	(gompd_getVariableValue): Add declaration.
	(gompd_getMemberAddress): Add declaration.
	(gompd_getMemberSize): Add declaration.
	(gompd_getMemberValue): Add declaration.
	* ompd-helper.c: New file.

---
 libgomp/Makefile.am   |   2 +-
 libgomp/Makefile.in   |   5 +-
 libgomp/libgompd.h    |  53 ++++++++++
 libgomp/ompd-helper.c | 228 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 285 insertions(+), 3 deletions(-)
 create mode 100644 libgomp/ompd-helper.c

diff --git a/libgomp/Makefile.am b/libgomp/Makefile.am
index fe0a92122ea..d126bc655fc 100644
--- a/libgomp/Makefile.am
+++ b/libgomp/Makefile.am
@@ -90,7 +90,7 @@ 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_SOURCES = ompd-lib.c ompd-proc.c
+libgompd_la_SOURCES = ompd-lib.c ompd-proc.c ompd-helper.c
 
 include $(top_srcdir)/plugin/Makefrag.am
 
diff --git a/libgomp/Makefile.in b/libgomp/Makefile.in
index f74d5f3ac8e..fdd488ca98e 100644
--- a/libgomp/Makefile.in
+++ b/libgomp/Makefile.in
@@ -235,7 +235,7 @@ am_libgomp_la_OBJECTS = alloc.lo atomic.lo barrier.lo critical.lo \
 	$(am__objects_1)
 libgomp_la_OBJECTS = $(am_libgomp_la_OBJECTS)
 libgompd_la_LIBADD =
-am_libgompd_la_OBJECTS = ompd-lib.lo ompd-proc.lo
+am_libgompd_la_OBJECTS = ompd-lib.lo ompd-proc.lo ompd-helper.lo
 libgompd_la_OBJECTS = $(am_libgompd_la_OBJECTS)
 AM_V_P = $(am__v_P_@AM_V@)
 am__v_P_ = $(am__v_P_@AM_DEFAULT_V@)
@@ -593,7 +593,7 @@ libgomp_la_SOURCES = alloc.c atomic.c barrier.c critical.c env.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 $(am__append_4)
-libgompd_la_SOURCES = ompd-lib.c ompd-proc.c
+libgompd_la_SOURCES = ompd-lib.c ompd-proc.c ompd-helper.c
 
 # Nvidia PTX OpenACC plugin.
 @PLUGIN_NVPTX_TRUE@libgomp_plugin_nvptx_version_info = -version-info $(libtool_VERSION)
@@ -817,6 +817,7 @@ distclean-compile:
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/oacc-plugin.Plo@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/oacc-profiling.Plo@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/oacc-target.Plo@am__quote@
+@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/ompd-helper.Plo@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/ompd-lib.Plo@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/ompd-proc.Plo@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/ordered.Plo@am__quote@
diff --git a/libgomp/libgompd.h b/libgomp/libgompd.h
index 495995e00d3..69bf3573d3c 100644
--- a/libgomp/libgompd.h
+++ b/libgomp/libgompd.h
@@ -30,12 +30,18 @@
 #define LIBGOMPD_H 1
 
 #include "omp-tools.h"
+#include <string.h>
 
 #define ompd_stringify(x) ompd_str2(x)
 #define ompd_str2(x) #x
 
 #define OMPD_VERSION 201811
 
+#define FOREACH_QUERYTYPE(TYPE)\
+	TYPE (gompd_query_address)\
+	TYPE (gompd_query_size)\
+
+
 extern ompd_callbacks_t gompd_callbacks;
 
 typedef struct _ompd_aspace_handle {
@@ -47,4 +53,51 @@ typedef struct _ompd_aspace_handle {
   ompd_size_t ref_count;
 } ompd_address_space_handle_t;
 
+typedef enum gompd_query_type {
+#define GENERATE_ENUM(ENUM) ENUM,
+  FOREACH_QUERYTYPE (GENERATE_ENUM)
+#undef GENERATE_ENUM
+} query_type;
+
+ompd_rc_t gompd_getQueryStringSize (size_t *, query_type, const char*,
+				    const char *);
+
+ompd_rc_t gompd_getQueryString (char **, query_type, const char*, const char *);
+
+ompd_rc_t gompd_getAddress (ompd_address_space_context_t *,
+			    ompd_thread_context_t *, ompd_address_t *,
+			    const char *, const char *, ompd_addr_t);
+
+ompd_rc_t gompd_getSize (ompd_address_space_context_t *,
+			 ompd_thread_context_t *, ompd_size_t *, const char *,
+			 const char *);
+
+ompd_rc_t gompd_getValue (ompd_address_space_context_t *,
+			  ompd_thread_context_t *,
+			  void *, ompd_address_t *, const char *, const char *);
+
+ompd_rc_t gompd_getVariableAddress (ompd_address_space_context_t *,
+				    ompd_thread_context_t *, ompd_address_t *,
+				    const char *, ompd_addr_t);
+
+ompd_rc_t gompd_getVariableSize (ompd_address_space_context_t *,
+				 ompd_thread_context_t *, ompd_size_t *,
+				 const char *);
+
+ompd_rc_t gompd_getVariableValue (ompd_address_space_context_t *,
+				  ompd_thread_context_t *,
+				  void *, ompd_address_t *, const char *);
+
+ompd_rc_t gompd_getMemberAddress (ompd_address_space_context_t *,
+				  ompd_thread_context_t *, ompd_address_t *,
+				  const char *, const char *, ompd_addr_t);
+
+ompd_rc_t gompd_getMemberSize (ompd_address_space_context_t *,
+			       ompd_thread_context_t *, ompd_size_t *,
+			       const char *, const char *);
+
+ompd_rc_t gompd_getMemberValue (ompd_address_space_context_t *,
+				ompd_thread_context_t *, void *,
+				ompd_address_t *, const char *, const char *);
+
 #endif /* LIBGOMPD_H */
diff --git a/libgomp/ompd-helper.c b/libgomp/ompd-helper.c
new file mode 100644
index 00000000000..dc8ea58c08c
--- /dev/null
+++ b/libgomp/ompd-helper.c
@@ -0,0 +1,228 @@
+/* Copyright (C) 2020 Free Software Foundation, Inc.
+   Contributed by Yoosuk Sim <y2s1982@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 PURoffsetE.  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/>.  */
+
+/* This file defines custom helper functions for OMPD functions.  */
+
+#include "libgompd.h"
+
+static const char *gompd_query_type_string[] = {
+#define GENERATE_STRING(STRING) ompd_stringify (STRING),
+  FOREACH_QUERYTYPE (GENERATE_STRING)
+#undef GENERATE_STRING
+};
+
+ompd_rc_t
+gompd_getQueryStringSize (size_t *size, query_type type,
+			  const char* variableType, const char *memberType)
+{
+  const char *sep = "_";
+  const char **typeString = &gompd_query_type_string[type];
+
+  *size = strlen (*typeString) + strlen (sep) + strlen (variableType);
+  if (memberType)
+    *size += strlen (sep) + strlen (memberType);
+
+  return ompd_rc_ok;
+}
+ompd_rc_t
+gompd_getQueryString (char **queryString, query_type type,
+		      const char* variableType, const char *memberType)
+{
+  if (!type || !variableType)
+    return ompd_rc_bad_input;
+
+  const char *sep = "_";
+  const char **typeString = &gompd_query_type_string[type];
+
+  size_t offset = 0;
+  strcpy (*queryString, *typeString);
+  offset += strlen (*typeString);
+  strcpy (*queryString + offset, sep);
+  offset += strlen (sep);
+  strcpy (*queryString + offset, variableType);
+  if (memberType)
+    {
+      offset += strlen (variableType);
+      strcpy (*queryString + offset, sep);
+      offset += strlen (sep);
+      strcpy (*queryString + offset, memberType);
+    }
+
+  return ompd_rc_ok;
+}
+
+ompd_rc_t
+gompd_getAddress (ompd_address_space_context_t *ah, ompd_thread_context_t *th,
+		  ompd_address_t *addr, const char *variableType,
+		  const char *memberType, ompd_addr_t seg)
+{
+  if (!ah || !th)
+    return ompd_rc_stale_handle;
+  ompd_rc_t ret = ompd_rc_ok;
+  char *queryString = NULL;
+  size_t querySize = 0;
+  ret = gompd_getQueryStringSize (&querySize, gompd_query_address, variableType,
+				  memberType);
+  if (ret != ompd_rc_ok)
+    return ret;
+  ret = gompd_callbacks.alloc_memory (querySize + 1, (void *) queryString);
+  if (ret != ompd_rc_ok)
+    return ret;
+  ret = gompd_getQueryString (&queryString, gompd_query_address, variableType,
+			      memberType);
+  if (ret != ompd_rc_ok)
+    return ret;
+
+  if (!memberType)
+    {
+      ret = gompd_callbacks.symbol_addr_lookup (ah, th, queryString, addr,
+						NULL);
+      if (ret != ompd_rc_ok)
+	return ret;
+    }
+  else if (addr)
+    {
+      ompd_size_t offset;
+      ompd_address_t offsetAddr;
+      ret = gompd_callbacks.symbol_addr_lookup (ah, th, queryString,
+						&offsetAddr, NULL);
+      if (ret != ompd_rc_ok)
+	return ret;
+      ret = gompd_callbacks.read_memory (ah, th, &offsetAddr,
+					 sizeof (ompd_size_t),
+					 (void *) &offset);
+      if (ret != ompd_rc_ok)
+	return ret;
+      addr->address += offset;
+    }
+  else
+    {
+      return ompd_rc_bad_input;
+    }
+
+  ret = gompd_callbacks.free_memory (queryString);
+  return ret;
+}
+
+ompd_rc_t
+gompd_getSize (ompd_address_space_context_t *ah, ompd_thread_context_t *th,
+	       ompd_size_t *size, const char *variableType,
+	       const char *memberType)
+{
+  if (!ah || !th)
+    return ompd_rc_stale_handle;
+  ompd_rc_t ret;
+  ompd_address_t sizeAddr;
+  char *queryString = NULL;
+  ret = gompd_getQueryString (&queryString, gompd_query_size, variableType,
+			      memberType);
+
+  if (ret != ompd_rc_ok)
+    return ret;
+
+  ret = gompd_callbacks.symbol_addr_lookup (ah, th, queryString, &sizeAddr,
+					    NULL);
+  if (ret != ompd_rc_ok)
+    return ret;
+
+  ret = gompd_callbacks.read_memory (ah, th, &sizeAddr, sizeof (ompd_size_t),
+				     (void *) size);
+  return ret;
+}
+
+ompd_rc_t gompd_getValue (ompd_address_space_context_t *ah,
+			  ompd_thread_context_t *th, void *value,
+			  ompd_address_t *addr, const char *variableType,
+			  const char *memberType)
+{
+  if (!ah || !th)
+    return ompd_rc_stale_handle;
+  if (!addr)
+    return ompd_rc_bad_input;
+
+  ompd_rc_t ret;
+  ompd_size_t size;
+
+  ret = gompd_getSize (ah, th, &size, variableType, memberType);
+  if (ret != ompd_rc_ok)
+    return ret;
+
+  ret = gompd_callbacks.read_memory (ah, th, addr, size, value);
+
+  return ompd_rc_ok;
+}
+
+ompd_rc_t
+gompd_getVariableAddress (ompd_address_space_context_t *ah,
+			  ompd_thread_context_t *th, ompd_address_t *addr,
+			  const char *variableType, ompd_addr_t seg)
+{
+  return gompd_getAddress (ah, th, addr, variableType, NULL, seg);
+}
+
+ompd_rc_t
+gompd_getVariableSize (ompd_address_space_context_t *ah,
+		       ompd_thread_context_t * th, ompd_size_t *size,
+		       const char *variableType)
+{
+  return gompd_getSize (ah, th, size, variableType, NULL);
+}
+
+ompd_rc_t
+gompd_getVariableValue (ompd_address_space_context_t *ah,
+			ompd_thread_context_t *th, void *value,
+			ompd_address_t *variableAddress,
+			const char *variableType)
+{
+  return gompd_getValue (ah, th, value, variableAddress, variableType, NULL);
+}
+
+ompd_rc_t
+gompd_getMemberAddress (ompd_address_space_context_t *ah,
+			ompd_thread_context_t *th, ompd_address_t *variableAddr,
+			const char *variableType, const char *memberType,
+			ompd_addr_t seg)
+{
+  if (!variableAddr)
+    return ompd_rc_bad_input;
+  return gompd_getAddress (ah, th, variableAddr, variableType, memberType, seg);
+}
+
+ompd_rc_t
+gompd_getMemberSize (ompd_address_space_context_t *ah,
+		     ompd_thread_context_t *th, ompd_size_t *size,
+		     const char *variableType, const char *memberType)
+{
+  return gompd_getSize (ah, th, size, variableType, memberType);
+}
+
+ompd_rc_t
+gompd_getMemberValue (ompd_address_space_context_t *ah,
+		      ompd_thread_context_t *th, void *value,
+		      ompd_address_t *addr, const char *variableType,
+		      const char *memberType)
+{
+  return gompd_getValue (ah, th, value, addr, variableType, memberType);
+}
-- 
2.27.0