[BCEL-280] MethodGen.setMaxLocals() should account for LocalVariableTable entries - #529
Merged
Merged
Conversation
…able entries setMaxLocals() only scanned the InstructionList for LocalVariableInstruction/RET/IINC operands, ignoring any LocalVariableGen entries registered via addLocalVariable(). Some compilers (e.g. kotlinc) emit LocalVariableTable entries for local slots that no instruction in the method body ever touches, such as unused loop variables. When code later calls setMaxLocals() (for example after instrumentation), it silently shrinks maxLocals below what the local variable table requires, producing a class file that fails verification with "Invalid index ... in LocalVariableTable". setMaxLocals() now also considers each registered LocalVariableGen's index + type size, mirroring the bound addLocalVariable() already enforces when a variable is added directly. Added MethodGenTest.testSetMaxLocalsAccountsForLocalVariableTable(), which fails on unpatched code (maxLocals drops from 7 to 1) and passes with the fix. Generated-by: Claude (claude-sonnet-5) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No unresolved issues; the fix and regression test address the reported bug.
Review effort: Lite
Findings: None
What changed in this PR
Fixes BCEL-280 by preserving local slots referenced only by LocalVariableTable entries.
Changes:
- Include
LocalVariableGenbounds when recalculatingmaxLocals. - Add regression coverage for unreferenced local-variable slots.
| File | Summary |
|---|---|
src/test/java/org/apache/bcel/generic/MethodGenTest.java |
Tests preservation of required local slots. |
src/main/java/org/apache/bcel/generic/MethodGen.java |
Accounts for local-variable table entries. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
garydgregory
added a commit
that referenced
this pull request
Sep 25, 2026
LocalVariableTable entries (#529)
Member
|
@akashchamp |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes BCEL-280.
Problem
MethodGen.setMaxLocals()recomputesmaxLocalsonly by scanning theInstructionListforLocalVariableInstruction/RET/IINCoperands. Itnever consults the
LocalVariableGenentries registered throughaddLocalVariable().Some compilers (e.g. kotlinc) emit
LocalVariableTableentries for localslots 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 silentlyshrinks
maxLocalsbelow 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 registeredLocalVariableGen'sindex + type.getSize()into account, the same boundaddLocalVariable()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
MethodGenTest.testSetMaxLocalsAccountsForLocalVariableTable(),which builds a method with a
LocalVariableTableentry for a slot noinstruction touches. It fails on unpatched code (
maxLocalsdrops from7 to 1 after
setMaxLocals()) and passes with the fix.mvn test): 3138 tests, only the twoBCELifierTest.testJavapCompareJava25KnownBrokencases fail, and theyfail identically on unmodified
masterin this environment (apre-existing, environment-specific
javap/Java 25 issue unrelated tothis change).
ClassGen/MethodGenreproducing thereported shape (a method with an unreferenced
LocalVariableTableentry,
setMaxLocals()called afteraddLocalVariable()), dumped it toa real
.classfile, and loaded/ran it with a JVMURLClassLoader:maxLocalsnow 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.