RFR: 8335619: Add an @apiNote to j.l.i.ClassFileTransformer to warn about recursive class loading and ClassCircularityErrors [v3]

[Volker Simonis]

On Wed, 10 Jul 2024 12:53:46 GMT, Alan Bateman <alanb at 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