[
https://issues.apache.org/jira/browse/GROOVY-12387?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18113330#comment-18113330
]
ASF GitHub Bot commented on GROOVY-12387:
-----------------------------------------
Copilot commented on code in PR #2909:
URL: https://github.com/apache/groovy/pull/2909#discussion_r3968791746
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/IndyCatchCompat.java:
##########
@@ -0,0 +1,117 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.codehaus.groovy.vmplugin.v8;
+
+import groovy.lang.GroovyRuntimeException;
+import groovy.lang.MissingMethodException;
+import org.codehaus.groovy.GroovyBugError;
+import org.codehaus.groovy.runtime.ScriptBytecodeAdapter;
+
+import java.lang.invoke.MethodHandle;
+import java.lang.invoke.MethodHandles;
+import java.lang.invoke.MethodType;
+
+/**
+ * Exception handling around a call-site target expressed in plain Java rather
+ * than with {@link MethodHandles#catchException} (GROOVY-12387).
+ * <p>
+ * Android's ART implements {@code catchException} with an exact class test,
+ * {@code thrown.getClass() == exType}, where the JDK applies
+ * {@code exType.isInstance(thrown)}, so an exception of a subclass of the
+ * declared type bypasses the handler (libcore {@code
Transformers.CatchException}).
+ * The runtime throws subclasses at both of the Selector's handler sites:
+ * {@code MissingMethodExceptionNoStack} where {@code MissingMethodException}
is
+ * declared, and the whole {@code GroovyRuntimeException} hierarchy at the
+ * unwrapper. On ART the GroovyObject fallback behind a failed metaclass call
+ * therefore never ran and runtime exceptions escaped unwrapped. The
+ * {@link Selector} uses these wrappers instead of the combinator when running
+ * on Android; on a JVM the
+ * combinator stays, so this class is not loaded there.
Review Comment:
This Javadoc claim is inaccurate/misleading because the class can still be
present and loaded on a JVM (e.g., unit tests in this PR reference it
directly). Consider rewording to something like: 'On a standard JVM the
combinator is used, so these wrappers are not used in production' (or similar),
without asserting class-loading behavior.
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/IndyCatchCompat.java:
##########
@@ -0,0 +1,117 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.codehaus.groovy.vmplugin.v8;
+
+import groovy.lang.GroovyRuntimeException;
+import groovy.lang.MissingMethodException;
+import org.codehaus.groovy.GroovyBugError;
+import org.codehaus.groovy.runtime.ScriptBytecodeAdapter;
+
+import java.lang.invoke.MethodHandle;
+import java.lang.invoke.MethodHandles;
+import java.lang.invoke.MethodType;
+
+/**
+ * Exception handling around a call-site target expressed in plain Java rather
+ * than with {@link MethodHandles#catchException} (GROOVY-12387).
+ * <p>
+ * Android's ART implements {@code catchException} with an exact class test,
+ * {@code thrown.getClass() == exType}, where the JDK applies
+ * {@code exType.isInstance(thrown)}, so an exception of a subclass of the
+ * declared type bypasses the handler (libcore {@code
Transformers.CatchException}).
+ * The runtime throws subclasses at both of the Selector's handler sites:
+ * {@code MissingMethodExceptionNoStack} where {@code MissingMethodException}
is
+ * declared, and the whole {@code GroovyRuntimeException} hierarchy at the
+ * unwrapper. On ART the GroovyObject fallback behind a failed metaclass call
+ * therefore never ran and runtime exceptions escaped unwrapped. The
+ * {@link Selector} uses these wrappers instead of the combinator when running
+ * on Android; on a JVM the
+ * combinator stays, so this class is not loaded there.
+ * <p>
+ * The wrappers box and collect arguments on every call, which is acceptable
+ * on ART, where method handle chains are interpreted anyway.
+ */
+final class IndyCatchCompat {
+
+ private static final MethodType INVOKE_TYPE =
+ MethodType.methodType(Object.class, Object.class, String.class,
Object[].class);
+ private static final MethodType SPREAD_TYPE =
+ MethodType.methodType(Object.class, Object[].class);
+
+ private static final MethodHandle INVOKE_WITH_FALLBACK;
+ private static final MethodHandle INVOKE_UNWRAPPING;
+
+ static {
+ try {
+ MethodHandles.Lookup lookup = MethodHandles.lookup();
+ INVOKE_WITH_FALLBACK = lookup.findStatic(IndyCatchCompat.class,
"invokeWithFallback",
+ MethodType.methodType(Object.class, MethodHandle.class,
Object.class, String.class, Object[].class));
+ INVOKE_UNWRAPPING = lookup.findStatic(IndyCatchCompat.class,
"invokeUnwrapping",
+ MethodType.methodType(Object.class, MethodHandle.class,
Object[].class));
+ } catch (ReflectiveOperationException e) {
+ throw new GroovyBugError(e);
+ }
+ }
+
+ private IndyCatchCompat() {
+ }
+
+ /**
+ * Wraps a metaclass invocation handle of type {@code (Object receiver,
+ * String name, Object[] args)Object} so that a {@link
MissingMethodException}
+ * is routed to {@link
IndyGuardsFiltersAndSignatures#invokeGroovyObjectInvoker}.
+ *
+ * @param target the metaclass invocation handle
+ * @return a handle of the same type with the fallback attached
+ */
+ static MethodHandle withGroovyObjectFallback(final MethodHandle target) {
+ return INVOKE_WITH_FALLBACK.bindTo(target.asType(INVOKE_TYPE));
+ }
+
+ /**
+ * Wraps a handle of any type so that a {@link GroovyRuntimeException}
thrown
+ * by it is replaced with {@link ScriptBytecodeAdapter#unwrap}'s result, as
+ * {@link Selector.MethodSelector#addExceptionHandler} does with the
combinator.
+ *
+ * @param target the call-site target
+ * @return a handle of the same type that unwraps runtime exceptions
+ */
+ static MethodHandle unwrapping(final MethodHandle target) {
+ MethodType type = target.type();
+ int arity = type.parameterCount();
+ MethodHandle spread = target.asSpreader(Object[].class,
arity).asType(SPREAD_TYPE);
+ return INVOKE_UNWRAPPING.bindTo(spread).asCollector(Object[].class,
arity).asType(type);
+ }
+
+ static Object invokeWithFallback(final MethodHandle target, final Object
receiver, final String name, final Object[] args) throws Throwable {
+ try {
+ return target.invokeExact(receiver, name, args);
+ } catch (MissingMethodException e) {
+ return IndyGuardsFiltersAndSignatures.invokeGroovyObjectInvoker(e,
receiver, name, args);
+ }
+ }
+
+ static Object invokeUnwrapping(final MethodHandle target, final Object[]
args) throws Throwable {
Review Comment:
These helper methods appear to be internal implementation details used only
via `findStatic` during this class’s own bootstrap. Making them `private
static` would tighten encapsulation without affecting usage (the `Lookup` is
created within the same class, so it can still access private members).
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/IndyCatchCompat.java:
##########
@@ -0,0 +1,117 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.codehaus.groovy.vmplugin.v8;
+
+import groovy.lang.GroovyRuntimeException;
+import groovy.lang.MissingMethodException;
+import org.codehaus.groovy.GroovyBugError;
+import org.codehaus.groovy.runtime.ScriptBytecodeAdapter;
+
+import java.lang.invoke.MethodHandle;
+import java.lang.invoke.MethodHandles;
+import java.lang.invoke.MethodType;
+
+/**
+ * Exception handling around a call-site target expressed in plain Java rather
+ * than with {@link MethodHandles#catchException} (GROOVY-12387).
+ * <p>
+ * Android's ART implements {@code catchException} with an exact class test,
+ * {@code thrown.getClass() == exType}, where the JDK applies
+ * {@code exType.isInstance(thrown)}, so an exception of a subclass of the
+ * declared type bypasses the handler (libcore {@code
Transformers.CatchException}).
+ * The runtime throws subclasses at both of the Selector's handler sites:
+ * {@code MissingMethodExceptionNoStack} where {@code MissingMethodException}
is
+ * declared, and the whole {@code GroovyRuntimeException} hierarchy at the
+ * unwrapper. On ART the GroovyObject fallback behind a failed metaclass call
+ * therefore never ran and runtime exceptions escaped unwrapped. The
+ * {@link Selector} uses these wrappers instead of the combinator when running
+ * on Android; on a JVM the
+ * combinator stays, so this class is not loaded there.
+ * <p>
+ * The wrappers box and collect arguments on every call, which is acceptable
+ * on ART, where method handle chains are interpreted anyway.
+ */
+final class IndyCatchCompat {
+
+ private static final MethodType INVOKE_TYPE =
+ MethodType.methodType(Object.class, Object.class, String.class,
Object[].class);
+ private static final MethodType SPREAD_TYPE =
+ MethodType.methodType(Object.class, Object[].class);
+
+ private static final MethodHandle INVOKE_WITH_FALLBACK;
+ private static final MethodHandle INVOKE_UNWRAPPING;
+
+ static {
+ try {
+ MethodHandles.Lookup lookup = MethodHandles.lookup();
+ INVOKE_WITH_FALLBACK = lookup.findStatic(IndyCatchCompat.class,
"invokeWithFallback",
+ MethodType.methodType(Object.class, MethodHandle.class,
Object.class, String.class, Object[].class));
+ INVOKE_UNWRAPPING = lookup.findStatic(IndyCatchCompat.class,
"invokeUnwrapping",
+ MethodType.methodType(Object.class, MethodHandle.class,
Object[].class));
+ } catch (ReflectiveOperationException e) {
+ throw new GroovyBugError(e);
+ }
+ }
+
+ private IndyCatchCompat() {
+ }
+
+ /**
+ * Wraps a metaclass invocation handle of type {@code (Object receiver,
+ * String name, Object[] args)Object} so that a {@link
MissingMethodException}
+ * is routed to {@link
IndyGuardsFiltersAndSignatures#invokeGroovyObjectInvoker}.
+ *
+ * @param target the metaclass invocation handle
+ * @return a handle of the same type with the fallback attached
+ */
+ static MethodHandle withGroovyObjectFallback(final MethodHandle target) {
+ return INVOKE_WITH_FALLBACK.bindTo(target.asType(INVOKE_TYPE));
+ }
+
+ /**
+ * Wraps a handle of any type so that a {@link GroovyRuntimeException}
thrown
+ * by it is replaced with {@link ScriptBytecodeAdapter#unwrap}'s result, as
+ * {@link Selector.MethodSelector#addExceptionHandler} does with the
combinator.
+ *
+ * @param target the call-site target
+ * @return a handle of the same type that unwraps runtime exceptions
+ */
+ static MethodHandle unwrapping(final MethodHandle target) {
+ MethodType type = target.type();
+ int arity = type.parameterCount();
+ MethodHandle spread = target.asSpreader(Object[].class,
arity).asType(SPREAD_TYPE);
+ return INVOKE_UNWRAPPING.bindTo(spread).asCollector(Object[].class,
arity).asType(type);
+ }
+
+ static Object invokeWithFallback(final MethodHandle target, final Object
receiver, final String name, final Object[] args) throws Throwable {
Review Comment:
These helper methods appear to be internal implementation details used only
via `findStatic` during this class’s own bootstrap. Making them `private
static` would tighten encapsulation without affecting usage (the `Lookup` is
created within the same class, so it can still access private members).
##########
src/main/java/org/codehaus/groovy/vmplugin/v8/Selector.java:
##########
@@ -1168,7 +1169,9 @@ public void setMetaClassCallHandleIfNeeded(boolean
standardMetaClass) {
// if the metaclass call fails we may still want to fall
back to call
// GroovyObject#invokeMethod if the receiver is a
GroovyObject
if (LOG_ENABLED) LOG.info("add MissingMethod handler for
GroovyObject#invokeMethod fallback path");
- handle = MethodHandles.catchException(handle,
MissingMethodException.class, GROOVY_OBJECT_INVOKER);
+ handle = AndroidSupport.isRunningAndroid()
+ ? IndyCatchCompat.withGroovyObjectFallback(handle)
// GROOVY-12387
+ : MethodHandles.catchException(handle,
MissingMethodException.class, GROOVY_OBJECT_INVOKER);
Review Comment:
The Android detection is queried inline here (and again later in this
class). If `setMetaClassCallHandleIfNeeded`/call-site construction runs
frequently, consider caching `AndroidSupport.isRunningAndroid()` in a `static
final boolean` and using that constant to avoid repeated runtime checks and
keep branching consistent across the class.
> Indy: exception-handler combinator bypassed on ART
> --------------------------------------------------
>
> Key: GROOVY-12387
> URL: https://issues.apache.org/jira/browse/GROOVY-12387
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> Involves: the Selector wraps its GroovyObject fallback and its exception
> unwrapper in MethodHandles.catchException, which ART does not honour for
> exceptions without a writable stack trace, that is all our *ExceptionNoStack
> classes. The spike routes those three sites through a Java try/catch only
> when running on Android. A proper version would use a dedicated helper per
> handle shape rather than invokeWithArguments. The ART bug itself should also
> be reported upstream, with the repro from the app.
> Impact on normal usage: none, the JVM path is untouched behind
> AndroidSupport.isRunningAndroid().
> ART Bug details below.
> h3. Background
> On Android's ART, {{MethodHandles.catchException}} does not invoke its
> handler for an exception constructed without a writable stack trace. Verified
> on an API 36 emulator with a pure-Java probe: an exception subclass
> overriding {{fillInStackTrace()}} to return {{this}}, or constructed through
> the four-argument {{Throwable}} constructor, passes straight through the
> combinator, while an ordinary exception thrown from the same target is
> handled. Groovy's control-flow exceptions {{MissingMethodExceptionNoStack}}
> and {{MissingPropertyExceptionNoStack}} are exactly that kind, so two things
> break in the indy dispatch path on ART:
> * the GroovyObject fallback installed by
> {{Selector.setMetaClassCallHandleIfNeeded}} never runs, e.g. {{null.foo()}}
> surfaces the no-stack {{MissingMethodException}} instead of the
> {{NullPointerException}} from {{NullObject.invokeMethod}};
> * the unwrapper installed by {{Selector.MethodSelector.addExceptionHandler}}
> is skipped, so the no-stack exceptions escape unwrapped instead of becoming
> {{MissingMethodException}} / {{MissingPropertyException}} with a stack trace,
> and {{InvokerInvocationException}} causes are not unwrapped.
> Minimal repro of the ART behaviour (kept in the Android hello-world spike's
> {{ApiProbeActivity.catchExceptionChecks}}):
> {code:java}
> static class Base extends RuntimeException { ... }
> static class NoStack extends Base { @Override public Throwable
> fillInStackTrace() { return this; } }
> MethodHandle target = ...; // throws new NoStack("x")
> MethodHandle guarded = MethodHandles.catchException(target, Base.class,
> handler);
> guarded.invoke(...); // handler runs on HotSpot; on ART the NoStack propagates
> {code}
> h3. Change
> New package-private {{org.codehaus.groovy.vmplugin.v8.IndyCatchCompat}} with
> the two handlers written as plain Java {{try}}/{{catch}}:
> * {{withGroovyObjectFallback(MethodHandle)}} for the metaclass invocation
> shape {{(Object receiver, String name, Object\[\] args)Object}}: catches
> {{MissingMethodException}} and delegates to the existing
> {{IndyGuardsFiltersAndSignatures.invokeGroovyObjectInvoker}};
> * {{unwrapping(MethodHandle)}} for a call-site target of any type: spreads
> and re-collects the arguments so the target's exact type is preserved
> (primitives and {{void}} included), catches {{GroovyRuntimeException}} and
> rethrows {{ScriptBytecodeAdapter.unwrap}}'s result.
> {{Selector}} uses them at its three {{catchException}} sites only when
> {{AndroidSupport.isRunningAndroid()}} is true. On a JVM the combinator code
> is unchanged and {{IndyCatchCompat}} is never loaded, so there is no dispatch
> or native-image impact. On ART the wrappers box and collect per call, which
> is acceptable there since method handle chains are interpreted anyway.
> h3. Tests
> {{IndyCatchCompatTest}} exercises the wrappers on a JVM with the no-stack
> exceptions: the fallback runs for a matching receiver and rethrows for a
> foreign one, unrelated exceptions propagate, the unwrapper keeps the
> call-site type, converts both no-stack kinds to their stack-carrying
> counterparts, unwraps {{InvokerInvocationException}} and supports {{void}}
> targets. Existing v8 plugin, {{NullObjectTest}} and {{MethodMissingTest}}
> suites pass; checkstyle gate clean. The on-device behaviour was confirmed
> with the Android spike: with the wrappers in place the hello-world probe's
> {{null.foo()}} check reports the expected {{NullPointerException}}.
> h3. Follow-up
> Report the {{catchException}} behaviour to the Android/ART project with the
> repro above.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
