[
https://issues.apache.org/jira/browse/GROOVY-12062?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18086236#comment-18086236
]
ASF GitHub Bot commented on GROOVY-12062:
-----------------------------------------
paulk-asert opened a new pull request, #2590:
URL: https://github.com/apache/groovy/pull/2590
…a try-block local as getProperty when the method has a non-empty finally
(classic non-indy codegen) - regression in 5.0.x
> 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: 6.0.0-alpha-1, 5.0.5, 5.0.6, 5.0.7
> Environment: JDK 21 (Corretto 21.0.11); optimizationOptions.indy =
> false (classic codegen)
> Reporter: James Fredley
> Priority: Major
>
> 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)
