Emit MPI Standard ABI handles in MPI_T event payloads - #14306
Conversation
Open MPI's MPI_T event producers publish MPI object handles (communicator, window, session, error handler, file) in their event payloads. Those values were always the internal object pointer, which is correct only for a tool built against the Open MPI ABI. A tool built against the MPI Standard ABI expects the integer handle instead, so every producer raise site had a placeholder that set the Standard-ABI handle to 0 pending this work. Record the process ABI at initialization and convert handles at each raise site accordingly. The Standard-ABI variants of MPI_Init, MPI_Init_thread, and MPI_Session_init set the process-global flag ompi_mpit_callback_abi to OMPI_MPIT_ABI_STANDARD (the Open MPI ABI keeps the default). Keying off process init, rather than callback registration, also covers the world-model initialization event, which is raised during MPI_Init before any tool can register a callback. The intern-to-ABI converters are generated only into libmpi_abi, a higher layer than the producer raise sites in libopen_mpi, so a raise site cannot call them directly without violating the one-way OPAL/OMPI linker boundary. Resolve this with the same downward callback-registration pattern the tree already uses for ompi_mpiext_init: the Standard-ABI init path installs a converter function pointer into libopen_mpi via ompi_mpit_register_abi_handle_convert(), and the raise sites reach it through ompi_mpit_abi_handle(). The converter itself lives in the new libmpi_abi-only translation unit mpit_abi_handle_convert.c and dispatches on the object's MPI_T_BIND_* kind to the appropriate generated reverse converter. When no converter is registered (the Open MPI ABI never installs one), ompi_mpit_abi_handle() returns 0, matching the previous placeholder rather than emitting a wrong pointer value. Rewrite all handle-emitting raise sites (communicator create/free/ name-set, window create/free, instance init/finalize, and error-handler invocation, including both the error-handler handle and the invoking object converted per its binding kind) to use the new helper in the Standard-ABI branch. Make the Standard-ABI copy of MPI_T_event_register_callback re-assert the flag as well, and drop its stale comment claiming the feature was unimplemented. Add an MPI Standard ABI test probe (callback_mpit_event_handle) to the mpi-abi harness: under a libmpi_abi-linked process it drives the communicator-created event and asserts the payload carries the Standard-ABI integer handle of the new communicator, exercising the downward converter registration and the raise-site conversion end to end. Update the MPI_T events specification to describe the registered converter and the init-time flag, and add a v6.1.x changelog entry. Signed-off-by: Howard Pritchard <howardp@lanl.gov>
|
this can't be backported till #14307 is merged in to 6.0.x |
jsquyres
left a comment
jsquyres
left a comment
There was a problem hiding this comment.
Review Summary
Verdict: REQUEST_CHANGES
Findings (4 validated from 12 raw)
- Agreed (cross-source): 0
- codex-reviewer: 1
- v3-reviewer: 3
- Description accuracy: 0
Filter Pipeline
- Dropped (no quote): 0
- Dropped (hallucination): 0 (path: 0, quote: 0, deleted: 0)
- Suppressed (dedup): 0
- Dropped (below cutoff): 8
codex-reviewer
[Edge cases and error handling | MEDIUM] ompi/mpi/c/session_init.c.in:74 — see inline comment
v3-reviewer
[Bugs and logic errors | HIGH] ompi/errhandler/errhandler_invoke.c:87 — see inline comment
[Cross-file consistency | MEDIUM] ompi/communicator/comm.c:2366 — see inline comment
[Edge cases and error handling | MEDIUM] ompi/mpi/tool/event_register_callback.c.in:47 — see inline comment
| * converter downward into libopen_mpi (the converter lives here in | ||
| * libmpi_abi; see mpit_abi_handle_convert.c), mirroring the mpiext_init | ||
| * registration above. */ | ||
| ompi_mpit_callback_abi = OMPI_MPIT_ABI_STANDARD; |
There was a problem hiding this comment.
Converter installation occurs after the earlier parameter-validation branches that jump to fn_exit. A Standard-ABI tool may legally initialize MPI_T and register the errhandler-invoked callback before initializing MPI. If a subsequent MPI_Session_init fails those early checks, fn_exit invokes the error handler while event_register_callback has already selected OMPI_MPIT_ABI_STANDARD but no converter has been installed; ompi_mpit_abi_handle() consequently publishes 0 for handles such as the predefined error handler. Install the converter when the Standard-ABI MPI_T interface is initialized or when its callback is registered, or move this installation ahead of all Session_init error exits.
| handler routed before MPI_INIT) has object_bind == NO_OBJECT and | ||
| a 0 object_handle, matching the Open MPI ABI's "0 when not | ||
| available" behavior. */ | ||
| payload.errhandler_handle |
There was a problem hiding this comment.
The Standard-ABI branch converts the two handle elements of the errhandler_invoked payload but leaves the other two elements — payload.err_code = (int32_t) err_code; and payload.object_type = object_bind; (lines 72-73, set unconditionally before the branch) — in Open MPI internal representation. Both differ from the MPI Standard ABI's mandated values. Verified against docs/mpi-standard-5.0-abi.json: the Standard ABI's MPI_T_BIND_* values are exactly internal+1 (MPI_T_BIND_NO_OBJECT=1, MPI_T_BIND_MPI_COMM=2, ..., MPI_T_BIND_MPI_SESSION=12, vs. the internal enum in ompi/include/mpi.h.in which starts at 0), so a Standard-ABI tool reading object_type misclassifies every event — e.g. an errhandler invoked on a communicator publishes 1, which the tool's mpi.h defines as MPI_T_BIND_NO_OBJECT. Likewise 20 of 64 MPI_ERR_* codes have different values in the Standard ABI (e.g. internal MPI_ERR_PENDING=19 is the Standard ABI's MPI_ERR_IN_STATUS, and vice versa), so err_code is also misread for a third of the error space. The generated converters for both already exist (ompi_convert_t_bind_ompi_to_standard, ompi_convert_intern_error_abi_error in ompi_bindings/consts.py); the fix is to extend the downward-registered converter interface (or register companion value-converter callbacks) so the Standard-ABI branch converts err_code and object_type too, moving those two assignments into the per-ABI branches.
| /* TODO ABI (#13280): set the MPI Standard ABI handle value. */ | ||
| payload.handle = 0; | ||
| /* MPI Standard ABI: publish the integer MPI_Comm handle. */ | ||
| payload.handle = ompi_mpit_abi_handle(comm, MPI_T_BIND_MPI_COMM); |
There was a problem hiding this comment.
The communicator_name_set event is object-bound: a tool can only receive this (now correctly converted) payload after binding a registration to a specific communicator via MPI_T_event_handle_alloc's obj_handle. That binding-side conversion is still missing: ompi/mpi/tool/event_handle_alloc.c.in passes obj_handle straight to mca_base_event_handle_alloc with no Standard-ABI-to-internal conversion (and the spec's sec. 6.1 bullet retained in this diff still says "The Standard ABI must convert"). Under the Standard ABI, binding works for user-created communicators only by accident (their ABI handle happens to equal the internal pointer) and silently never matches for predefined handles such as MPI_COMM_WORLD, whose Standard-ABI handle is a small reserved integer. If the binding side is deliberately deferred to a follow-up, the spec's sec. 10 test-coverage claim of "end to end" Standard-ABI coverage overstates what is exercised; either close the gap or note it explicitly as remaining work in specs/mpi-t-events/spec.md.
There was a problem hiding this comment.
sorry this event_handle_alloc.c.in changes will be deferred to a separate PR. Comment about deferring solution concerning event_register_callback.c.in is incorrect.
| Standard-ABI tool registering after an intervening reinit is still seen | ||
| correctly; this is idempotent with the init-time set. */ | ||
| #if OMPI_ABI_SRC | ||
| ompi_mpit_callback_abi = OMPI_MPIT_ABI_STANDARD; |
There was a problem hiding this comment.
A Standard-ABI tool may legally call MPI_T_init_thread + MPI_T_event_register_callback before MPI_Init (MPI_T is init-independent, and the producers are registered from the MPI_T_init_thread path). In that window this re-assert correctly sets ompi_mpit_callback_abi = OMPI_MPIT_ABI_STANDARD, but the handle converter is only installed by the init entry points (init.c.in / init_thread.c.in / session_init.c.in), so any event raised before MPI init — notably ompi.mpi.errhandler_invoked for errors routed to a predefined handler pre-init, a case the code's own comments call out — publishes handle 0 via the ompi_mpit_abi_handle() NULL-converter fallback, even though the predefined errhandler has a well-defined Standard-ABI handle. Since this Standard-ABI copy of the template is compiled into the same library (libmpi_abi) as ompi_mpit_abi_handle_convert_impl, the converter could also be installed here (or in the Standard-ABI copy of MPI_T_init_thread) under the same #if OMPI_ABI_SRC guard, closing the window with two lines.
There was a problem hiding this comment.
deferring solution to this problem till a later PR. We'll solve this by adding more types and a new convertor to the bindings framework
There was a problem hiding this comment.
this should have been address by commit b26d9a1
Signed-off-by: Howard Pritchard <howardp@lanl.gov>
…it gaps The initial Standard-ABI MPI_T event work converted object handles in event payloads but left several gaps that a Standard-ABI MPI_T tool would observe as wrong values or missed deliveries. Address the review feedback on that work. Install converters before Session_init error exits. In the Standard-ABI copy of MPI_Session_init the process-ABI flag and the handle converter were installed after the MPI_PARAM_CHECK branches that jump to fn_exit. A failing check routes through OMPI_ERRHANDLER_RETURN on MPI_SESSION_NULL, which can invoke an error handler and raise ompi.mpi.errhandler_invoked before any converter is installed, publishing 0 for handles such as the predefined error handler. Move the whole registration block to the top of the function, ahead of all early error exits. Install converters in the Standard-ABI event_register_callback. MPI_T is init-independent: a tool may call MPI_T_init_thread and MPI_T_event_register_callback before MPI_Init, and events can be raised in that window. The re-assert of the process-ABI flag was already here, but the converters were installed only by the init entry points, so pre-init events fell back to the NULL-converter (handle 0). Install the converters here too, under the same OMPI_ABI_SRC guard; this Standard-ABI copy is compiled into libmpi_abi alongside the converter implementations, and the registration is idempotent with the init-time set. Convert the err_code and object_type payload fields, not just handles. The errhandler_invoked payload also carries an MPI error code and an MPI_T_BIND_* object-binding kind, whose numeric encodings differ between the Open MPI ABI and the MPI Standard ABI (roughly a third of the MPI_ERR_* space differs, and the Standard-ABI MPI_T_BIND_* values are the internal values + 1). These were left in internal representation, so a Standard-ABI tool misread them. The generated value converters live in libmpi_abi (the upper layer), so plumb them downward the same way as the handle converter: add ompi_mpit_register_abi_error_convert() / ompi_mpit_register_abi_bind_convert() function-pointer slots in libopen_mpi and ompi_mpit_abi_error() / ompi_mpit_abi_bind() forwarders (which return the value unchanged when unregistered, correct for the Open MPI ABI). The implementations wrap the generated ompi_convert_intern_error_abi_error() and ompi_convert_t_bind_ompi_to_standard(). All Standard-ABI init entry points and the event_register_callback copy install them, and the errhandler_invoke raise site converts both fields. Document the remaining binding-side gap. Object-bound delivery still lacks a Standard-ABI-to-internal conversion of the tool's obj_handle in event_handle_alloc.c.in, so predefined handles such as MPI_COMM_WORLD never match under the Standard ABI. Record this as deferred work in the MPI_T events spec and qualify the "end to end" test-coverage claim accordingly. Signed-off-by: Howard Pritchard <howardp@lanl.gov>
|
@jsquyres pushed some changes please rereview. |
|
@jsquyres ping |
2 participants
Open MPI's MPI_T event producers publish MPI object handles (communicator, window, session, error handler, file) in their event payloads. Those values were always the internal object pointer, which is correct only for a tool built against the Open MPI ABI. A tool built against the MPI Standard ABI expects the integer handle instead, so every producer raise site had a placeholder that set the Standard-ABI handle to 0 pending this work.
Record the process ABI at initialization and convert handles at each raise site accordingly. The Standard-ABI variants of MPI_Init, MPI_Init_thread, and MPI_Session_init set the process-global flag ompi_mpit_callback_abi to OMPI_MPIT_ABI_STANDARD (the Open MPI ABI keeps the default). Keying off process init, rather than callback registration, also covers the world-model initialization event, which is raised during MPI_Init before any tool can register a callback.
The intern-to-ABI converters are generated only into libmpi_abi, a higher layer than the producer raise sites in libopen_mpi, so a raise site cannot call them directly without violating the one-way OPAL/OMPI linker boundary. Resolve this with the same downward callback-registration pattern the tree already uses for ompi_mpiext_init: the Standard-ABI init path installs a converter function pointer into libopen_mpi via ompi_mpit_register_abi_handle_convert(), and the raise sites reach it through ompi_mpit_abi_handle(). The converter itself lives in the new libmpi_abi-only translation unit mpit_abi_handle_convert.c and dispatches on the object's MPI_T_BIND_* kind to the appropriate generated reverse converter. When no converter is registered (the Open MPI ABI never installs one), ompi_mpit_abi_handle() returns 0, matching the previous placeholder rather than emitting a wrong pointer value.
Rewrite all handle-emitting raise sites (communicator create/free/ name-set, window create/free, instance init/finalize, and error-handler invocation, including both the error-handler handle and the invoking object converted per its binding kind) to use the new helper in the Standard-ABI branch. Make the Standard-ABI copy of MPI_T_event_register_callback re-assert the flag as well, and drop its stale comment claiming the feature was unimplemented.
Add an MPI Standard ABI test probe (callback_mpit_event_handle) to the mpi-abi harness: under a libmpi_abi-linked process it drives the communicator-created event and asserts the payload carries the Standard-ABI integer handle of the new communicator, exercising the downward converter registration and the raise-site conversion end to end.
Update the MPI_T events specification to describe the registered converter and the init-time flag, and add a v6.1.x changelog entry.