openshmem-org / tests-sos

Sandia OpenSHMEM unit tests and performance testing suite
Other
6 stars 11 forks source link

Question about asym_alloc.c test #42

Closed ronawho closed 3 months ago

ronawho commented 4 months ago

asym_alloc.c calls shmem_malloc with a different size argument on each PE, which undefined behavior. The 1.5 spec for shmem_malloc says:

The user is also responsible for calling these routines with identical argument(s) on all PEs; if differing ptr, size, or alignment arguments are used, the behavior of the call and any subsequent OpenSHMEM calls is undefined.

The test itself says:

  • This semantic is provided in OpenSHMEM 1.1 and some versions of Cray SHMEM.
  • It was removed from OpenSHMEM in 1.2, but we maintain it for backward
  • compatibility.

This makes me wonder if the test could be opt-in since it's not standard compliant and is testing a specific implementation.


FWIW though I don't see this text about this semantic being supported in 1.1 The 1.1 spec for shmalloc says:

The user is responsible for calling these functions with identical argument(s) on all PEs; if differing size arguments are used, the behavior of the call and any subsequent OpenSHMEM calls becomes undefined.

And the 1.2 spec, which added shmem_malloc says:

The user is responsible for calling these routines with identical argument(s) on all PEs; if differing size arguments are used, the behavior of the call and any subsequent OpenSHMEM calls becomes undefined.

I'm probably just missing some history/context, but just reading the old specs quickly it's not obvious to me when this was legal.

wrrobin commented 4 months ago

@ronawho - Thanks for pointing this out. This is an old test, even though the context is relevant to some of the recent conversations related to asymmetric allocation proposal. As you pointed out, since this usage is currently leads to an undefined behavior, we decided to remove the test from the repo. Does that sound ok to you?

ronawho commented 4 months ago

That works for me, thanks!

wrrobin commented 3 months ago

Resolved by #43