[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