Ompi: `pshmem.h` now not usable without `shmem.h`

[bertwesarg]

The new ctx API introduced in Open MPI 4.0.0 of SHMEM now requires to include shmem.h before pshmem.h, because of the shmem_ctx_t type.

pshmem.h

OSHMEM_DECLSPEC int pshmem_ctx_create(long options, shmem_ctx_t *ctx);
OSHMEM_DECLSPEC void pshmem_ctx_destroy(shmem_ctx_t ctx);

shmem.h

typedef struct { int dummy; } * shmem_ctx_t;

#define SHMEM_CTX_DEFAULT oshmem_ctx_default;

OSHMEM_DECLSPEC int shmem_ctx_create(long options, shmem_ctx_t *ctx);
OSHMEM_DECLSPEC void shmem_ctx_destroy(shmem_ctx_t ctx);

I'm not sure, if this is an issue at all, but maybe this change was unintended.

[jsquyres]

@xinzhao3 Is this a bug, or is it per the OSHMEM spec?

[hoopoepg]

pshmem is profiled interface for shmem, isn't it?
and there are set of defines (like version) in shmem.h which not defined in pshmem

I believe pshmem could not be used without shmem (or pshmem should include shmem.h)

[bertwesarg]

The standard does not mention the profiling interface at all.

[manjugv]

@bertwesarg OpenSHMEM does not specify the profiling interface, yet. However, there is a discussion in the working group to specify the profiling interface [1].

[1] https://github.com/openshmem-org/specification/pull/254

[bertwesarg]

[1] [openshmem-org/specification#254](https://github.com/openshmem-org/specification/pull/254)

Thanks. In the draft on page 112 is an example, which begins with:

#include <stdio.h>
#include <sys/time.h>
#include <pshmem.h>

Thus I conclude that pshmem.h should be able to be included without prio inclusion of shmem.h.

[manjugv]

No. This is not ratified, so you should not draw any conclusions. We are only at the discussion stage now. However, your comment is useful for us, when we discuss this in the WG.

[ax3l]

Sounds like a typical case of "include what you use" to me. If the pshmem.h header uses defines from shmem.h, it should include it.

[jdinan]

I think this issue was always there, since you use the constants defined in shmem.h (e.g. collectives constants) with the pshmem interfaces. In this case, including only pshmem.h wasn't causing a build error in the SHMEM library, but it would have caused a build failure in application code that used the constants.

[bertwesarg]

@jdinan There is a subtle difference between #include a header and using the header. What I moan about is the fact, that since 4.0.0 the sole #include <pshmem.h> already fails. And Autoconf based packages check second (the first check is an existence check) if the header can be #include'd at all. Thus this fails now.

[jdinan]

Got it. The observation I was making is that before shmem_ctx_t was introduced you also needed the constants from shmem.h to use some of the routines pshmem.h. Thus, it also made sense prior to contexts to include shmem.h from pshmem.h. The separate header business originates from OpenSHMEM specifying what can be included in shmem.h (and pshmem is not yet part of the spec so we couldn't put it in there). MPI doesn't have a separate header for its profiling interfaces and perhaps we shouldn't separate these out when we incorporate them into the OpenSHMEM spec. The only reason to do so would be the sheer number of function prototypes increasing the work for the compiler. Interested to hear your thoughts on this.

[ax3l]

I think keeping separate headers make sense, you don't want to declare (profiling) symbols you don't use in regular/unprofiled code. This all adds to compile time and symbol clutter.

Nevertheless, if pshmem.h needs symbols from shmem.h then it needs to include it or alternatively, include just the few selected declarations it needs already. But this detail is opaque to the user, since the "official" facade headers are what a user interacts with. Internal organization of declarations and include names can change.

So the easiest, lowest cost way to fix this single type be missing might be: put the declaration of shmem_ctx_t in an internal header, include this header in both pshmem.h and shmem.h.

[jsquyres]

@ax3l I agree -- I'm not really part of the SHMEM community / don't really care, but to my untrained / outsider eye, a user should either:

#include <shmem.h>

or

#include <pshmem.h>

If the spec isn't clear on this point (or doesn't define <pshmem.h> at all [yet]), then my vote is to do the Simple Thing for users: have them just include a single header file.

[jdinan]

I think the choices in front of the committee are roughly what @ax3l described:

1. pshmem.h ensures that any types and constants needed to use the pshmem interfaces are defined
2. pshmem.h ensures that all shmem API symbols, types, and constants are defined
3. profiling interfaces are defined in shmem.h (there is no pshmem.h)

Options 3 needlessly includes hundreds of function prototypes, which may slow down compilers. Option 2 is easy and may be what users of pshmem do anyway. I think I prefer 1, but my concern is specifying 1 and having 2 become the de facto standard because of assumptions made in user code.

[wrrobin]

@jdinan For Option 1, are we allowing the tool developers to include both the headers (pshmem.h and shemem.h) if they want/need to? Or are we going to treat that as a wrong use-case and report error?

[ax3l]

@wrrobin yes, including both should be allowed as well since shemem.h might have additional functionality. (Or the other way around: one already uses shemem.h and wants some additional tooling functionality.)

Option 1 is what I think is the one with the least namespace "pollution" and (compile, symbol, ...) overhead and users do understand that there can be several facade headers for pulling different aspects (user API, tooling API) of shmem.

[bertwesarg]

Looks like this missed the 4.0.1c1. Is it too late again?

[bertwesarg]

Please note, that Open MPI is en route with the upcoming profiling interface specification of OpenSHMEM here: https://github.com/openshmem-org/specification/pull/254/commits/53d9a4c33bfc23f1d40f9eee4173a24f6e9086a2

[bertwesarg]

This is now in master (#6132 and #6183) and in v4.0.x (#6465). And the profiling interface is alo in the OpenSHMEM specification now (openshmem-org/specification#254). Thus this can be closed now.