FIx constant pool construction and throw correct exception on overflow. - #2498
aardvark179 wants to merge 3 commits into
Conversation
gbrail
left a comment
There was a problem hiding this comment.
I'm happy to bring this in before the larger fix, but there are opportunities to make this change a bit more robust if we can.
| } | ||
|
|
||
| void setConstantData(int index, Object data) { | ||
| if ((index & 0xffff) != index) { |
There was a problem hiding this comment.
My AI friend's is convinced that 65535 is not a valid index even though it would pass this test because of the way that we often compute or store itsTopIndex + 1. (And FWIW we seem to +2 it a lot as well.) That would imply a different check here. A unit test to verify the duplication or non-duplication here would probably answer for sure.
There was a problem hiding this comment.
It's also a fair point that we make this overflow check in setConstantData but not in other places that allocate handles, including addConstant and others.
| setConstantData(itsTopIndex, r); | ||
| itsPoolTypes.put(itsTopIndex, CONSTANT_InterfaceMethodref); | ||
| index = itsTopIndex++; | ||
| } |
There was a problem hiding this comment.
Is the goal here to put the calculated value of "index" back in the "itsConstantHash" table? It doesn't appear that we do.
0a6a640 to
8345119
Compare
|
Okay, I got my robot friend to generate a bunch of unit tests and then do a more extensive rewrite. I moved all the indices to be |
2e27c3d to
233cbd6
Compare
|
I've added some simple class splitting to handle large scripts and emit a size exception if we emit too many |
233cbd6 to
a70c0d0
Compare
@gbrail this should deal with the problems you saw related to #2461 and I was seeing on my const and loop fix branch. I've put it as a separate PR because I don't know what order you want to make these changes in.