FW: [External] : Re: [openjdk/jdk-sandbox] Classfile API stack map processing (PR #32)

[Adam Sotona]

It seems that there are some issue delivering my replies, so forwarding to  directly:

From: Brian Goetz <notifications at github.com>

In reviewing this PR, I noticed a vestige of an earlier abandoned
direction, which is CodeElement.Kind.  There are Kind values for
PARAMETER_ANNOTATION (basically unused), TYPE_ANNOTATION (misused), and
you added one for STACK_MAP.  I think all three of these should be
replaced with a single ATTRIBUTE kind, and the codeKind() of any
attribute that extends CodeElement should be ATTRIBUTE.  The END kind is
also effectively dead and should be removed (it is used only by a dead
opcode.)

OK, I’ll work on CodeElement.Kind removal.


This vestige is also present in your stack map attribute:

        @Override
        public Opcode opcode() {
            return Opcode.TYPE_ANNO;
        }

I cannot find this one.


which is clearly the wrong opcode to return for stack maps, but its also
the wrong thing to return for type annotation attributes as well.
Probably best to create a pseudo opcode for ATTRIBUTE to return in these
places too.


OK, will use common pseudo opcode ATTRIBUTE.

My reading of this PR is that:

 - You add a new option, PROCESS_STACK_MAP
 - The behavior of PSM is to deliver the StackMap attribute whole when
iterating the code elements (as we do with e.g. RVTA).
 - If SM generation has been disabled, and the user has send a stack
map attribute to the builder, we can write that (?)
Yes.


I like that you have to disengage the automatic stack map generation in
order to be able to generate your own, but this last bullet still makes
me a little uncomfortable.  I'll think about how to make this more clear.

It is an important corner-case (<5%), so we agreed it can be a bit uncomfortable.


In any case, I think you have a bug in DirectCodeBuilder::buildContent.
You either generate or reuse a stack map attribute, *and then you write
all the attributes*. This looks like it may case a second SMA to be
written in some cases.

If user provides custom SMA and does not disable generation/reuse – it is overridden.
AttributeHolder::withAttribute checks if AttributeMapper::allowMultiple and overrides the user provided SMA with the generated/reused one.


The stack map attribute is similar to the bootstrap methods attribute;
while it is an attribute, it is logically part of, and tightly coupled
to, something else (for BMA, the constant pool.)  When we write the
Class attributes, we explicitly filter the BMA; we should probably do so
here -- instead of attributes.writeTo, we should iterate the attributes
and write all but a SMA.

SMA has special treatment - it is not streamed. When SMA appears in attributes, it is always explicitly user-provided instance.
If user reads or transforms a class and needs to access original SMA, it is available in the model only (not in the stream).
User can manually pass original (or modified) SMA to the CodeBuilder and it will be applied only when SM generation is disabled.
No standard transformation will pass SMA through.
This was the latest output from our SMA-treatment discussions.




-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/classfile-api-dev/attachments/20221012/b499eed6/attachment-0001.htm>