Pass the Apple event tracer's C++ handle opaquely across the framework boundary - #22396
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22396
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 43 Pending, 1 Unrelated FailureAs of commit addcfac with merge base 1cfcdf8 ( NEW FAILURE - The following job has failed:
FLAKY - The following job failed but was likely due to flakiness present on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
There was a problem hiding this comment.
Pull request overview
This PR updates the Apple event tracer handoff to cross the ExecuTorch ↔︎ ETDump framework boundary using an opaque void * handle (mirroring existing Apple wrapper patterns like ExecuTorchTensor), eliminating the need to export an ObjC++/C++-typed header and avoiding Swift umbrella parsing issues under Buck.
Changes:
- Add
initWithNativeInstance:andnativeInstancetoExecuTorchEventTracerto move astd::unique_ptr<EventTracer>opaquely across targets. - Remove the private ObjC++ category seam header and stop exporting it from
extension/apple/BUCK. - Update module and dump tracer implementations to move the native tracer through the opaque handle, and simplify the ETDump CMake header staging to only include Exported headers.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| extension/apple/ExecuTorch/Internal/ExecuTorchEventTracer+Internal.h | Deletes the ObjC++ category seam that exposed C++ types across targets. |
| extension/apple/ExecuTorch/Exported/ExecuTorchModule.mm | Switches module construction to move the C++ tracer via nativeInstance. |
| extension/apple/ExecuTorch/Exported/ExecuTorchEventTracer.mm | Implements initWithNativeInstance: and nativeInstance using std::unique_ptr<EventTracer> internally. |
| extension/apple/ExecuTorch/Exported/ExecuTorchEventTracer.h | Publicly exposes the opaque native-instance initializer/property (Swift-unavailable). |
| extension/apple/dump/ExecuTorchDump/Exported/ExecuTorchDumpTracer.mm | Updates ETDump tracer subclass to pass the tracer through initWithNativeInstance: and removes designated-initializer warning suppression. |
| extension/apple/dump/CMakeLists.txt | Stops staging core Internal headers since the dump target no longer needs them. |
| extension/apple/BUCK | Removes the special exported-header workaround for the deleted internal seam header. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| auto cppTracer = std::move( | ||
| *reinterpret_cast<std::unique_ptr<EventTracer> *>(eventTracer.nativeInstance)); |
There was a problem hiding this comment.
eventTracer is nonnull here (the initializer is inside NS_ASSUME_NONNULL_BEGIN with no nullable on the parameter), so passing nil is a contract violation the caller is warned about at compile time. For a live tracer the nativeInstance getter returns &_tracer, which is never null, so a subclass cannot return a null handle either.
This also matches the established pattern in this codebase: every nativeInstance + reinterpret_cast site derefs directly (ExecuTorchTensor.mm:123,135, ExecuTorchImageProcessor.mm:223, and the tensor path in this same file), guarding with ET_CHECK at most, never with graceful nil-degradation. The existing NSAssert/if (!cppTracer) below already handles the real runtime case (a tracer whose handle was already emptied by a prior module).
b2195b7 to
8b22b70
Compare
| auto cppTracer = | ||
| std::move(*reinterpret_cast<std::unique_ptr<EventTracer> *>( | ||
| eventTracer.nativeInstance)); | ||
| NSAssert(cppTracer != nullptr, | ||
| @"This event tracer was already given to another module. Create one " |
There was a problem hiding this comment.
eventTracer is nonnull here (the initializer is inside NS_ASSUME_NONNULL_BEGIN with no nullable), so nil is a caller contract violation flagged at compile time. The current head also guards the handle with ET_CHECK(cppTracerPtr) before dereferencing, which is exactly the established pattern (see the tensor read a few lines above: ET_CHECK(nativeTensorPtr) then deref). No nativeInstance site in this codebase does graceful nil-degradation, so matching the house pattern here.
8b22b70 to
12c7b16
Compare
12c7b16 to
58ae0e5
Compare
| #import "ExecuTorchEventTracer.h" | ||
|
|
||
| #import "ExecuTorchEventTracer+Internal.h" | ||
| #import <executorch/runtime/core/event_tracer.h> | ||
|
|
There was a problem hiding this comment.
It does compile in a clean build: the apple (*) / build and build-frameworks-ios CI jobs (OSS CMake framework build) all pass on this branch, and I also compiled the pattern locally under Xcode 26. event_tracer.h transitively provides both: its evalue.h/result.h/array_ref.h includes pull platform/assert.h (ET_CHECK), and <memory> is present in its preprocessed output. The sibling ExecuTorchTensor.mm relies on the same transitive availability (no explicit <memory> there), so this follows the module convention.
| auto generator = std::make_unique<ETDumpGen>(); | ||
| _generator = generator.get(); | ||
| self = [super initWithCppTracer:std::move(generator)]; | ||
| // The base moves out of the pointee as a std::unique_ptr<EventTracer>, so the | ||
| // local must be that type, not unique_ptr<ETDumpGen>. _generator keeps the | ||
| // concrete pointer for readback. |
There was a problem hiding this comment.
This ivar-before-super ordering is pre-existing and unchanged by this PR (_generator = generator.get() before [super init...] is identical on main). It is also safe on every path: the local unique_ptr owns the ETDumpGen, so if super returns nil it is freed at scope exit (no leak), and _generator on an abandoned instance is never read because self is nil. Reworking it would expand this PR beyond the seam change for no behavioral gain, so I'm leaving the established shape.
58ae0e5 to
4c9de12
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
extension/apple/ExecuTorch/Exported/ExecuTorchEventTracer.mm:13
ExecuTorchEventTracer.mmnow usesstd::unique_ptrbut no longer includes<memory>(the removed internal header previously provided it). This can break compilation depending on transitive includes; include<memory>explicitly here.
#import "ExecuTorchEventTracer.h"
#import <executorch/runtime/core/event_tracer.h>
using executorch::runtime::EventTracer;
| #import "ExecuTorchDumpError.h" | ||
| #import "ExecuTorchDumpTracer+Internal.h" | ||
|
|
||
| #import <ExecuTorch/ExecuTorchEventTracer+Internal.h> | ||
|
|
||
| #import <flatcc/flatcc_builder.h> | ||
|
|
There was a problem hiding this comment.
This compiles today (all six apple (*) / build jobs plus build-frameworks-ios pass on this branch) because <memory> is pulled transitively. This matches the established convention in this module: the sibling ExecuTorchTensor.mm uses std::make_unique/shared_ptr in four places and includes no <memory> either. Keeping includes minimal (no redundant transitively-provided headers) is the deliberate house style here, so I'm matching it rather than diverging for this one file.
8e9230f to
ea2b52f
Compare
ea2b52f to
a68f8d0
Compare
| /** | ||
| * Pointer to the underlying native EventTracer instance. | ||
| * | ||
| * @return A raw pointer to the native std::unique_ptr<EventTracer> held by this | ||
| * class. | ||
| */ |
There was a problem hiding this comment.
Fixed. Both the summary line and the @return now say the handle is a pointer to a std::unique_ptr, matching what initWithNativeInstance: reads back. Aligned the initializer doc the same way. This mirrors ExecuTorchTensor, whose nativeInstance is documented as a TensorPtr (a std::shared_ptr), i.e. the handle type, not the raw object.
…k boundary The event tracer's construction seam took and returned a std::unique_ptr<EventTracer> through a category in a private ObjC++ header, Internal/ExecuTorchEventTracer+Internal.h. The core module used it within the target, and the ETDump framework, a separate target, subclassed the tracer and called it across the framework boundary. That cross-target C++ header is the problem. Under Buck a private Internal header is not visible to another target, and exporting it pulls a C++-typed header into the module umbrella, which the Swift module precompile cannot parse. It also let the two targets hold independent declarations of a C++ signature whose layout varies with ET_EVENT_TRACER_ENABLED, an unchecked ABI handshake. Cross the boundary the way ExecuTorchTensor, Value and Module already do: an opaque handle. The base class takes a void * designated initializer pointing at the std::unique_ptr<EventTracer> and exposes it back through a nativeInstance property, both NS_SWIFT_UNAVAILABLE. No shared header carries a C++ type, so the private seam header is deleted, its cross-target export is removed, and the dump subclass hands its tracer up through the public initializer with no private import and no designated-initializer warning to suppress. Test Plan: Compiled the ObjC++ pattern (derived-to-base unique_ptr handoff, opaque move-out) with ARC on Xcode 26; it builds warning-clean and moves the tracer through correctly. The shape mirrors ExecuTorchTensor's initWithNativeInstance:, which ships today.
a68f8d0 to
addcfac
Compare
Problem
The Apple event tracer's C++ construction seam (
initWithCppTracer:/takeCppTracer, taking astd::unique_ptr<EventTracer>) lived in a private ObjC++ category header,Internal/ExecuTorchEventTracer+Internal.h. The core module used it within its own target; the ETDump framework — a separate target — subclassed the tracer and used it across the framework boundary.A cross-target C++ header does not work under Buck:
EXPORT_UNLESS_INTERNAL), it is not visible to the dump target — the build fails with "file not found".autoglob_additional_exported_headers), it enters the ExecuTorch Swift module umbrella, andswift_underlying_pcm_compilecannot parse a C++-typed header — the module fails to build.It also left two targets holding independent declarations of a C++ signature whose class layout varies with
ET_EVENT_TRACER_ENABLED— an unchecked cross-target ABI handshake.Fix
Cross the boundary with an opaque handle, exactly the way
ExecuTorchTensor,ExecuTorchValueandExecuTorchModulealready do (void *nativeInstance,NS_DESIGNATED_INITIALIZER,NS_SWIFT_UNAVAILABLE).ExecuTorchEventTracergains- (instancetype)initWithNativeInstance:(void *)and a readonlynativeInstanceproperty. Neither the public header nor any shared header carries a C++ type.Internal/ExecuTorchEventTracer+Internal.hseam header is deleted, and its cross-target export inextension/apple/BUCKis removed.nativeInstance(the samereinterpret_castit already uses for tensors).-Wobjc-designated-initializerssuppression (it is now a real public designated initializer).This supersedes the header-import workaround in #22363: it removes C++ from the framework boundary entirely, so the umbrella/export problem disappears in both Buck and the OSS CMake build.
Test Plan
Compiled the ObjC++ pattern (derived-to-base
unique_ptrhandoff, opaque move-out) with ARC on Xcode 26: builds warning-clean and moves the tracer through correctly. The shape mirrorsExecuTorchTensor'sinitWithNativeInstance:, which ships today.