On Fri, 26 Jul 2024 08:03:01 GMT, Adam Sotona <asot...@openjdk.org> wrote:
>> As discussed in offline meeting, the max stack and locals information are >> part of the code attribute and not meaningful for buffered code elements. >> Computation would be costly and these see no real usage during >> transformations. Thus, the proposed solution is to move these APIs to be >> CodeAttribute specific, as this is already how all these APIs' users are >> using. >> >> Also removed useless `Writable` on buffered models, and fixed >> `BufferedMethodBuilder::code` implementation. > > src/java.base/share/classes/jdk/internal/classfile/impl/BufferedCodeBuilder.java > line 67: > >> 65: this.maxLocals = Util.maxLocals(methodInfo.methodFlags(), >> methodInfo.methodTypeSymbol()); >> 66: if (original != null) >> 67: this.maxLocals = Math.max(this.maxLocals, >> original.maxLocals()); > > `original::maxLocals`set the counter for `CodeBuilder::allocateLocal` > By restricting calculation of maxLocals to "origin instanceof CodeAttribute" > may cause invalid locals allocations for chained code builders. The number > might not be exposed in the API, however we need to know it internally. I think we should just do `Util.maxLocals` without this `origin` check. Your said problem can happen too if a DirectMethodBuilder transforms a BufferedCodeBuilder.Model. Note that any code builder can receive a code model half way with `cob.transform(anyModel, CodeTransform.ACCEPT_ALL)`, which the buffered code builder initialization also uses. I think we should just ask users to use `CodeLocalsShifter` in that case, especially if the transform is inserting new locals. We cannot buffer existing chained builders for performance reasons. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20338#discussion_r1693049481
