up

Author: metascroy

extension/apple/ExecuTorch/Exported/ExecuTorchModule.mm

@@ -259,8 +259,8 @@ @implementation ExecuTorchModule {
   //
   // INVARIANT: this ivar is only ever overwritten with another non-nil
   // BackendOptionsMap, and never reset to nil while `_module` is alive.
-  // Resetting to nil would release the C++ map underneath `_module`'s
-  // borrowed pointer, causing a dangling-pointer crash.
+  // Resetting to nil would release the C++ map while `_module` still holds
+  // a borrowed pointer into it.
   //
   // THREAD SAFETY: like the rest of `ExecuTorchModule`, write access here
   // is not thread-safe. The ARC retain/release on assignment is non-atomic
@@ -375,12 +375,12 @@ - (BOOL)loadMethod:(NSString *)methodName
            options:(ExecuTorchBackendOptionsMap *)options
              error:(NSError **)error {
   NSParameterAssert(options);
-  // Defensively retain the options for the Module's lifetime, matching
-  // -loadWithOptions:. Today Module::load_method consumes the pointer
-  // synchronously and does not cache it, so retention is not strictly
-  // required for this call. We retain anyway so that a future change to
-  // the C++ API that caches the pointer cannot silently introduce a
-  // dangling-pointer bug in this wrapper.
+  // Retain the options for the Module's lifetime, matching -loadWithOptions:.
+  // Today Module::load_method consumes the pointer synchronously and does
+  // not cache it, so retention is not strictly required for this call.
+  // We retain uniformly so the ObjC ownership rule is the same for both
+  // entry points: any BackendOptionsMap handed to the Module stays alive
+  // as long as the Module does.
   _loadedBackendOptions = options;
   const auto errorCode = _module->load_method(methodName.UTF8String,
                                                /*planned_memory=*/nullptr,

extension/apple/ExecuTorch/__tests__/ModuleTest.swift

@@ -263,10 +263,10 @@ class ModuleTest: XCTestCase {
     XCTAssertFalse(desc.contains("0x"), "description should not include a pointer: \(desc)")
   }
 
-  // Regression test for the lazy load_method path: after load(options),
-  // forward() triggers load_method which must still see valid backend
-  // options. Requires a delegated model — the plain add.pte has no
-  // delegates and so does not exercise the code path.
+  // Exercises the full feature end-to-end against a delegated model:
+  // load(options) installs backend options, then forward() triggers lazy
+  // load_method which consumes them. A delegated fixture is required so
+  // the per-delegate option lookup actually runs.
   func testLoadWithBackendOptionsThenExecuteOnCoreMLDelegatedModel() throws {
     let modelPath = try requireFixture("add_coreml", ofType: "pte")
     let module = Module(filePath: modelPath)
@@ -278,17 +278,17 @@ class ModuleTest: XCTestCase {
     ])
     XCTAssertNoThrow(try module.load(options))
     // No explicit load("forward") here — exercise the lazy load_method path
-    // that previously dereferenced a dangling LoadBackendOptionsMap.
+    // that consumes the retained LoadBackendOptionsMap.
     let inputs: [Tensor<Float>] = [Tensor([1]), Tensor([1])]
     var outputs: [Value]?
     XCTAssertNoThrow(outputs = try module.forward(inputs))
     XCTAssertEqual(outputs?.first?.tensor(), Tensor([Float(2)]))
   }
 
-  // Regression test: calling load(_:BackendOptionsMap) twice on the same
-  // Module must remain safe. The Module retains the most recently passed
-  // map via ARC; the previous one is released only after the new one is
-  // installed, so the C++ pointer it stored is always valid.
+  // Calling load(_:BackendOptionsMap) repeatedly replaces the installed
+  // options. The Module retains the most recently passed map via ARC; the
+  // previous one is released only after the new one is installed, so the
+  // C++ pointer it stored is always valid.
   func testRepeatedLoadWithBackendOptionsThenExecuteOnCoreMLDelegatedModel() throws {
     let modelPath = try requireFixture("add_coreml", ofType: "pte")
     let module = Module(filePath: modelPath)

extension/apple/ExecuTorch/__tests__/ObjC/ModuleTestObjC.m

@@ -45,10 +45,9 @@ - (nullable NSString *)requireFixture:(NSString *)name ofType:(NSString *)type {
   return nil;
 }
 
-// Mirror of the Swift testLoadWithBackendOptionsThenExecuteOnCoreMLDelegatedModel
-// regression test, exercising the ObjC API surface directly so we don't lose
-// coverage if Swift overlays change. See ModuleTest.swift for the full
-// description of the bug class this protects against.
+// Mirrors the Swift testLoadWithBackendOptionsThenExecuteOnCoreMLDelegatedModel,
+// exercising the ObjC API surface directly so coverage does not depend on
+// the Swift overlays.
 - (void)testLoadWithOptionsThenExecuteOnCoreMLDelegatedModel {
   NSString *modelPath = [self requireFixture:@"add_coreml" ofType:@"pte"];
   if (!modelPath) return;
@@ -64,8 +63,8 @@ - (void)testLoadWithOptionsThenExecuteOnCoreMLDelegatedModel {
   ExecuTorchModule *module = [[ExecuTorchModule alloc] initWithFilePath:modelPath];
   XCTAssertTrue([module loadWithOptions:options error:&error], @"%@", error);
 
-  // No explicit -loadMethod: — exercise the lazy load_method path that was
-  // the original regression site.
+  // No explicit -loadMethod: — exercise the lazy load_method path that
+  // consumes the retained backend options map.
   ExecuTorchTensor *one =
       [[ExecuTorchTensor alloc] initWithScalars:@[@1.0f] dataType:ExecuTorchDataTypeFloat];
   NSArray<ExecuTorchValue *> *outputs =

extension/apple/ExecuTorch/__tests__/resources/generate_coreml_test_models.py

@@ -3,8 +3,9 @@
 Currently produces:
   - add_coreml.pte: a CoreML-delegated tensor-add model whose forward(x, y)
     returns x + y. Used by ModuleTest.testLoadWithBackendOptionsThenExecuteOnCoreML
-    as a regression fixture for the LoadBackendOptionsMap lifetime fix in the
-    ObjC wrapper.
+    to exercise the BackendOptionsMap lifetime path end-to-end against a
+    delegated model (add.pte has no delegates, so the per-delegate option
+    lookup path is only exercised with a delegated fixture like this one).
 
 Usage:
     python extension/apple/ExecuTorch/__tests__/resources/generate_coreml_test_models.py