[External] : Re: ClassHierarchyResolver using Reflection information

Mon Mar 20 13:56:01 UTC 2023

New behavior is “no assumptions” and throw IllegalArgumentException if information about type necessary to build stack map (or verify stack map) is not obtained from the resolver (or better say chain of resolvers). If user provides no resolver – the default is used.
The way how default resolver gets the information from system classloader is subject of discussion below (resource parsing and fallback to class loading).
Custom resolver can be constructed as a fallback chain from all provided resolver types (including the default resolver instance).

On 20.03.2023 14:25, "Brian Goetz" <brian.goetz at oracle.com> wrote:

If the user provides no resolver at all, what is the new behavior?  Do we just assume Object is the common supertype for any pair of classes?
On 3/20/2023 7:17 AM, Adam Sotona wrote:
FYI: I’ve filled a new bug and proposed patch: 8304502: Classfile API class hierarchy makes assumptions when class is not resolved #13099<https://github.com/openjdk/jdk/pull/13099>

Thanks,
Adam

On 20.03.2023 10:50, "Adam Sotona" <adam.sotona at oracle.com><mailto:adam.sotona at oracle.com> wrote:

I agree with your concerns. The benevolence of the CHR was decision of an early development stage, it supposed to be temporary and now it is a clear bug.
So from my perspective -  yes, fail fast and fail when any required class is not resolved.
Classfile API as well as ASM are designed to handle majority of cases without user knowledge of CHR or class loading details. I guess only few ASM users know how to pass custom class loader for correct CHR for stack maps generation. I’ll try to summarize benefits and negatives of my proposal to extend default CHR to fall back to a class loading CHR.

Benefits of CHR by classfile parsing:

1.       Performance – “skip-parsing” of constant pool + classfile header is in order of magnitude faster than class loading

2.       Simplicity/Stability – it is not necessary to provide all dependencies to identify if the type is an interface or what is its super. It is not necessary to have complete classpath with implementation of some API to generate code using the API.
Negatives of CHR by classfile parsing:

3.       Availability  - a classfile may not be available or accessible as a resource for parsing

4.       Different results – a classfile may be modified when loaded, however the modification would have to change its super class or its status of being an interface to make any impact.

By adding a fallback to class loading CHR we will eliminate the “Availability“ negative, with no performance loose.
I think that the default should be rock-solid, because 99% of users will use the default.

Thanks,
Adam

BTW: Maybe even more beneficial would be an ability to question already loaded classes first (without loading them), parse them as resource if not already loaded and fallback to load them if not available as resource.

On 18.03.2023 5:06, "Michael van Acken" <michael.van.acken at gmail.com><mailto:michael.van.acken at gmail.com> wrote:

Am Fr., 17. März 2023 um 19:34 Uhr schrieb Brian Goetz <brian.goetz at oracle.com<mailto:brian.goetz at oracle.com>>:
I'm not so comfortable adding this to DEFAULT_CHA.  In addition to
adding another degree of environmental dependence, and the issues of
additional exceptions you raise, I suspect it may also have a
performance cost.  I would like for programmers to opt into which CHA
they use, and the default should be the most minimal.  This means having
a menu of CHAs to choose from, rather than guessing what the user wants.

The default classHierarchyResolver has an opaque (to me) error
mode that tripped me up.  Similar to my experience with ASM some
years ago, it works fine for a lot (most?) of code, but then some
input causes it to silently produce a stack map the verifier does
not agree with.  For me, this happens usually quite late in the
game and at first is completely baffling.  Back in July it was
mostly luck and half buried memories that sent me into the
direction of CHR as the solution to the verifier error.  If the
default CHR had raised an exception at the point where
information was missing, the path to the fix (preloading the CHR
with information about future classes) would have been clearer.

--mva

