bindings: add OBJ_HANDLE datatype - #14358
Open
hppritcha wants to merge 3 commits into
Open
Conversation
Introduce a new binding parameter type, `OBJ_HANDLE`, for the `obj_handle` argument of `MPI_T_Cvar_handle_alloc` and `MPI_T_Event_handle_alloc`. Previously these used the generic `VOID` (`void *`) type, so under the **standard MPI ABI** the incoming handle was passed straight through without being translated from the standard-ABI integer representation to Open MPI's internal handle pointers. The change adds: - `TypeObjHandle` / `TypeObjHandleStandard` in `c_type.py` (both render as `void *`; the standard variant injects an `init_code` conversion call). - `ConvertFuncs.OBJ_HANDLE` in `consts.py`. - A generator method `generate_obj_handle_convert_fn()` in `c.py` that emits a runtime dispatcher (`ompi_convert_abi_obj_handle_intern_obj_handle`) which inspects the integer value of the handle and routes it to the correct per-type converter. - The `.c.in` prototype edits for the two MPI_T functions. Related to open-mpi#14306 Signed-off-by: Howard Pritchard <howardp@lanl.gov>
The OBJ_HANDLE binding type converts the generic obj_handle argument of
the MPI_T handle-allocation routines from the standard MPI ABI integer
representation to Open MPI's internal handle pointers. The initial
dispatcher matched predefined handles by hard-coded numeric ABI value
ranges, which had two defects:
* The datatype range stopped at MPI_2INTEGER (ABI value 562), so every
predefined datatype numbered above it -- the fixed-width C types
(MPI_INT8_T..MPI_UINT64_T) and the optional Fortran types
(MPI_LOGICAL1..MPI_COMPLEX32) -- fell through unconverted. The
standard-ABI integer was then handed to the back end as if it were a
real internal pointer.
* The group and info branches tested only the _NULL value, so
MPI_GROUP_EMPTY and MPI_INFO_ENV were never routed to their
converters.
Rewrite generate_obj_handle_convert_fn() to be data-driven: it now
iterates the same consts.* predefined-handle lists that the individual
per-type converters use and emits one explicit equality test per handle.
This covers every predefined handle by construction and cannot drift
away from the ABI header when values are renumbered. As part of the
rewrite the pointer/integer round trip uses uintptr_t rather than
uint64_t (correct and warning-free on 32-bit targets), and the generated
code no longer carries trailing whitespace or stray empty statements.
Also convert the obj_handle parameter of MPI_T_Pvar_handle_alloc from
VOID to OBJ_HANDLE so it receives the same standard-ABI conversion as
the cvar and event handle-allocation routines it mirrors.
Add TestObjHandleConverter to the bindings-generator unit tests
(wired into make check) to guard against regressions: it renders the
dispatcher and asserts that every predefined handle in every family is
routed, that each family calls its converter, that NULL and unrecognized
handles pass through unchanged, and that the cast uses uintptr_t.
Signed-off-by: Cline <cline@example.com>
Signed-off-by: Howard Pritchard <howardp@lanl.gov>
MPI-5.0 (e.g. p.751) defines the obj_handle argument of
MPI_T_cvar_handle_alloc(), MPI_T_event_handle_alloc(), and
MPI_T_pvar_handle_alloc() as the ADDRESS of a local variable that
holds the bound MPI object's handle -- a pointer to a handle, not a
handle value itself. Every consumer on the Open MPI side already
honors that contract with exactly one dereference: ompit_obj_invalid(),
mca_base_event_handle_alloc(), and mca_base_pvar_handle_alloc() all do
*(void **) obj_handle to reach the real handle.
TypeObjHandleStandard, the standard-ABI binding type introduced for
obj_handle, did not honor it. Its init_code fed the outer obj_handle
pointer itself into ompi_convert_abi_obj_handle_intern_obj_handle(),
and its (inherited) argument returned that converted value directly as
the new obj_handle -- i.e. it treated obj_handle as the handle, not as
the address of one. Two failures followed:
* The converter's sentinel comparisons were made against the wrong
operand: a stack address, which never equals a small ABI sentinel
(256, 257, 512, ...), so binding to a PREDEFINED object (e.g.
MPI_COMM_WORLD) never matched and fell through unconverted.
* Even on a hypothetical match, the result was returned as a bare
pointer rather than the address of storage holding it, so the
downstream single dereference in ompit_obj_invalid() /
mca_base_event_handle_alloc() / mca_base_pvar_handle_alloc() would
read the wrong memory.
Concretely, MPI_T_event_handle_alloc(idx, &MPI_COMM_WORLD, ...) under
the standard ABI silently bound the registration to a bogus identity:
the tool believed the call succeeded, but the registration never
matched events raised for MPI_COMM_WORLD.
Fix TypeObjHandleStandard so its init_code dereferences obj_handle once
to reach the ABI-encoded value, converts only that pointed-to value,
and stores the result in a local temporary; its argument then passes
the ADDRESS of that temporary (or NULL) down to the internal entry
point, matching the "address of a variable holding the handle"
contract on both sides of the call.
Add a make check-abi regression probe, mpit_obj_handle_predefined,
that binds MPI_T event registrations to the predefined MPI_COMM_WORLD
and MPI_COMM_SELF communicators and confirms each fires only for its
own bound communicator. Binding to a dynamically created communicator
would not have caught this bug, because under the Open MPI ABI (and,
incidentally, for most dynamic handles under the standard ABI) the
handle value is already the real internal pointer; only a predefined
handle exercises the converter's sentinel-matching path that was
broken. Verified the new probe fails against the prior code and
passes with the fix, and that the full existing make check-abi suite,
the bindings-generator unit tests, and ompi/test/t's MPI_T tests all
still pass.
Signed-off-by: Howard Pritchard <howardp@lanl.gov>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduce a new binding parameter type,
OBJ_HANDLE, for theobj_handleargument ofMPI_T_Cvar_handle_allocandMPI_T_Event_handle_alloc. Previously these used the genericVOID(
void *) type, so under the standard MPI ABI the incoming handlewas passed straight through without being translated from the
standard-ABI integer representation to Open MPI's internal handle
pointers.
The change adds:
TypeObjHandle/TypeObjHandleStandardinc_type.py(both render asvoid *; the standard variant injects aninit_codeconversion call).ConvertFuncs.OBJ_HANDLEinconsts.py.A generator method
generate_obj_handle_convert_fn()inc.pythatemits a runtime dispatcher
(
ompi_convert_abi_obj_handle_intern_obj_handle) which inspects theinteger value of the handle and routes it to the correct per-type
converter.
The
.c.inprototype edits for the two MPI_T functions.Related to Emit MPI Standard ABI handles in MPI_T event payloads #14306