On Wed, 10 Jul 2024 12:53:46 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> Volker Simonis has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Addressed @AlanBateman's suggestions and updated copyright year
>
> src/java.instrument/share/classes/java/lang/instrument/ClassFileTransformer.java
> line 158:
>
>> 156: *
>> 157: * <P>
>> 158: * Note the term <i>class file</i> is used as defined in section {@jvms
>> 4} The
>
> The existing sentence uses "section" but I assume it should be "Chapter".
Correct, it was section 3.1 before but now it's chapter 4. Fixed.
> src/java.instrument/share/classes/java/lang/instrument/ClassFileTransformer.java
> line 167:
>
>> 165: * same time required during the transformation process as this can
>> lead to class
>> 166: * circularity or linkage errors. Using bytecode transformation
>> libraries which depend
>> 167: * on core JDK class can increase the risk of such errors.
>
> It's probably impossible to create a BCI library that don't depend on JDK
> core classes so maybe it would be better to drop the second sentence.
Fixed.
> src/java.instrument/share/classes/java/lang/instrument/ClassFileTransformer.java
> line 179:
>
>> 177: * This means that a {@link LinkageError} triggered during
>> transformation of
>> 178: * {@code C} in a class {@code D} not directly related to {@code C} can
>> repeatedly
>> 179: * occur later in arbitrary user code which uses {@code D}.
>
> This paragraph looks okay but I can't help thinking we should have something
> in normative text to reference that specifies the reentrancy behavior. Maybe
> I missed it but I thought we have something in the API docs on this.
I haven't found anything either. The only specification-relevant mentioning of
the issue I found is in the [JVMTI
Specification](https://docs.oracle.com/en/java/javase/21/docs/specs/jvmti.html#bci)
referenced at the beginning of the PR:
> Care must be taken to avoid perturbing dependencies, especially when
> instrumenting core classes.
The example that follows describes an infinite recursion when instrumenting the
the `j.l.Object()` constructor.
I think the exact reentrancy behavior isn't specified anywhere. Not even the
exact that should be thrown in such a case is specified (see [8164165: JVM
throws incorrect exception when ClassFileTransformer.transform() triggers class
loading of class already being
loaded](https://bugs.openjdk.org/browse/JDK-8164165) for a discussion of
different scenarios).
I think the real problem is that the JVMS predates the JVMTI specification and
the interaction between instrumentation and class loading isn't clearly
defined. I think it might even be possible to treat class loading errors during
transformation differently, such that they will not lead to a permanent
resolution error for the corresponding constant pool entries. I know that this
will violate the current section ยง 5.4.3 Resolution
(https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-5.html#jvms-5.4.3) of
the JVM specification which mandates that "if an attempt by the Java Virtual
Machine to resolve a symbolic reference fails because an error is thrown that
is an instance of LinkageError (or a subclass), then subsequent attempts to
resolve the reference always fail with the same error that was thrown as a
result of the initial resolution attempt". But as I wrote, that predates JVMTI
and when JVMTI was added, we missed the opportunity to specify its exact impact
on class loading an
d resolution.
But all this is a much bigger discussion. Maybe we should open another issue
for it?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20011#discussion_r1672636821
PR Review Comment: https://git.openjdk.org/jdk/pull/20011#discussion_r1672637843
PR Review Comment: https://git.openjdk.org/jdk/pull/20011#discussion_r1672663335
