James Fredley created GROOVY-12062:
--------------------------------------
Summary: OptimizingStatementWriter __$stMC slow branch resolves a
try-block local as getProperty when the method has a non-empty finally (classic
non-indy codegen) - regression in 5.0.x
Key: GROOVY-12062
URL: https://issues.apache.org/jira/browse/GROOVY-12062
Project: Groovy
Issue Type: Bug
Components: bytecode, class generator
Affects Versions: 5.0.6, 5.0.5, 6.0.0-alpha-1, 5.0.7
Environment: JDK 21 (Corretto 21.0.11); optimizationOptions.indy =
false (classic codegen)
Reporter: James Fredley
h2. Summary
Under classic (non-{{indy}}) codegen, {{OptimizingStatementWriter}} emits an
optimizable statement behind a {{__$stMC}} fast/slow guard built from the
*same* AST. When the method has a non-empty {{finally}} block, a local variable
declared in the {{try}} block and passed as a method-call argument is compiled
inconsistently: the fast branch ({{__$stMC=true}}) loads it from its local
slot, but the slow branch ({{__$stMC=false}}) re-resolves it as
{{this.getProperty("x")}} and throws {{MissingPropertyException}} at runtime.
The slow branch is taken depending on the receiver's metaclass state, so
correct source fails at runtime. {{indy=true}} has no {{__$stMC}} fork and is
unaffected. This is a regression: Groovy 4.0.x compiles both branches correctly.
h2. Steps to reproduce
Two pure-Groovy files - no Grails dependency, no AST transform.
*Target.groovy*
{code:groovy}
class Target {
String compute(int x) {
"ok${x}"
}
Object compute() {
try {
int x = 7
return this.compute(x) // 'x' is a try-block local
} finally {
123 // a NON-EMPTY finally is required to
trigger the bug
}
}
}
{code}
*RunRepro.groovy* - compiles {{Target.groovy}} with {{indy=false}}, then
exercises both branches:
{code:groovy}
import org.codehaus.groovy.control.CompilationUnit
import org.codehaus.groovy.control.CompilerConfiguration
File out = new File('build/repro-classes'); out.mkdirs()
def config = new CompilerConfiguration()
config.optimizationOptions.put('indy', false) // classic codegen
config.targetDirectory = out
def cu = new CompilationUnit(config)
cu.addSource(new File('Target.groovy'))
cu.compile()
def loader = new URLClassLoader([out.toURI().toURL()] as URL[],
this.class.classLoader)
Class<?> t = loader.loadClass('Target')
def stmc = t.getDeclaredField('__$stMC'); stmc.accessible = true
[false, true].each { boolean v ->
stmc.setBoolean(null, v)
def obj = t.getDeclaredConstructor().newInstance()
try {
println "__\$stMC=${v} -> ${t.getMethod('compute').invoke(obj)}"
} catch (java.lang.reflect.InvocationTargetException e) {
println "__\$stMC=${v} -> ${e.cause.class.name}: ${e.cause.message}"
}
}
{code}
Run it with: {{groovy RunRepro.groovy}}
h2. Actual result (Groovy 5.0.5 / 5.0.7-SNAPSHOT / 6.0.0-alpha-1)
{noformat}
__$stMC=false -> groovy.lang.MissingPropertyException: No such property: x for
class: Target
__$stMC=true -> ok7
{noformat}
A bare {{new Target().compute()}} (no reflection) also throws
{{MissingPropertyException}} on 5.0.x, because {{__$stMC}} is {{false}} on
first use.
h2. Expected result (as on Groovy 4.0.32)
{noformat}
__$stMC=false -> ok7
__$stMC=true -> ok7
{noformat}
h2. Affected versions
|| Groovy || stMC=false (slow path) || stMC=true (fast path) || Result ||
| 4.0.32 | ok7 | ok7 | correct |
| 5.0.5 | MissingPropertyException | ok7 | broken |
| 5.0.7-SNAPSHOT | MissingPropertyException | ok7 | broken |
| 6.0.0-alpha-1 | MissingPropertyException | ok7 | broken |
h2. Bytecode evidence
{{Target.compute()}} disassembled with {{javap -c}} (Groovy 5.0.7-SNAPSHOT,
indy=false):
{noformat}
public java.lang.Object compute();
8: getstatic #60 // Field __$stMC:Z (the fast/slow
guard)
... __$stMC=true (FAST branch):
28: iload_2 // loads local x
-> correct
44: areturn
... __$stMC=false (SLOW branch):
55: invokeinterface CallSite.callGroovyObjectGetProperty //
this.getProperty("x") -> BUG
63: invokevirtual compute:(I)String
75: areturn
{noformat}
The two branches are not semantically equivalent for the same local {{x}}.
h2. Root cause (Groovy 5.0.5 sources, org.codehaus.groovy.classgen.asm)
* {{OptVisitor.visitMethodCallExpression}} marks the {{this.compute(x)}} call
optimizable, so the enclosing {{return}} becomes a {{__$stMC}} fork and is
emitted twice.
* Whether a variable compiles to a local load or a property read is decided at
codegen by {{CompileStack.getVariable(name, false)}}
({{AsmClassGenerator.visitVariableExpression}}); a miss is rewritten to
{{this.<name>}} and emitted as {{getProperty}}.
* Emitting the non-empty {{finally}} restores/unwinds the {{CompileStack}}
scope and drops {{x}}, so on one of the two emissions {{getVariable("x")}}
misses and that branch falls back to {{this.getProperty("x")}}.
* Net: the finally-driven scope restore makes the two {{__$stMC}} emissions of
the same {{return}} see different {{CompileStack}} state for {{x}}. Groovy
4.0.x does not exhibit this.
h2. Real-world impact
Apache Grails {{ControllerActionTransformer}} generates a zero-arg action
wrapper of exactly this shape (locals bound from request params, a delegating
{{this.action(p1, ...)}} call, wrapped in {{try/catch/finally}}). Under
{{-PgrailsIndy=false}} a parameterized action throws
{{MissingPropertyException}} in any live app (apache/grails-core PR #15557).
Standalone Grails reproducer:
https://github.com/jamesfredley/groovy5-controller-action-param-scope-bug
h2. Workaround
Tag the {{ClassNode}} with {{OptimizingStatementWriter.ClassNodeSkip}} to
suppress the fork (apache/grails-core PR #15557, commit fb725b178b).
h2. Suggested fix
Keep {{try}}-block locals registered in the {{CompileStack}} across the
non-empty {{finally}} scope restore so both {{__$stMC}} emissions resolve them
identically; or skip the fast/slow fork for statements whose locals cross a
{{finally}} boundary.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
