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

[y2s1982 <y2s1982@gmail.com>]

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
>
>