On 3/17/2023 2:25 PM, - wrote:
> I've submitted a patch at https://github.com/openjdk/jdk/pull/13082<https://urldefense.com/v3/__https:/github.com/openjdk/jdk/pull/13082__;!!ACWV5N9M2RV99hQ!IckIM3pDx0kj8-sYmKqh2mYHCcMDA3jEXIwOwzMAsirh8YKg1N949owV5wzMUHjA8_uF8Seb8GjSAMGhFN0zL4nTfIE$>
> Yet with it comes 3 questions:
>
> 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.
>
> The main concern for me to fail fast is that I don't understand how
> the Classfile API propagates resolver exceptions. If wrapping the
> IllegalAccessException in an IllegalAccessError is safe, then I may
> change it and add a test case with HashMap.
>
> 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 be redefined with
> instrumentation or jfr, which may not be reflected in the system
> resources.
>
> This idea came to me while I was working on jfr and instrumentation
> tests. Luckily, it seems they don't touch class hierarchy that the
> default resolver suffices.
>
> 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
>
> On Fri, Mar 17, 2023 at 7:11 AM Adam Sotona <adam.sotona at oracle.com<mailto:adam.sotona at oracle.com>> wrote:
>> I like your proposal and I would like to see it as a fallback solution for DEFAULT_CLASS_HIERARCHY_RESOLVER when the class is not available as resource stream.
>>
>>
>>
>>     /**
>>
>>       * Default singleton instance of {@linkplain ClassHierarchyResolver}
>>
>>       * using {@link ClassLoader#getSystemResourceAsStream(String)}
>>
>>       * as the {@code ClassStreamResolver} with fallback to
>>
>>       * {@link ClassLoader.getSystemClassLoader()} class loading resolver.
>>
>>       */
>>
>>      ClassHierarchyResolver DEFAULT_CLASS_HIERARCHY_RESOLVER
>>
>>              = new ClassHierarchyImpl.CachedClassHierarchyResolver(
>>
>>              new Function<ClassDesc, InputStream>() {
>>
>>                  @Override
>>
>>                  public InputStream apply(ClassDesc classDesc) {
>>
>>                      return ClassLoader.getSystemResourceAsStream(Util.toInternalName(classDesc) + ".class");
>>
>>                  }
>>
>>              }).orElse(ClassHierarchyResolver.of(ClassLoader.getSystemClassLoader()));
>>
>>
>>
>> Thanks,
>>
>> Adam
>>
>>
>>
>> On 16.03.2023 17:05, "liangchenblue at gmail.com<mailto:liangchenblue at gmail.com>" <liangchenblue at gmail.com<mailto:liangchenblue at gmail.com>> wrote:
>>
>>
>>
>> For context, in 8294961
>> https://github.com/openjdk/jdk/pull/10991/files#r1133086265<https://urldefense.com/v3/__https:/github.com/openjdk/jdk/pull/10991/files*r1133086265__;Iw!!ACWV5N9M2RV99hQ!IckIM3pDx0kj8-sYmKqh2mYHCcMDA3jEXIwOwzMAsirh8YKg1N949owV5wzMUHjA8_uF8Seb8GjSAMGhFN0zkG4CkgE$> I wondered
>> about whether to use a hierarchy resolver for LambdaMetafactory, that
>> I think resolution may encounter problems as the default resolver may
>> not be able to resolve user-supplied interfaces.
>>
>> In addition, many of the class file generation usages I've seen
>> include firing events via an event bus, calling patched external addon
>> code (as seen in Minecraft modding), in which Reflection-based
>> hierarchy resolution would work better than reading bytecode files.
>>
>> Thus, I've prepared a patch creating ClassHierarchyResolver
>> https://github.com/openjdk/jdk/commit/0c888ba1e2953cf946012244446f4f8c05ba5d77<https://urldefense.com/v3/__https:/github.com/openjdk/jdk/commit/0c888ba1e2953cf946012244446f4f8c05ba5d77__;!!ACWV5N9M2RV99hQ!IckIM3pDx0kj8-sYmKqh2mYHCcMDA3jEXIwOwzMAsirh8YKg1N949owV5wzMUHjA8_uF8Seb8GjSAMGhFN0zl_RRNVo$>
>> to ease up resolution with reflection, when a ClassLoader (for older
>> APIs) or a MethodHandle.Lookup (for modern APIs) is available.
>>
>> Does this appear feasible?
>>
>> Chen

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