-
Notifications
You must be signed in to change notification settings - Fork 970
Back out "Re-apply D101260086: Android unified error reporting" #19201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,7 +66,8 @@ class ModuleInstrumentationTest { | |
| fun testModuleLoadMethodAndForward() { | ||
| val module = Module.load(getTestFilePath(TEST_FILE_NAME)) | ||
|
|
||
| module.loadMethod(FORWARD_METHOD) | ||
| val loadMethod = module.loadMethod(FORWARD_METHOD) | ||
| Assert.assertEquals(loadMethod.toLong(), OK.toLong()) | ||
|
|
||
| val results = module.forward() | ||
| Assert.assertTrue(results[0].isTensor) | ||
|
|
@@ -95,22 +96,17 @@ class ModuleInstrumentationTest { | |
| fun testModuleLoadMethodNonExistantMethod() { | ||
| val module = Module.load(getTestFilePath(TEST_FILE_NAME)) | ||
|
|
||
| val exception = | ||
| Assert.assertThrows(ExecutorchRuntimeException::class.java) { | ||
| module.loadMethod(NONE_METHOD) | ||
| } | ||
| Assert.assertEquals( | ||
| ExecutorchRuntimeException.INVALID_ARGUMENT, | ||
| exception.getErrorCode(), | ||
| ) | ||
| val loadMethod = module.loadMethod(NONE_METHOD) | ||
| Assert.assertEquals(loadMethod.toLong(), INVALID_ARGUMENT.toLong()) | ||
| } | ||
|
|
||
| @Test(expected = RuntimeException::class) | ||
| @Throws(IOException::class) | ||
| fun testNonPteFile() { | ||
| val module = Module.load(getTestFilePath(NON_PTE_FILE_NAME)) | ||
|
|
||
| module.loadMethod(FORWARD_METHOD) | ||
| val loadMethod = module.loadMethod(FORWARD_METHOD) | ||
| Assert.assertEquals(loadMethod.toLong(), INVALID_ARGUMENT.toLong()) | ||
| } | ||
|
Comment on lines
103
to
110
|
||
|
|
||
| @Test | ||
|
|
@@ -120,19 +116,22 @@ class ModuleInstrumentationTest { | |
|
|
||
| module.destroy() | ||
|
|
||
| Assert.assertThrows(IllegalStateException::class.java) { module.loadMethod(FORWARD_METHOD) } | ||
| val loadMethod = module.loadMethod(FORWARD_METHOD) | ||
| Assert.assertEquals(loadMethod.toLong(), INVALID_STATE.toLong()) | ||
| } | ||
|
|
||
| @Test | ||
| @Throws(IOException::class) | ||
| fun testForwardOnDestroyedModule() { | ||
| val module = Module.load(getTestFilePath(TEST_FILE_NAME)) | ||
|
|
||
| module.loadMethod(FORWARD_METHOD) | ||
| val loadMethod = module.loadMethod(FORWARD_METHOD) | ||
| Assert.assertEquals(loadMethod.toLong(), OK.toLong()) | ||
|
|
||
| module.destroy() | ||
|
|
||
| Assert.assertThrows(IllegalStateException::class.java) { module.forward() } | ||
| val results = module.forward() | ||
| Assert.assertEquals(0, results.size.toLong()) | ||
| } | ||
|
|
||
| @Ignore( | ||
|
|
@@ -176,5 +175,9 @@ class ModuleInstrumentationTest { | |
| private const val NON_PTE_FILE_NAME = "/test.txt" | ||
| private const val FORWARD_METHOD = "forward" | ||
| private const val NONE_METHOD = "none" | ||
| private const val OK = 0x00 | ||
| private const val INVALID_STATE = 0x2 | ||
| private const val INVALID_ARGUMENT = 0x12 | ||
| private const val ACCESS_FAILED = 0x22 | ||
|
Comment on lines
+178
to
+181
|
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,83 +13,34 @@ | |
| import java.util.HashMap; | ||
| import java.util.Map; | ||
|
|
||
| /** | ||
| * Base exception for all ExecuTorch runtime errors. Each instance carries an integer error code | ||
| * corresponding to the native {@code runtime/core/error.h} values, accessible via {@link | ||
| * #getErrorCode()}. | ||
| */ | ||
| public class ExecutorchRuntimeException extends RuntimeException { | ||
| // Error code constants - keep in sync with runtime/core/error.h | ||
|
|
||
| // System errors | ||
|
|
||
| /** Operation completed successfully. */ | ||
| public static final int OK = 0x00; | ||
|
|
||
| /** An unexpected internal error occurred in the runtime. */ | ||
| public static final int INTERNAL = 0x01; | ||
|
|
||
| /** The runtime or method is in an invalid state for the requested operation. */ | ||
| public static final int INVALID_STATE = 0x02; | ||
|
|
||
| /** The method has finished execution and has no more work to do. */ | ||
| public static final int END_OF_METHOD = 0x03; | ||
|
|
||
|
Comment on lines
17
to
23
|
||
| /** A required resource has already been loaded. */ | ||
| public static final int ALREADY_LOADED = 0x04; | ||
|
|
||
| // Logical errors | ||
|
|
||
| /** The requested operation is not supported by this build or backend. */ | ||
| public static final int NOT_SUPPORTED = 0x10; | ||
|
|
||
| /** The requested operation has not been implemented. */ | ||
| public static final int NOT_IMPLEMENTED = 0x11; | ||
|
|
||
| /** One or more arguments passed to the operation are invalid. */ | ||
| public static final int INVALID_ARGUMENT = 0x12; | ||
|
|
||
| /** A value or tensor has an unexpected type. */ | ||
| public static final int INVALID_TYPE = 0x13; | ||
|
|
||
| /** A required operator kernel is not registered. */ | ||
| public static final int OPERATOR_MISSING = 0x14; | ||
|
|
||
| /** The maximum number of registered kernels has been exceeded. */ | ||
| public static final int REGISTRATION_EXCEEDING_MAX_KERNELS = 0x15; | ||
|
|
||
| /** A kernel with the same name is already registered. */ | ||
| public static final int REGISTRATION_ALREADY_REGISTERED = 0x16; | ||
|
|
||
| // Resource errors | ||
|
|
||
| /** A required resource (file, tensor, program) was not found. */ | ||
| public static final int NOT_FOUND = 0x20; | ||
|
|
||
| /** A memory allocation failed. */ | ||
| public static final int MEMORY_ALLOCATION_FAILED = 0x21; | ||
|
|
||
| /** Access to a resource was denied or failed. */ | ||
| public static final int ACCESS_FAILED = 0x22; | ||
|
|
||
| /** The loaded program is malformed or incompatible. */ | ||
| public static final int INVALID_PROGRAM = 0x23; | ||
|
|
||
| /** External data referenced by the program is invalid or missing. */ | ||
| public static final int INVALID_EXTERNAL_DATA = 0x24; | ||
|
|
||
| /** The system has run out of a required resource. */ | ||
| public static final int OUT_OF_RESOURCES = 0x25; | ||
|
|
||
| // Delegate errors | ||
|
|
||
| /** A delegate reported an incompatible model or configuration. */ | ||
| public static final int DELEGATE_INVALID_COMPATIBILITY = 0x30; | ||
|
|
||
| /** A delegate failed to allocate required memory. */ | ||
| public static final int DELEGATE_MEMORY_ALLOCATION_FAILED = 0x31; | ||
|
|
||
| /** A delegate received an invalid or stale handle. */ | ||
| public static final int DELEGATE_INVALID_HANDLE = 0x32; | ||
|
|
||
| private static final Map<Integer, String> ERROR_CODE_MESSAGES; | ||
|
|
@@ -102,7 +53,6 @@ public class ExecutorchRuntimeException extends RuntimeException { | |
| map.put(INTERNAL, "Internal error"); | ||
| map.put(INVALID_STATE, "Invalid state"); | ||
| map.put(END_OF_METHOD, "End of method reached"); | ||
| map.put(ALREADY_LOADED, "Already loaded"); | ||
| // Logical errors | ||
| map.put(NOT_SUPPORTED, "Operation not supported"); | ||
| map.put(NOT_IMPLEMENTED, "Operation not implemented"); | ||
|
|
@@ -134,7 +84,7 @@ static String formatMessage(int errorCode, String details) { | |
|
|
||
| String safeDetails = details != null ? details : "No details provided"; | ||
| return String.format( | ||
| "[ExecuTorch Error 0x%s] %s: %s", | ||
| "[Executorch Error 0x%s] %s: %s", | ||
| Integer.toHexString(errorCode), baseMessage, safeDetails); | ||
| } | ||
|
|
||
|
|
@@ -163,12 +113,10 @@ public ExecutorchRuntimeException(int errorCode, String details) { | |
| this.errorCode = errorCode; | ||
| } | ||
|
|
||
| /** Returns the numeric error code from {@code runtime/core/error.h}. */ | ||
| public int getErrorCode() { | ||
| return errorCode; | ||
| } | ||
|
|
||
| /** Returns detailed log output captured from the native runtime, if available. */ | ||
| public String getDetailedError() { | ||
| return ErrorHelper.getDetailedErrorLogs(); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -8,6 +8,7 @@ | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| package org.pytorch.executorch; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| import android.util.Log; | ||||||||||||||||||||||
| import com.facebook.jni.HybridData; | ||||||||||||||||||||||
| import com.facebook.jni.annotations.DoNotStrip; | ||||||||||||||||||||||
| import com.facebook.soloader.nativeloader.NativeLoader; | ||||||||||||||||||||||
|
|
@@ -129,10 +130,11 @@ public EValue[] forward(EValue... inputs) { | |||||||||||||||||||||
| * @return return value from the method. | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| public EValue[] execute(String methodName, EValue... inputs) { | ||||||||||||||||||||||
| mLock.lock(); | ||||||||||||||||||||||
| try { | ||||||||||||||||||||||
| mLock.lock(); | ||||||||||||||||||||||
| if (!mHybridData.isValid()) { | ||||||||||||||||||||||
| throw new IllegalStateException("Module has been destroyed"); | ||||||||||||||||||||||
| Log.e("ExecuTorch", "Attempt to use a destroyed module"); | ||||||||||||||||||||||
| return new EValue[0]; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| return executeNative(methodName, inputs); | ||||||||||||||||||||||
| } finally { | ||||||||||||||||||||||
|
|
@@ -149,17 +151,17 @@ public EValue[] execute(String methodName, EValue... inputs) { | |||||||||||||||||||||
| * synchronous, and will block until the method is loaded. Therefore, it is recommended to call | ||||||||||||||||||||||
| * this on a background thread. However, users need to make sure that they don't execute before | ||||||||||||||||||||||
| * this function returns. | ||||||||||||||||||||||
| * | ||||||||||||||||||||||
| * @return the Error code if there was an error loading the method | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| public void loadMethod(String methodName) { | ||||||||||||||||||||||
| mLock.lock(); | ||||||||||||||||||||||
| public int loadMethod(String methodName) { | ||||||||||||||||||||||
| try { | ||||||||||||||||||||||
| mLock.lock(); | ||||||||||||||||||||||
| if (!mHybridData.isValid()) { | ||||||||||||||||||||||
| throw new IllegalStateException("Module has been destroyed"); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| int errorCode = loadMethodNative(methodName); | ||||||||||||||||||||||
| if (errorCode != 0) { | ||||||||||||||||||||||
| throw new ExecutorchRuntimeException(errorCode, "Failed to load method: " + methodName); | ||||||||||||||||||||||
| Log.e("ExecuTorch", "Attempt to use a destroyed module"); | ||||||||||||||||||||||
| return 0x2; // InvalidState | ||||||||||||||||||||||
|
||||||||||||||||||||||
| return 0x2; // InvalidState | |
| return ExecutorchRuntimeException.INVALID_STATE; |
Copilot
AI
Apr 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getMethods() is now a direct native call with no mLock/mHybridData.isValid() guard. This breaks the class’s existing thread-safety + destroyed-object pattern (execute/loadMethod still lock and check isValid), and it can call into freed native state after destroy(). Consider restoring a Java wrapper that locks + checks isValid (returning an empty array on destroyed), and keep the native method private (e.g., getMethodsNative).
Copilot
AI
Apr 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
readLogBuffer() now forwards directly to readLogBufferNative() without any mHybridData.isValid() check. After destroy(), this risks invoking JNI on an invalid HybridData/native pointer (potential crash). Please add the same isValid guard used by execute/loadMethod (return an empty array or similar when destroyed) and consider also guarding with mLock if the native side isn’t thread-safe.
| return readLogBufferNative(); | |
| mLock.lock(); | |
| try { | |
| if (!mHybridData.isValid()) { | |
| return new String[0]; | |
| } | |
| return readLogBufferNative(); | |
| } finally { | |
| mLock.unlock(); | |
| } |
Copilot
AI
Apr 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
etdump() is now a direct native method without an isValid check or locking. Calling this after destroy() (or concurrently with execute()) can race with native teardown and lead to crashes or corrupted dumps. Wrap the call in a Java method that acquires mLock and checks mHybridData.isValid(), returning false when destroyed.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,11 +46,5 @@ default void onStats(String stats) {} | |
| * @param message Human-readable error description | ||
| */ | ||
| @DoNotStrip | ||
| default void onError(int errorCode, String message) { | ||
| try { | ||
| android.util.Log.e("ExecuTorch", "LLM error " + errorCode + ": " + message); | ||
| } catch (Throwable t) { | ||
| System.err.println("ExecuTorch LLM error " + errorCode + ": " + message); | ||
| } | ||
| } | ||
| default void onError(int errorCode, String message) {} | ||
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test introduces a local OK constant (0x00). To keep tests aligned with the runtime’s source of truth and avoid drift, prefer using ExecutorchRuntimeException.OK (or a shared constant) instead of duplicating the numeric value here.