oshmem yoda spml call to add_procs seems wrong

[jsquyres]

When compiling master and v2.x:

spml_yoda.c: In function 'mca_spml_yoda_add_procs':
spml_yoda.c:645:33: warning: passing argument 1 of 'mca_pml.pml_add_procs' from incompatible pointer type [-Wincompatible-pointer-types]
     rc = MCA_PML_CALL(add_procs(procs, nprocs));
                                 ^
../../../../ompi/mca/pml/pml.h:550:41: note: in definition of macro 'MCA_PML_CALL'
 #define MCA_PML_CALL(a) mca_pml.pml_ ## a
                                         ^
spml_yoda.c:645:33: note: expected 'struct ompi_proc_t **' but argument is of type 'oshmem_proc_t ** {aka struct oshmem_proc_t **}'
     rc = MCA_PML_CALL(add_procs(procs, nprocs));
                                 ^
../../../../ompi/mca/pml/pml.h:550:41: note: in definition of macro 'MCA_PML_CALL'
 #define MCA_PML_CALL(a) mca_pml.pml_ ## a
                                         ^

I note that pml add_procs takes an (ompi_proc_t **), but https://github.com/open-mpi/ompi/blob/master/oshmem/mca/spml/yoda/spml_yoda.c#L645 is calling with an (oshmem_proc_t **).

This was added just 10 days ago by @ggouaillardet in 6b7bc64.

However, oshmem_proc_t is defined as (https://github.com/open-mpi/ompi/blob/master/oshmem/proc/proc.h#L44-L58):

struct oshmem_proc_t {
    opal_proc_t                    super;
    /* endpoint data */
    void *proc_endpoints[OMPI_PROC_ENDPOINT_TAG_MAX];
    /*
     * All transport channels are globally ordered.
     * pe(s) can talk to each other via subset of transports
     * these holds indexes of each transport into global array
     *  proc -> id, where id can be btl id in yoda or mxm ptl id
     *  in ikrit
     *  spml is supposed to fill this during add_procs()
     **/
    int                             num_transports;
    char                           *transport_ids;
};

which does not contain an (ompi_proc_t), thereby making the (implicit) cast from (oshmem_proc_t**) to (ompi_proc_t**) incorrect.

@ggouaillardet Can you please have a look?

[jsquyres]

Actually, looking at ompi/proc/proc.h, I guess (ompi_proc_t) is compatible with (oshmem_proc_t), so I guess the implicit cast is functional. Ugh -- this is strange and unexpected (i.e., it doesn't fit in with the rest of the OMPI code base convention).

Can we do something a a little less unexpected with the (oshmem_proc_t) type -- e.g., make it have its first member be ompi_proc_t? I realize something would need to be done with the padding in the ompi_proc_t (since oshmem is using it differently).

[ggouaillardet]

I was typing when you posted your last comment...

I can easily silence the warning.

struct ompi_proc_t {
    opal_proc_t                     super;

    /* endpoint data */
    void *proc_endpoints[OMPI_PROC_ENDPOINT_TAG_MAX];

    char padding[16];         /* for future extensions (OSHMEM uses this area also)*/
};

and

struct oshmem_proc_t {
    opal_proc_t                    super;
    /* endpoint data */
    void *proc_endpoints[OMPI_PROC_ENDPOINT_TAG_MAX];
    /*
     * All transport channels are globally ordered.
     * pe(s) can talk to each other via subset of transports
     * these holds indexes of each transport into global array
     *  proc -> id, where id can be btl id in yoda or mxm ptl id
     *  in ikrit
     *  spml is supposed to fill this during add_procs()
     **/
    int                             num_transports;
    char                           *transport_ids;
};

the only difference is ompi has padding whereas oshmem has real data.

bottom line, if an ompi_proc_t is expected, it is fine to pass an oshmem_proc_t,
but the compiler cannot figure it out by itself.

I have several options on top of my head

1. type casting to silence the warning
2. make oshmem_proc_t an union that includes ompi_proc_t
3. get rid of oshmem_proc_t and replace the padding of ompi_proc_t with the data used by oshmem (this is a kind of abstraction violation, and only fly if there is no more project xxx that defines xxx_proc_t)
4. get rid of oshmem_proc_t and update oshmem so it accesses num_transports and transport_ids via accessors (I am thinking macros to keep it simple)

please share your thoughts and let me know if this should be added to the agenda of the next weekly meeting.

[jsquyres]

I added the cast in #2024.

@ggouaillardet Maybe another option is to have a common / shared type that both ompi_proc_t and oshmem_proc_t use as their first member, and then also have an assert() somewhere to ensure that the sizeof(oshmem_proc_t) <= sizeof(ompi_proc_t) (i.e., so that oshmem's extra data doesn't exceed the padding in the ompi_proc_t)...?

[ggouaillardet]

btw, oshmem_proc_t should likely end with a 8 bytes padding if sizeof(char *) == 4 (assuming the compiler adds a 4 bytes padding between the int and the char* in 64 bits arch)
a safer option would be to swap the two elements and pad 4 or 8 bytes

[jsquyres]

@ggouaillardet Good point; I don't know if there's any code that assumes that the sizeof the two types are the same. It would probably be best to ensure that they are the same (e.g., perhaps an assert() should be added to ensure that sizeof(oshmem_proc_t) == sizeof(ompi_proc_t) -- not <=, as I proposed above).

[jsquyres]

The more I think about it, it probably is important to guarantee that the sizes of the two types are identical. Otherwise, there wouldn't be any padding at the end of (ompi_proc_t), right?

[ggouaillardet]

that makes sense to me.
I like option 4. best, since it does not require any modifications in ompi
(though oshmem changes might be quite important) and it is future-proof
(assuming we put the right asserts at the right places)

[jsquyres]

I'm not familiar enough with the OSHMEM code -- is it easy to make a PR for option 4?

[ggouaillardet]

I will give it a try this week.
that should not be hard, but that could be a bit tedious ...

[jsquyres]

@ggouaillardet Excellent; thanks.