Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GR-43837] Enable --report-unsupported-elements-at-runtime by default and deprecate the option. #8815

Closed

Conversation

graalvmbot
Copy link
Collaborator

Libraries can freely remove --report-unsupported-elements-at-runtime from their native-image.properties files. If these libraries are used with previous GraalVM versions --report-unsupported-elements-at-runtime should be added to the command line (ideally by the framework).

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Apr 22, 2024
@zakkak
Copy link
Collaborator

zakkak commented Apr 23, 2024

Hi @vjovanov, does --link-at-build-time suffice to keep the old behavior? Perhaps it would be worth mentioning how to bring back the old behavior in the changelog and the deprecation message.

@vjovanov
Copy link
Member

@zakkak the previous behavior can be kept with -H:-ReportUnsupportedElementsAtRuntime but I would not recommend that for end users. This mode should not ever affect the end users as they are free to write any type of code. It should only be used for internal testing to guarantee that certain elements obviated by substitutions can never be reached (without using reflection).

I would not connect this to link-at-build-time as link-at-build-time is something that can be fixed by end-users by providing regular jars. Here, if a library reaches some deleted element it can hardly be fixed by the user.

@zakkak
Copy link
Collaborator

zakkak commented Apr 23, 2024

@zakkak the previous behavior can be kept with -H:-ReportUnsupportedElementsAtRuntime

Oh, it's just the API option being deprecated not the internal one. I see.

but I would not recommend that for end users. This mode should not ever affect the end users as they are free to write any type of code.
[...snip...]
Here, if a library reaches some deleted element it can hardly be fixed by the user.

If I get this right, you mean that user code should never result in an unsupported element exception. In that case what's the difference for them either way? What's the benefit of changing this default?

Furthermore, wouldn't it be preferable for the build to fail and prompt the user to report the issue instead of letting it build and result in a potential unexpected crash at runtime?

@vjovanov
Copy link
Member

Well, the user code that does not use reflection should not be able to reach elements marked as @Delete. When using reflection users can reach anything, and in these cases we need to fail at run-time. The reached elements will anyhow fail at run-time with a fatal error.

So this is for users that reach @Delete elements by accident via reflection so their build doesn't fail. For the authors that use reflection they should just use the -H:-ReportUnsupportedElementsAtRuntime flag in their tests.

@vjovanov
Copy link
Member

This flag is also historic from the time when Native Image did not support many things. Now, Native Image does not support only run-time class definition and a few other details. In all those cases we will throw an UnsupportedOperationException so no need for build-time failures.

@zakkak
Copy link
Collaborator

zakkak commented Apr 23, 2024

Thanks for the info @vjovanov.

FWIW doing a quick search in the Quarkus repository I see multiple uses of @Delete, which makes me believe that it might not be that difficult for Quarkus users to reach an element that is being marked as @Delete by Quarkus. I will bring this to the attention of the Quarkus community.

Update: FTR the email requesting feedback from the Quarkus community is https://groups.google.com/g/quarkus-dev/c/v5jLzl6cwuE

@zakkak
Copy link
Collaborator

zakkak commented Apr 25, 2024

As pointed out by @vjovanov this PR results in a number of tests in the Quarkus CI job to fail (see https://github.com/oracle/graal/actions/runs/8819845538).

Looking into the failures I see that they are triggered by the deletion of classes that depend on other classes not present on the classpath.

For example DeleteFullTextLucene.java registers
FullTextLucene for deletion which depends on IndexFormatTooOldException, used here.

So my understanding is that GraalVM tries to load the FullTextLucene class in order to go over its fields, constructors, and methods) but fails because it's not on the classpath...

I recall raising an issue about @Delete and @Substitute requiring all dependencies to be present but can't seem to find it know...

A potential work around in Quarkus would be to require all dependencies of @Deleted and @Substituted classes to be present at build-time, even though the app doesn't actually use them... The main drawback of this is that it might hide cases were the code we want to make unreachable is still reachable and due to the class being on the classpath we don't notice it. Note that Quarkus build time initializes everything by default and uses --link-at-build-time.

I wonder if there is any way to have GraalVM achieve its goal without having to load the class, e.g. by having a "trap" that catches any call or field access to the @Deleted class.

@graalvmbot graalvmbot force-pushed the vj/GR-43837-deprecate-report-unsupported-elements branch 4 times, most recently from 2ec1127 to 9340c06 Compare April 30, 2024 15:24
@vjovanov
Copy link
Member

vjovanov commented Apr 30, 2024

I used JVMCI to get the class information and we seem to be good:

https://github.com/oracle/graal/actions/runs/8896343413

Thanks @zakkak!

@graalvmbot graalvmbot force-pushed the vj/GR-43837-deprecate-report-unsupported-elements branch 3 times, most recently from 9c9a937 to 3499578 Compare May 3, 2024 09:40
@graalvmbot graalvmbot force-pushed the vj/GR-43837-deprecate-report-unsupported-elements branch from 3499578 to 83c7648 Compare May 13, 2024 06:27
@zakkak
Copy link
Collaborator

zakkak commented Sep 13, 2024

I recall raising an issue about @delete and @substitute requiring all dependencies to be present but can't seem to find it now...

FTR that was #1725

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants