[PATCH] libgomp: Add OMPD functions in 5.5.6 and related data types.

[PATCH] libgomp: Add OMPD functions in 5.5.6 and related data types.

[y2s1982]

This patch adds function definition described in the section 5.5.6 of
the OpenMP API Specificiation 5.0. It also partially defines some of the
handler data types related to the section.

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

libgomp/ChangeLog:

	* libgompd.h (ompd_thread_handle_t): Add.
	(ompd_parallel_handle_t): Add.
	(ompd_task_handle_t): Add.
	* ompd-parallel.c: New file.

---
 libgomp/libgompd.h      |  17 ++++++
 libgomp/ompd-parallel.c | 125 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 142 insertions(+)
 create mode 100644 libgomp/ompd-parallel.c

diff --git a/libgomp/libgompd.h b/libgomp/libgompd.h
index 495995e00d3..557f1c36ec2 100644
--- a/libgomp/libgompd.h
+++ b/libgomp/libgompd.h
@@ -47,4 +47,21 @@ typedef struct _ompd_aspace_handle {
   ompd_size_t ref_count;
 } ompd_address_space_handle_t;
 
+typedef struct _ompd_thread_handle {
+  ompd_parallel_handle_t *ph;
+  ompd_thread_context_t *thread_context;
+  ompd_address_t thread; /* Stores gomp_thread *.  */
+}ompd_thread_handle_t;
+
+typedef struct _ompd_parallel_handle{
+  ompd_address_space_handle_t *ah;
+  ompd_parallel_handle_t *enclosing_ph;
+  ompd_size_t enclosed_ph;
+  ompd_address_t thread_pool; /* Stores gomp_thread_pool *.  */
+}ompd_parallel_handle_t;
+
+typedef struct _ompd_task_handle{
+  ompd_parallel_handle_t *ph;
+}ompd_task_handle_t;
+
 #endif /* LIBGOMPD_H */
diff --git a/libgomp/ompd-parallel.c b/libgomp/ompd-parallel.c
new file mode 100644
index 00000000000..ca2de726238
--- /dev/null
+++ b/libgomp/ompd-parallel.c
@@ -0,0 +1,125 @@
+/* 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 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/>.  */
+
+/* This file contains function definition related to OMPD's Parallel Region
+   Handles.  The specification is defined in the section 5.5.6 of the OpenMP
+   API Specification 5.0.  */
+
+#include "omp-tools.h"
+#include "libgomp.h"
+#include "libgompd.h"
+
+ompd_rc_t
+ompd_get_curr_parallel_handle (ompd_thread_handle_t *th,
+			       ompd_parallel_handle_t **ph)
+{
+  if (!th)
+    return ompd_rc_stale_handle;
+  if (!ph)
+    return ompd_rc_bad_input;
+
+  if (((struct gomp_thread *)th->thread.address)->ts.team != NULL)
+    return ompd_rc_error;
+
+  *ph = th->ph;
+  return ompd_rc_ok;
+}
+
+ompd_rc_t
+ompd_get_enclosing_parallel_handle (ompd_parallel_handle_t *ph,
+				    ompd_parallel_handle_t **enclosing_ph)
+{
+  if (!ph)
+    return ompd_rc_stale_handle;
+  if (!enclosing_ph)
+    return ompd_rc_bad_input;
+  if (!ph->enclosing_ph)
+    return ompd_rc_unavailable;
+
+  ompd_rc_t ret = ompd_rc_error;
+  ompd_size_t i = 0;
+  struct gomp_thread_pool * pool =
+  (struct gomp_thread_pool *)ph->thread_pool.address;
+  for (i = 0; i < pool->threads_used && ret == ompd_rc_error; i++)
+  {
+    if (pool->threads[i]->ts.team == NULL)
+      ret = ompd_rc_ok;
+  }
+  if (ret != ompd_rc_ok)
+    return ret;
+
+  *enclosing_ph = ph->enclosing_ph;
+  return ompd_rc_ok;
+}
+
+ompd_rc_t
+ompd_get_task_parallel_handle (ompd_task_handle_t *th,
+			       ompd_parallel_handle_t **ph)
+{
+  if (!th || !th->ph)
+    return ompd_rc_stale_handle;
+  if (!ph)
+    return ompd_rc_bad_input;
+
+  ompd_rc_t ret = ompd_rc_error;
+  ompd_size_t i = 0;
+  struct gomp_thread_pool * pool =
+  (struct gomp_thread_pool *)th->ph->thread_pool.address;
+  for (i = 0; i < pool->threads_used && ret == ompd_rc_error; i++)
+  {
+    if (pool->threads[i]->ts.team == NULL)
+      ret = ompd_rc_ok;
+  }
+  if (ret != ompd_rc_ok)
+    return ret;
+
+  *ph = th->ph;
+  return ompd_rc_ok;
+}
+
+ompd_rc_t ompd_rel_parallel_handle (ompd_parallel_handle_t *ph)
+{
+  if (ph->enclosed_ph > 0)
+    return ompd_rc_unavailable;
+
+  gompd_callbacks.free_memory (ph);
+  return ompd_rc_ok;
+}
+
+ompd_rc_t
+ompd_parallel_handle_compare (ompd_parallel_handle_t *lhs,
+			      ompd_parallel_handle_t *rhs, int *cmp_value)
+{
+  int compare = 1;
+
+  /* FIXME: Properly follow the implementation details on ordering parallel
+     region.  */
+  if (lhs->ah != rhs->ah
+      || lhs->thread_pool.address != rhs->thread_pool.address)
+    compare = 0;
+
+  *cmp_value = compare;
+  return ompd_rc_ok;
+}
-- 
2.27.0

