akashchamp opened a new pull request, #529: URL: https://github.com/apache/commons-bcel/pull/529
Fixes [BCEL-280](https://issues.apache.org/jira/browse/BCEL-280). ## Problem `MethodGen.setMaxLocals()` recomputes `maxLocals` only by scanning the `InstructionList` for `LocalVariableInstruction`/`RET`/`IINC` operands. It never consults the `LocalVariableGen` entries registered through `addLocalVariable()`. Some compilers (e.g. kotlinc) emit `LocalVariableTable` entries for local slots that no instruction in the method body ever references — for example unused loop variables. If BCEL copies such a method and something later calls `setMaxLocals()` again (e.g. an instrumentation pass), it silently shrinks `maxLocals` below what the local variable table entries require, producing a class file that a stricter verifier/toolchain rejects with `Invalid index N in LocalVariableTable`, as reported in the issue. ## Fix `setMaxLocals()` now also takes each registered `LocalVariableGen`'s `index + type.getSize()` into account, the same bound `addLocalVariable()` already enforces when a variable is added directly. This is a minimal, additive change to the existing scan; behavior for methods with no such "orphan" local variable entries is unchanged. ## Testing - Added `MethodGenTest.testSetMaxLocalsAccountsForLocalVariableTable()`, which builds a method with a `LocalVariableTable` entry for a slot no instruction touches. It fails on unpatched code (`maxLocals` drops from 7 to 1 after `setMaxLocals()`) and passes with the fix. - Ran the full suite (`mvn test`): 3138 tests, only the two `BCELifierTest.testJavapCompareJava25KnownBroken` cases fail, and they fail identically on unmodified `master` in this environment (a pre-existing, environment-specific `javap`/Java 25 issue unrelated to this change). - Manually built a class with `ClassGen`/`MethodGen` reproducing the reported shape (a method with an unreferenced `LocalVariableTable` entry, `setMaxLocals()` called after `addLocalVariable()`), dumped it to a real `.class` file, and loaded/ran it with a JVM `URLClassLoader`: `maxLocals` now comes out correct (7, matching the pre-`setMaxLocals()` value) instead of dropping. --- I used AI (Claude/Anthropic) to help analyze the root cause, draft the fix and the regression test, and write this description. I read the relevant source and JIRA history myself, reproduced the bug before changing anything, and ran the verification described above before opening this PR. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
