Hello Jakub,
Thank you for the feedback. I can't believe I made some careless mistakes.
I am also very interested in learning some new techniques mentioned. I had
some questions about them.
On Thu, Jun 25, 2020 at 4:28 AM Jakub Jelinek <ja...@redhat.com> wrote:
>
> > --- /dev/null
> > +++ b/libgomp/libgompd.map
> > @@ -0,0 +1,49 @@
> > +OMPD_5.0 {
> > + global:
> > + ompd_dll_locations_valid;
>
> ompd_dll_locations and ompd_dll_locations_valid both need to be exported,
> but not from libgompd.so.1 but from libgomp.so.1, so they need to go into
> libgomp.map and be defined somewhere in libgomp.so.1, so likely env.c.
>
Ah, so code in env.c gets executed before OMPD gets started?
Include omp-tools.h and plugin-suffix.h in there and move ompd_dll_locations
> definition in there (into the #ifndef LIBGOMP_OFFLOADED_ONLY section)
> and for ompd_dll_locations_valid, e.g. make it an alias to
> initialize_env or for the time being just an empty function with
> __attribute__((noipa)) that initialize_env
> calls and I'll help with making it an alias afterwards.
I got curious about making a function alias and started looking around.
Can it be as simple as #define ompd_dll_location_valid initialize_env?
Or should I use something like ialias_call (fn) fn, defined in libgomp.h?
The latter seems like a wrapper that expands differently depending on the
condition,
and since I wasn't sure what gomp_ialias does, I wasn't sure if that was
the way to go.
>
>
> > diff --git a/libgomp/omp-tools.h b/libgomp/omp-tools.h
> > index 394c33e40dd..b6b8c5295a5 100644
> > --- a/libgomp/omp-tools.h
> > +++ b/libgomp/omp-tools.h
> > @@ -101,7 +101,7 @@ typedef struct ompd_device_type_sizes_t {
> > } ompd_device_type_sizes_t;
> >
> >
> > -const char **ompd_dll_locations;
> > +//const char **ompd_dll_locations;
>
> Extern declaration would be
> extern const char **ompd_dll_locations;
>
Okay. I do have an issue:
extern const char **ompd_dll_locations; and const char*
ompd_dll_locations[2]; seems to clash with each other.
At least it was when I had it defined in omp-lib.c. It stopped giving me
errors when it was moved to env.c.
It seems giving the letter a different name and then assigning the value to
the first seems to work, but I was wondering
if there was a better solution.
>
>
> > +ompd_rc_t
> > +ompd_initialize (ompd_word_t api_version, const ompd_callbacks_t
> *callbacks)
> > +{
> > + /* initialized flag */
>
> You don't need to comment everything the function is doing, I think the
> comments don't say anything that isn't obvious from it.
> Comment only for larger blocks of code where it might not be obvious what
> the code is doing and the comment adds added value; comments should be
> usually full sentences, starting with capital letter, ending with . and
> two spaces after the .
> Or one can add function comments (comments before function definition) that
> explain what the function does, what are the arguments etc., but in the
> case of functions documented in the standard it seems pointless,
> the best documentation is the standard.
>
> You don't use the callbacks argument yet, but you will need it later,
> so instead of adding an unused attribute to it, just add
> (void) callbacks;
> for now to shut up warnings.
>
Okay. I will do the same for the api_version argument, too.
For the next patch, I had some questions about this function. It seems I
need to declare an address space
for callbacks. Where does this address space get stored?
>
> Otherwise LGTM.
>
> Jakub
>
> One of the things I wanted to do was create more unit tests.
Should I try making unit tests for the API version functions, focusing on
the formatting of the string?
Would that be something useful for maintaining consistency?
Thank you always for helping me learn more about the project and giving me
great suggestions.
Cheers,
Tony Sim