Re: [PATCH] libgomp: Add OMPD functions in 5.5.6 and related data types.

[Jakub Jelinek]

On Sat, Jul 11, 2020 at 06:05:36PM -0400, y2s1982 wrote:
> 2020-07-11  Tony Sim  <y2s1982@gmail.com>
> 
> libgomp/ChangeLog:
> 
> 	* libgompd.h (ompd_thread_handle_t): Add.
> 	(ompd_parallel_handle_t): Add.
> 	(ompd_task_handle_t): Add.
> 	* ompd-parallel.c: New file.

So you add a new file, but don't add it to Makefile.am - that means
nothing will compile it.
> +}ompd_thread_handle_t;

Formatting, space after }
> +
> +typedef struct _ompd_parallel_handle{

and space bef before { (etc.).
> +  ompd_address_space_handle_t *ah;
> +  ompd_parallel_handle_t *enclosing_ph;
> +  ompd_size_t enclosed_ph;
> +  ompd_address_t thread_pool; /* Stores gomp_thread_pool *.  */
> +}ompd_parallel_handle_t;
> +
> +typedef struct _ompd_task_handle{
> +  ompd_parallel_handle_t *ph;
> +}ompd_task_handle_t;
> +
>  #endif /* LIBGOMPD_H */
> +  ompd_rc_t ret = ompd_rc_error;
> +  ompd_size_t i = 0;
> +  struct gomp_thread_pool * pool =
> +  (struct gomp_thread_pool *)ph->thread_pool.address;

Formatting, = should never be at the end of line.  And no space between
* and pool, so:
  struct gomp_thread_pool *pool
    = (struct gomp_thread_pool *) ph->thread_pool.address;

> +  for (i = 0; i < pool->threads_used && ret == ompd_rc_error; i++)
> +  {
> +    if (pool->threads[i]->ts.team == NULL)
> +      ret = ompd_rc_ok;
> +  }

Like I said on other patches, { would need to be indented by 2 spaces from
for, but as the body contains a single statement, just leave the {}s out
completely and then it can stay as is.

More important, I don't see any function that would initialize
e.g. threads_used, etc., IMNSHO you should start with those and
there write the reading of those from the inferior.
And, unless that routine copies everything from the inferior, which is risky
because it can change there any time, I think the above is not really what
you want, you instead want to read it from the inferior.
The debugged process (if it is a process and not e.g. a core file) is not in
the same address space as the debugger (that loads the OMPD library), so
even if you get pointers copied from the debugged process, you can't
dereference them, but need to use the callbacks to read the corresponding
memory.

	Jakub

Re: [PATCH] libgomp: Add OMPD functions in 5.5.6 and related data types.

[y2s1982]

Hello Jakub,


On Tue, Jul 14, 2020 at 6:07 AM Jakub Jelinek <jakub@redhat.com> wrote:

> On Sat, Jul 11, 2020 at 06:05:36PM -0400, y2s1982 wrote:
> > 2020-07-11  Tony Sim  <y2s1982@gmail.com>
> >
> > libgomp/ChangeLog:
> >
> >       * libgompd.h (ompd_thread_handle_t): Add.
> >       (ompd_parallel_handle_t): Add.
> >       (ompd_task_handle_t): Add.
> >       * ompd-parallel.c: New file.
>
> So you add a new file, but don't add it to Makefile.am - that means
> nothing will compile it.

> +}ompd_thread_handle_t;
>
> Formatting, space after }
> > +
> > +typedef struct _ompd_parallel_handle{
>
> and space bef before { (etc.).
> > +  ompd_address_space_handle_t *ah;
> > +  ompd_parallel_handle_t *enclosing_ph;
> > +  ompd_size_t enclosed_ph;
> > +  ompd_address_t thread_pool; /* Stores gomp_thread_pool *.  */
> > +}ompd_parallel_handle_t;
> > +
> > +typedef struct _ompd_task_handle{
> > +  ompd_parallel_handle_t *ph;
> > +}ompd_task_handle_t;
> > +
> >  #endif /* LIBGOMPD_H */
> > +  ompd_rc_t ret = ompd_rc_error;
> > +  ompd_size_t i = 0;
> > +  struct gomp_thread_pool * pool =
> > +  (struct gomp_thread_pool *)ph->thread_pool.address;
>
> Formatting, = should never be at the end of line.  And no space between
> * and pool, so:
>   struct gomp_thread_pool *pool
>     = (struct gomp_thread_pool *) ph->thread_pool.address;
>
Ah! I was hoping to find some information on this. I kept looking around the
code base but the formatting wasn't consistent. Thank you for spelling this
out for me.

>
> > +  for (i = 0; i < pool->threads_used && ret == ompd_rc_error; i++)
> > +  {
> > +    if (pool->threads[i]->ts.team == NULL)
> > +      ret = ompd_rc_ok;
> > +  }
>
> Like I said on other patches, { would need to be indented by 2 spaces from
> for, but as the body contains a single statement, just leave the {}s out
> completely and then it can stay as is.
>
Yes, of course. I am sorry, my old habbit is dieing hard. I will keep it in
mind.


>
> More important, I don't see any function that would initialize
> e.g. threads_used, etc., IMNSHO you should start with those and
> there write the reading of those from the inferior.
>

I started from parallel only because it seemed easier haha.
Okay, I will get working on the thread section, which was just previous to
this section.
Does this mean I should scrap this patch for now and submit something on
this section after the thread section is completed?

And, unless that routine copies everything from the inferior, which is risky
> because it can change there any time, I think the above is not really what
> you want, you instead want to read it from the inferior.
>

When you say inferior, are you referring to the gomp_thread and
gomp_thread_pool?

The debugged process (if it is a process and not e.g. a core file) is not in
> the same address space as the debugger (that loads the OMPD library), so
> even if you get pointers copied from the debugged process, you can't
> dereference them, but need to use the callbacks to read the corresponding
> memory.
>

Okay. I was afraid I might have made a mistake with this regard. The use of
callbacks was to make the debugging process decoupled, right? I think I
remember reading the example of running the process and debugging in a
different machine.

Thank you again for the help.

Cheers,

Tony Sim

>
>         Jakub
>
>