up

Author: metascroy

extension/apple/ExecuTorch/Exported/ExecuTorchBackendOption.mm

@@ -102,13 +102,18 @@ - (BOOL)isEqual:(id)object {
       return _boolValue == other.boolValue;
     case ExecuTorchBackendOptionTypeInteger:
       return _intValue == other.intValue;
-    case ExecuTorchBackendOptionTypeString:
+    case ExecuTorchBackendOptionTypeString: {
       // Both are non-null when type is String (init enforces it), but be
       // defensive in case of subclass/manual misuse.
-      if (_stringValue == other.stringValue) {
+      NSString *otherString = other.stringValue;
+      if (_stringValue == otherString) {
         return YES;
       }
-      return [_stringValue isEqualToString:other.stringValue];
+      if (_stringValue == nil || otherString == nil) {
+        return NO;
+      }
+      return [_stringValue isEqualToString:otherString];
+    }
   }
   return NO;
 }

extension/apple/ExecuTorch/Exported/ExecuTorchModule.mm

@@ -259,9 +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 it to nil would release the C++ map underneath `_module`'s
-  // borrowed pointer and re-introduce the dangling-pointer bug class this
-  // refactor was created to eliminate.
+  // Resetting to nil would release the C++ map underneath `_module`'s
+  // borrowed pointer, causing a dangling-pointer crash.
   //
   // THREAD SAFETY: like the rest of `ExecuTorchModule`, write access here
   // is not thread-safe. The ARC retain/release on assignment is non-atomic
@@ -376,11 +375,13 @@ - (BOOL)loadMethod:(NSString *)methodName
            options:(ExecuTorchBackendOptionsMap *)options
              error:(NSError **)error {
   NSParameterAssert(options);
-  // Note: unlike -loadWithOptions:..., this path passes the C++ map pointer
-  // synchronously into Module::load_method, which uses it within the same
-  // call stack and does not store it. We therefore do NOT need to retain
-  // `options` past this method's return — `options` is held alive by ARC
-  // via the local parameter for the duration of the call.
+  // 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.
+  _loadedBackendOptions = options;
   const auto errorCode = _module->load_method(methodName.UTF8String,
                                                /*planned_memory=*/nullptr,
                                                /*event_tracer=*/nullptr,

extension/apple/ExecuTorch/__tests__/ModuleTest.swift

@@ -28,7 +28,7 @@ class ModuleTest: XCTestCase {
     if let path = resourceBundle.path(forResource: name, ofType: type) {
       return path
     }
-    let message = "\(name).\(type) not bundled — run extension/apple/ExecuTorch/__tests__/resources/generate_coreml_test_models.py to generate it."
+    let message = "\(name).\(type) not bundled."
     if ProcessInfo.processInfo.environment["CI"] != nil {
       // Throw a plain Error (NOT XCTSkip) so the test is reported as failed
       // rather than skipped. The thrown error's localizedDescription is the
@@ -263,80 +263,10 @@ class ModuleTest: XCTestCase {
     XCTAssertFalse(desc.contains("0x"), "description should not include a pointer: \(desc)")
   }
 
-  func testLoadWithBackendOptions() throws {
-    guard let modelPath = resourceBundle.path(forResource: "add", ofType: "pte") else {
-      XCTFail("Couldn't find the model file")
-      return
-    }
-    let module = Module(filePath: modelPath)
-    let options = try BackendOptionsMap(options: [
-      "SomeBackend": [
-        BackendOption("num_threads", 4),
-        BackendOption("use_cache", true),
-      ]
-    ])
-    XCTAssertNoThrow(try module.load(options))
-    XCTAssertTrue(module.isLoaded())
-  }
-
-  func testLoadWithEmptyBackendOptions() throws {
-    guard let modelPath = resourceBundle.path(forResource: "add", ofType: "pte") else {
-      XCTFail("Couldn't find the model file")
-      return
-    }
-    let module = Module(filePath: modelPath)
-    let options = try BackendOptionsMap(options: [:])
-    XCTAssertNoThrow(try module.load(options))
-    XCTAssertTrue(module.isLoaded())
-  }
-
-  func testLoadMethodWithBackendOptions() throws {
-    guard let modelPath = resourceBundle.path(forResource: "add", ofType: "pte") else {
-      XCTFail("Couldn't find the model file")
-      return
-    }
-    let module = Module(filePath: modelPath)
-    let options = try BackendOptionsMap(options: [
-      "SomeBackend": [
-        BackendOption("compute_unit", "cpu_and_gpu"),
-      ]
-    ])
-    XCTAssertNoThrow(try module.load("forward", options: options))
-    XCTAssertTrue(module.isLoaded("forward"))
-  }
-
-  func testLoadWithBackendOptionsThenExecute() throws {
-    guard let modelPath = resourceBundle.path(forResource: "add", ofType: "pte") else {
-      XCTFail("Couldn't find the model file")
-      return
-    }
-    let module = Module(filePath: modelPath)
-    let options = try BackendOptionsMap(options: [
-      "SomeBackend": [
-        BackendOption("num_threads", 4),
-      ]
-    ])
-    XCTAssertNoThrow(try module.load(options))
-
-    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: when load(_:BackendOptionsMap) is followed by a lazy
-  // load_method (triggered by forward without an explicit load("forward")),
-  // the C++ LoadBackendOptionsMap held inside the BackendOptionsMap must
-  // outlive the wrapper call. The Module retains the BackendOptionsMap via
-  // ARC for exactly that reason. The per-delegate loop in Method::init
-  // would otherwise dereference a dangling pointer in strcmp and crash
-  // with EXC_BAD_ACCESS.
-  //
-  // The plain add.pte fixture does NOT trigger this because it has zero
-  // delegates, so the per-delegate loop never executes. We use a
-  // CoreML-delegated add model (add_coreml.pte, generated at CI time by
-  // resources/generate_coreml_test_models.py) which has at least one
-  // delegate.
+  // 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.
   func testLoadWithBackendOptionsThenExecuteOnCoreMLDelegatedModel() throws {
     let modelPath = try requireFixture("add_coreml", ofType: "pte")
     let module = Module(filePath: modelPath)

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

@@ -32,10 +32,7 @@ - (NSBundle *)resourceBundle {
 - (nullable NSString *)requireFixture:(NSString *)name ofType:(NSString *)type {
   NSString *path = [[self resourceBundle] pathForResource:name ofType:type];
   if (path) return path;
-  NSString *message = [NSString stringWithFormat:
-      @"%@.%@ not bundled — run "
-       "extension/apple/ExecuTorch/__tests__/resources/generate_coreml_test_models.py "
-       "to generate it.", name, type];
+  NSString *message = [NSString stringWithFormat:@"%@.%@ not bundled.", name, type];
   if (NSProcessInfo.processInfo.environment[@"CI"] != nil) {
     // CI: hard fail. XCTFail records the failure; we then return nil so the
     // caller exits early. Single failure artifact.