Use exclusive live-range end in LocalVariableTable.getLocalVariable#517
Conversation
JVMS 4.7.13 defines a local variable's live range as the half-open interval [start_pc, start_pc + length), so a lookup at start_pc + length must not match. Change the upper comparison from pc <= endPc to pc < endPc.
|
The JVM reference is https://docs.oracle.com/javase/specs/jvms/se25/html/jvms-4.html#jvms-4.7.13 |
There was a problem hiding this comment.
Pull request overview
Fixes LocalVariableTable#getLocalVariable(int index, int pc) to match JVMS 4.7.13 local-variable live-range semantics by treating the end of the range as exclusive, preventing out-of-scope variables from being reported as live at start_pc + length.
Changes:
- Update
LocalVariableTable#getLocalVariable(index, pc)to use a half-open interval check (pc < startPc + length). - Add a unit test that asserts
pc == startPc + lengthis out of scope.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/main/java/org/apache/bcel/classfile/LocalVariableTable.java | Adjusts the live-range end check to be exclusive per JVMS. |
| src/test/java/org/apache/bcel/classfile/LocalVariableTableTest.java | Adds coverage for the exclusive-end behavior of getLocalVariable(index, pc). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final ConstantPool constantPool = new ConstantPool(new Constant[1]); | ||
| // Variable in slot 1, live at pcs 2, 3, 4 (start 2, length 3). | ||
| final LocalVariable variable = new LocalVariable(2, 3, 0, 0, 1, constantPool); | ||
| final LocalVariableTable table = new LocalVariableTable(0, 0, new LocalVariable[] {variable}, constantPool); |
There was a problem hiding this comment.
fixed in ac35334, the variable now points at valid utf8 name/signature entries so a failing assertion prints the LocalVariable instead of throwing ClassFormatException.
|
Hi @rootvector2 Please see the comment from copilot, it might be worth fixing. In the future, you can run copilot on your own PRs to see if it finds anything interesting. |
Point the test variable at real Utf8 name and signature entries so a failed assertion stringifies the LocalVariable instead of throwing ClassFormatException. Also link the JVMS se25 spec in the javadoc.
|
Done, the test now uses real utf8 entries at indices 1 and 2 so a failed assertion stays readable, and the test javadoc links the se25 spec you referenced. Full default |
|
Merged 🚀 Thank you @rootvector2 |
|
I looked at the 27-ea job: the only failure is thanks for merging this. |
LocalVariableTable.getLocalVariable(index, pc)treatspc == startPc + getLength()as in scope, but JVMS 4.7.13 defines a local variable's live range as the half-open interval[start_pc, start_pc + length), so a query atstart_pc + lengthreturns a variable that is already out of scope and reports the wrong variable as live at that program counter for a parsed class. The generation side already treats the end as exclusive (MethodGenderives length fromend - start). Found while auditing the range checks in the local-variable attributes. The added test builds a variable live over[2, 5)and assertsgetLocalVariable(1, 5)isnull, which fails before the change.mvn; that'smvnon the command line by itself.