[GR-39370] Fall back to older JVMCI API when HotSpotJDKReflection is not available

[zakkak]

Closes #4655

[zakkak]

Marking as draft till Mandrel CI tests complete.

[loicottet]

Thank you for this! I had started working on a fix with pretty much the same idea as what you've done. I assume that the tests are passing on your side since the PR is no longer marked as draft, and I can merge those changes as is?

[jerboaa]

This looks OK to me. Are there tests for SubstrateAnnotationExtractor that can can be run? I.e. how can we be sure to not have introduced a regression for Mandrel JDK 11?

[zakkak]

Thank you for this! I had started working on a fix with pretty much the same idea as what you've done.

Sorry for not self-assigning the issue to myself the moment I started working on it. Nice to see that my approach ended up similar to yours :)

I assume that the tests are passing on your side since the PR is no longer marked as draft, and I can merge those changes as is?

Correct, the tests are passing on our side. Note however that I am not sure whether these changes result in getRoot always returning the same AnnotatedElement for a given parameter, but I am confident enough that it does so. In any case even if it doesn't AFAIU it would only slightly affect performance but not correctness.

Are there tests for SubstrateAnnotationExtractor that can can be run?

AFAIK no, there are no such tests. @loicottet might know better though.

I.e. how can we be sure to not have introduced a regression for Mandrel JDK 11?

Other than our existing tests passing I think we don't have any other way to be sure at the moment.

This PR could potentially introduce two kinds of regressions:

1. Returning different annotations when using OpenJDK 11 versus when using LabsJDK 11
2. Initializing referenced classes/interfaces essentially reverting the changes of [GR-38011] Parse annotations without intializing them. #4614

For 2 I feel pretty confident that it is not happening since we are still not using getAnnotations directly and we go through the GuardedAnnotationAccess.

To be sure we need to craft new tests for SubstrateAnnotationExtractor and GuardedAnnotationAccess, I don't feel familiar enough with this code to think of enough complex cases to test though. @loicottet do you think you could help on that side? e.g. by providing a high-level description of interesting cases we could test.

[loicottet]

Correct, the tests are passing on our side. Note however that I am not sure whether these changes result in getRoot always returning the same AnnotatedElement for a given parameter, but I am confident enough that it does so. In any case even if it doesn't AFAIU it would only slightly affect performance but not correctness.

The returned AnnotatedElement will be the same since it can only return the underlying Class, Method, Constructor or Field, which is unique (in all cases except Class, the objects are not actually unique, however they are all pairwise equal and contain the same annotation data, so they effectively look like the same object to the annotation parsing code).

This PR could potentially introduce two kinds of regressions:

1. Returning different annotations when using OpenJDK 11 versus when using LabsJDK 11
2. Initializing referenced classes/interfaces essentially reverting the changes of [GR-38011] Parse annotations without intializing them. #4614

I am pretty confident that none of those could happen, the first one because of what I wrote above, and the second one since the annotations are only initialised on a call to getAnnotations or a similar method, which indeed doesn't happen in that case.

To be sure we need to craft new tests for SubstrateAnnotationExtractor and GuardedAnnotationAccess, I don't feel familiar enough with this code to think of enough complex cases to test though. @loicottet do you think you could help on that side? e.g. by providing a high-level description of interesting cases we could test.

I think simply testing whether an annotation class that is marked for run-time initialisation is indeed not initialised at build-time would be sufficient, since all image classes have their annotations parsed in the image class loader during the Native Image builder startup.

[jerboaa]

Pushed with 3fccbac. I think this can be closed.

[zakkak]

Hi @loicottet , can you please confirm that this is also getting backported to the 22.2 release branch? Thanks

[zakkak]

Backported with ea42fa7. Thank you @loicottet !