Re: RFR: 8304425: ClassHierarchyResolver from Reflection [v7]
> Add API to explore Class Hierarchy with a `ClassLoader` or a `Lookup` with > proper privileges, with tests. > > This addition is useful in case classes at runtime are not loaded from the > system class loader, such as Proxy. This is also useful to APIs that generate > bytecode with a `Lookup` object, such as a custom single-abstract-method > class implementations from a method handle. > > See > https://mail.openjdk.org/pipermail/classfile-api-dev/2023-March/000249.html > as well. > > Current questions, which I wish to discuss with @asotona: > 1. Should the resolver fail fast on `IllegalAccessException` from the lookup? > This usually indicates the hierarchy resolver is set up improperly, and > proceeding may simply yield verification errors in class loading that are > hard to track. For bytecode-generating APIs, throwing access errors for the > Lookup eagerly is also more preferable than later silent generation failure. > 2. Whether the default resolver should be reading from jrt alone, reflection > alone, or jrt then reflection. I personally believe reflection alone is more > reliable, for classes may redefined with instrumentation or jfr, which may > not be reflected in the system resources. > 3. In addition, I don't think chaining system class loader reflection after > system resource retrieval is really meaningful: is there any case where > reflection always works while the system resource retrieval always fails? Chen Liang has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 10 additional commits since the last revision: - rename to ofClassLoading/ofResourceParsing convert the default class provider to bypass security manager restrictions - Merge branch 'master' into hierarchy-resolve - Merge branch 'master' into hierarchy-resolve - Test both cached and uncached resolvers - Update the class hierarchy resolver api as per mailing list last week - Merge branch 'master' into hierarchy-resolve - Update src/java.base/share/classes/jdk/internal/classfile/impl/ClassHierarchyImpl.java Co-authored-by: Andrey Turbanov - Make lookup based resolver throw on illegal access eagerly - 8304425: ClassHierarchyResolver from Reflection - ClassHierarchyResolver using Reflection - Changes: - all: https://git.openjdk.org/jdk/pull/13082/files - new: https://git.openjdk.org/jdk/pull/13082/files/78c8f2f7..be9fa4d0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13082&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13082&range=05-06 Stats: 356591 lines in 3790 files changed: 293420 ins; 37875 del; 25296 mod Patch: https://git.openjdk.org/jdk/pull/13082.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13082/head:pull/13082 PR: https://git.openjdk.org/jdk/pull/13082
Re: RFR: 8304425: ClassHierarchyResolver from Reflection [v7]
On Wed, 26 Apr 2023 09:47:58 GMT, Adam Sotona wrote: > > @asotona Just curious, what's the current state of our new API model of > > caching class hierarchy info in a Classfile context object as we've > > discussed on the mailing list? Will you create a patch, or should I update > > this patch to that model? > > In the discussion I tried to fine-tune exact naming of the factory methods > and the default. Mainly to differentiate when the class is parsed as a > resource `ofResourceParsing(ClassLoader loader)` and when it is loaded > `ofClassLoading(ClassLoader loader)` > > Otherwise this patch looks good. > > The other part of the discussion with proposed `ClassfileReaderWriter` model > is out of the scope of this topic, it has much bigger impact on all existing > code and I would deferred it at least until this and other open PRs are > merged. Thanks for your evaluation, Adam. I have updated the method names to `ofClassLoading` and `ofResourceParsing`, and incorporated the idea of SecurityManager bypassing from https://github.com/openjdk/jdk/pull/13197/commits/cc6994202c93edd0c98e6fb0bb5f0cbe1da7df8e#diff-f044f7ab894bab3b36ad9f29e0b3f541df5246c90159635159343ce20e02c12cR54-R63 to ensure system libraries can use the hierarchy resolver all fine. I've ran `test/jdk/jdk/classfile` tests on my machine. Can you review again? - PR Comment: https://git.openjdk.org/jdk/pull/13082#issuecomment-1540326710
Re: RFR: 8304425: ClassHierarchyResolver from Reflection [v7]
On Tue, 9 May 2023 15:09:54 GMT, Chen Liang wrote:
>> Add API to explore Class Hierarchy with a `ClassLoader` or a `Lookup` with
>> proper privileges, with tests.
>>
>> This addition is useful in case classes at runtime are not loaded from the
>> system class loader, such as Proxy. This is also useful to APIs that
>> generate bytecode with a `Lookup` object, such as a custom
>> single-abstract-method class implementations from a method handle.
>>
>> See
>> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-March/000249.html
>> as well.
>>
>> Current questions, which I wish to discuss with @asotona:
>> 1. Should the resolver fail fast on `IllegalAccessException` from the
>> lookup? This usually indicates the hierarchy resolver is set up improperly,
>> and proceeding may simply yield verification errors in class loading that
>> are hard to track. For bytecode-generating APIs, throwing access errors for
>> the Lookup eagerly is also more preferable than later silent generation
>> failure.
>> 2. Whether the default resolver should be reading from jrt alone, reflection
>> alone, or jrt then reflection. I personally believe reflection alone is more
>> reliable, for classes may redefined with instrumentation or jfr, which may
>> not be reflected in the system resources.
>> 3. In addition, I don't think chaining system class loader reflection after
>> system resource retrieval is really meaningful: is there any case where
>> reflection always works while the system resource retrieval always fails?
>
> Chen Liang has updated the pull request with a new target base due to a merge
> or a rebase. The incremental webrev excludes the unrelated changes brought in
> by the merge/rebase. The pull request contains 10 additional commits since
> the last revision:
>
> - rename to ofClassLoading/ofResourceParsing
>convert the default class provider to bypass security manager restrictions
> - Merge branch 'master' into hierarchy-resolve
> - Merge branch 'master' into hierarchy-resolve
> - Test both cached and uncached resolvers
> - Update the class hierarchy resolver api as per mailing list last week
> - Merge branch 'master' into hierarchy-resolve
> - Update
> src/java.base/share/classes/jdk/internal/classfile/impl/ClassHierarchyImpl.java
>
>Co-authored-by: Andrey Turbanov
> - Make lookup based resolver throw on illegal access eagerly
> - 8304425: ClassHierarchyResolver from Reflection
> - ClassHierarchyResolver using Reflection
src/java.base/share/classes/jdk/internal/classfile/impl/ClassHierarchyImpl.java
line 143:
> 141: var sm = System.getSecurityManager();
> 142: if (sm != null) {
> 143: return AccessController.doPrivileged(new
> PrivilegedAction<>() {
This is out of my competency, need another reviewer for use of
`PriviledgedAction` in this context.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13082#discussion_r1188773857
