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-37582] Default to running image-builder on module path. #4468

Merged
merged 41 commits into from
Jun 9, 2022

Conversation

graalvmbot
Copy link
Collaborator

No description provided.

@graalvmbot graalvmbot force-pushed the github/paw/GR-37582 branch 4 times, most recently from 6fd6d0a to 139280e Compare April 6, 2022 16:08
@graalvmbot graalvmbot force-pushed the github/paw/GR-37582 branch 3 times, most recently from fdd8459 to 5ca7721 Compare May 9, 2022 09:16
@DanHeidinga
Copy link

Hi, is there a description (or issue?) that goes along with this PR? I'm interested in the rationale behind moving the native image builder to the module path and what the follow-on repercussions of the move are.

  • Will this affect the ability to apply Substitutions from outside a module?
  • Is this the first stage in wider adoption of modules?

Any additional context for this would be appreciated.

@olpaw
Copy link
Member

olpaw commented May 17, 2022

Hi @DanHeidinga

Hi, is there a description (or issue?) that goes along with this PR? I'm interested in the rationale behind moving the native image builder to the module path and what the follow-on repercussions of the move are.

For native-image to support building Java modules into images we run the image builder on module path. So far we only do that when native-image gets a -p <module path> / --module-path <module path> argument passed. Running the builder either on classpath or module-path complicates things.That's why we want to get rid of the mode where we run the builder on classpath and instead always run it on module-path even if the image we are building is only specifying its classes on the classpath.

* Will this affect the ability to apply Substitutions from outside a module?

Nope. You can still do that. But if you use internal APIs you will need to add options to open org.graalvm.nativeimage.builder-packages that contain those internal APIs to your image build arguments. This PR has many places that show exactly how that is done. See e.g. a8b4bf2

The best strategy is to open up package org.graalvm.nativeimage.base/com.oracle.svm.util to get access to com.oracle.svm.util.ModuleSupport#accessModuleByClass and com.oracle.svm.util.ModuleSupport#accessPackagesToClass. This allows you to further open up what you need in your org.graalvm.nativeimage.hosted.Feature#afterRegistration implementation.

* Is this the first stage in wider adoption of modules?

That's already the second stage 😉 For a while now all the images of a GraalVM release that are provided by the substratevm suite are entirely build on module-path. (Search for use_modules='image' in https://github.com/oracle/graal/blob/master/substratevm/mx.substratevm/mx_substratevm.py)

In the future we hope to be able to build all images that are part of GraalVM on the module-path. For that to happen we have to make truffle also load truffle languages as Java modules (and not via URLClassloader, as it is now).

Once this is achieved we can remove all the code from the builder that is specific to running the builder on classpath.

BTW, I talked more about this a while ago in the Native Image Committer Community Meeting #4385

Feel free to attend those meetings in the future.

@DanHeidinga
Copy link

Hi @DanHeidinga

Hi, is there a description (or issue?) that goes along with this PR? I'm interested in the rationale behind moving the native image builder to the module path and what the follow-on repercussions of the move are.

For native-image to support building Java modules into images we run the image builder on module path. So far we only do that when native-image gets a -p <module path> / --module-path <module path> argument passed. Running the builder either on classpath or module-path complicates things.That's why we want to get rid of the mode where we run the builder on classpath and instead always run it on module-path even if the image we are building is only specifying its classes on the classpath.

That makes a lot of sense - a single way to do things is easier to support than two ways.

* Will this affect the ability to apply Substitutions from outside a module?

Nope. You can still do that. But if you use internal APIs you will need to add options to open org.graalvm.nativeimage.builder-packages that contain those internal APIs to your image build arguments. This PR has many places that show exactly how that is done. See e.g. a8b4bf2

The best strategy is to open up package org.graalvm.nativeimage.base/com.oracle.svm.util to get access to com.oracle.svm.util.ModuleSupport#accessModuleByClass and com.oracle.svm.util.ModuleSupport#accessPackagesToClass. This allows you to further open up what you need in your org.graalvm.nativeimage.hosted.Feature#afterRegistration implementation.

So we can still Substitute any class, we just need to open the required Graal modules to get access to APIs to change the runtime (not buildtime) view of module accessibility. Or does using com.oracle.svm.util.ModuleSupport change both the build and runtime view of accessibility?

BTW, I talked more about this a while ago in the Native Image Committer Community Meeting #4385

Thanks for sharing the link. There's some interesting context in there, especially related to how the BootModule.layer() needs to be rebuilt between the build & run time.

Feel free to attend those meetings in the future.

Will do.

@olpaw
Copy link
Member

olpaw commented May 17, 2022

. Or does using com.oracle.svm.util.ModuleSupport change both the build and runtime view of accessibility?

Module accessibility is carried over from hosted to runtime world. Thus if you change the state of the module system of modules that are seen by the reachability-analysis then you also affected the state of the module system that is part of the image that you are building.

@graalvmbot graalvmbot force-pushed the github/paw/GR-37582 branch 2 times, most recently from bcef040 to fc95742 Compare June 2, 2022 07:58
@galderz
Copy link
Contributor

galderz commented Jun 2, 2022

@zakkak has been doing some testing and found an error on this run:

  Fails with
  java.lang.IllegalAccessError: class io.quarkus.runner.AutoFeature (in unnamed module @0x26f28df5) cannot access class com.oracle.svm.core.jdk.proxy.DynamicProxyRegistry (in module org.graalvm.nativeimage.builder) because module org.graalvm.nativeimage.builder does not export com.oracle.svm.core.jdk.proxy to unnamed module @0x26f28df5

@olpaw
Copy link
Member

olpaw commented Jun 2, 2022

@zakkak has been doing some testing and found an error on this run:

  Fails with
  java.lang.IllegalAccessError: class io.quarkus.runner.AutoFeature (in unnamed module @0x26f28df5) cannot access class com.oracle.svm.core.jdk.proxy.DynamicProxyRegistry (in module org.graalvm.nativeimage.builder) because module org.graalvm.nativeimage.builder does not export com.oracle.svm.core.jdk.proxy to unnamed module @0x26f28df5

You can pass --add-exports=org.graalvm.nativeimage.builder/com.oracle.svm.core.jdk.proxy=ALL-UNNAMED to native-image to make DynamicProxyRegistry accessible for quarkus.

zakkak added a commit to zakkak/quarkus that referenced this pull request Jun 7, 2022
Once oracle/graal#4468 gets merged, native-image
will run on module-path requiring some additional exports to be passed
to it in order to access internal APIs.
@zakkak
Copy link
Collaborator

zakkak commented Jun 8, 2022

You can pass --add-exports=org.graalvm.nativeimage.builder/com.oracle.svm.core.jdk.proxy=ALL-UNNAMED to native-image to make DynamicProxyRegistry accessible for quarkus.

Hi @olpaw I did make Quarkus add all the necessary add-exports and all tests seem to pass except for an AWT test which builds but fails at runtime with:

2022-06-08 09:10:36,506 ERROR [io.qua.run.Application] (main) Failed to start application (with profile prod): java.io.IOException: Problem reading font data.
	at java.awt.Font.createFont0(Font.java:1208)
	at java.awt.Font.createFont(Font.java:1076)
	at io.quarkus.awt.it.Application.init(Application.java:62)
	at io.quarkus.awt.it.Application_Bean.create(Unknown Source)
	at io.quarkus.awt.it.Application_Bean.create(Unknown Source)
	at io.quarkus.arc.impl.AbstractSharedContext.createInstanceHandle(AbstractSharedContext.java:111)
	at io.quarkus.arc.impl.AbstractSharedContext$1.get(AbstractSharedContext.java:35)
	at io.quarkus.arc.impl.AbstractSharedContext$1.get(AbstractSharedContext.java:32)
	...

but I fail to see how the module path change could be affecting that part. Do you have any clue that could possibly help me figure out what's wrong?

Note that java.awt.Font is registered for runtime initialization and that the same test works fine when setting USE_NATIVE_IMAGE_JAVA_PLATFORM_MODULE_SYSTEM=false.

@graalvmbot graalvmbot force-pushed the github/paw/GR-37582 branch 2 times, most recently from f74c8be to bf7efe5 Compare June 8, 2022 13:57
@olpaw
Copy link
Member

olpaw commented Jun 8, 2022

Note that java.awt.Font is registered for runtime initialization and that the same test works fine when setting USE_NATIVE_IMAGE_JAVA_PLATFORM_MODULE_SYSTEM=false.

@zakkak please provide me with a simple way to reproduce this and I will debug it to figure out what is missing.

@zakkak
Copy link
Collaborator

zakkak commented Jun 8, 2022

@zakkak please provide me with a simple way to reproduce this and I will debug it to figure out what is missing.

Thank you @olpaw.

Not a "minimal" reproducer, but the following commands build and run the test that is failing.
If that doesn't do it I will try to reproduce the issue outside Quarkus as well, just let me know.

export GRAALVM_HOME=path/to/graalvm/running/on/module-path
git clone --branch maodule-path-patch https://github.com/zakkak/quarkus
cd quarkus
./mvnw -Dquickly
./mvnw -Dnative -pl integration-tests/awt -Dquarkus.native.container-build=false -Dnative.surefire.skip -Dformat.skip -Dno-descriptor-tests clean verify 

To pass additional options to native-image please set quarkus.native.additional-build-args, e.g., -Dquarkus.native.additional-build-args=--diagnostics-mode

export USE_NATIVE_IMAGE_JAVA_PLATFORM_MODULE_SYSTEM=false 
./mvnw -Dnative -pl integration-tests/awt -Dquarkus.native.container-build=false -Dnative.surefire.skip -Dformat.skip -Dno-descriptor-tests clean verify 

@graalvmbot graalvmbot merged commit eac2590 into master Jun 9, 2022
@graalvmbot graalvmbot deleted the github/paw/GR-37582 branch June 9, 2022 09:59
@olpaw
Copy link
Member

olpaw commented Jun 9, 2022

Hi @zakkak!

Re:

2022-06-08 09:10:36,506 ERROR [io.qua.run.Application] (main) Failed to start application (with profile prod): java.io.IOException: Problem reading font data.
	at java.awt.Font.createFont0(Font.java:1208)
	at java.awt.Font.createFont(Font.java:1076)
	at io.quarkus.awt.it.Application.init(Application.java:62)
	at io.quarkus.awt.it.Application_Bean.create(Unknown Source)
	at io.quarkus.awt.it.Application_Bean.create(Unknown Source)
	at io.quarkus.arc.impl.AbstractSharedContext.createInstanceHandle(AbstractSharedContext.java:111)
	at io.quarkus.arc.impl.AbstractSharedContext$1.get(AbstractSharedContext.java:35)
	at io.quarkus.arc.impl.AbstractSharedContext$1.get(AbstractSharedContext.java:32)
	...

I debugged a bit deeper into this and this is where we end up at image runtime:

0  sun.font.SunFontManager::initStatic(void) () at sun/font/SunFontManager.java:273
#1  0x0000000001554078 in sun.font.SunFontManager::<clinit>(void) () at sun/font/SunFontManager.java:268
#2  0x000000000046cdbd in com.oracle.svm.core.classinitialization.ClassInitializationInfo::invokeClassInitializer(java.lang.Class *) (this=0x7ffff6a1d7f8, hub=0x7ffff60fba38)
    at com/oracle/svm/core/classinitialization/ClassInitializationInfo.java:361
#3  0x000000000046c2b7 in com.oracle.svm.core.classinitialization.ClassInitializationInfo::initialize(com.oracle.svm.core.classinitialization.ClassInitializationInfo *, java.lang.Class *) (
    info=0x7ffff6a1d7f8, hub=0x7ffff60fba38) at com/oracle/svm/core/classinitialization/ClassInitializationInfo.java:277
#4  0x0000000000cf8503 in java.lang.Class::ensureInitialized(void) (this=0x7ffff60fba38) at com/oracle/svm/core/hub/DynamicHub.java:525
#5  0x000000000046c4ed in com.oracle.svm.core.classinitialization.ClassInitializationInfo::initialize(com.oracle.svm.core.classinitialization.ClassInitializationInfo *, java.lang.Class *) (
    info=0x7ffff6a3c2f8, hub=0x7ffff60fbee8) at com/oracle/svm/core/classinitialization/ClassInitializationInfo.java:242
#6  0x0000000000cf8503 in java.lang.Class::ensureInitialized(void) (this=0x7ffff60fbee8) at com/oracle/svm/core/hub/DynamicHub.java:525
#7  0x000000000046c4ed in com.oracle.svm.core.classinitialization.ClassInitializationInfo::initialize(com.oracle.svm.core.classinitialization.ClassInitializationInfo *, java.lang.Class *) (
    info=0x7ffff6a48278, hub=0x7ffff652e298) at com/oracle/svm/core/classinitialization/ClassInitializationInfo.java:242
#8  0x0000000000cf8503 in java.lang.Class::ensureInitialized(void) (this=0x7ffff652e298) at com/oracle/svm/core/hub/DynamicHub.java:525
#9  0x0000000000cf8c3c in java.lang.Class::forName(java.lang.String *, boolean, java.lang.ClassLoader *) (name=0x7fffede3c080, initialize=true, loader=0x7ffff6889a18)
    at com/oracle/svm/core/hub/DynamicHub.java:1139
#10 0x000000000151f6f1 in sun.font.FontManagerFactory$1::run(void) (this=0x7fffedc16628) at sun/font/FontManagerFactory.java:83
#11 0x0000000000e898ee in java.security.AccessController::executePrivileged(java.security.PrivilegedAction *, java.security.AccessControlContext *, java.lang.Class *) (action=0x7fffedc16628, 
    context=0x7ffff5c00000, caller=0x7ffff60eb930) at com/oracle/svm/core/jdk/SecuritySubstitutions.java:169
#12 0x0000000000e893d9 in java.security.AccessController::doPrivileged(java.security.PrivilegedAction *) (action=0x7fffedc16628) at java/security/AccessController.java:318
#13 0x000000000151fe33 in sun.font.FontManagerFactory::getInstance(void) () at sun/font/FontManagerFactory.java:75
#14 0x0000000000bdf17d in java.awt.Font::createFont0(int, java.io.InputStream *, boolean, sun.font.CreatedFontTracker *) (fontFormat=0, fontStream=0x7fffedc0f510, allFonts=false, tracker=0x7ffff5c00000)
    at java/awt/Font.java:1170
#15 0x0000000000bde96b in java.awt.Font::createFont(int, java.io.InputStream *) (fontFormat=0, fontStream=0x7fffedc0f510) at java/awt/Font.java:1076
#16 0x00000000009c14a6 in io.quarkus.awt.it.Application::init(void) (this=0x7fffede0c7b0) at io/quarkus/awt/it/Application.java:62

Then in sun.font.SunFontManager::initStatic things go wrong with

    private static void initStatic() {
        AccessController.doPrivileged(new PrivilegedAction<Void>() {
            public Void run() {
                FontManagerNativeLibrary.load();

                // JNI throws an exception if a class/method/field is not found,
                // so there's no need to do anything explicit here.
                initIDs();

                switch (StrikeCache.nativeAddressSize) {
                case 8: longAddresses = true; break;
                case 4: longAddresses = false; break;
                default: throw new RuntimeException("Unexpected address size"); <=== BANG
                }

It looks like StrikeCache.nativeAddressSize doesn't have the right size and we end up with a RuntimeException during running of <clinit> of sun.font.SunFontManager which happens as part of <clinit> for sun.awt.X11FontManager which is needed for the reflective instantiation of FontManager in sun.font.FontManagerFactory::getInstance at image runtime.

That's all for now. I will further look into this next week.

@olpaw
Copy link
Member

olpaw commented Jun 13, 2022

sun.font.SunFontManager::initIDs leads to native code
in https://github.com/openjdk/jdk/blob/8f400b9aab57d0639721add2ba511bfc0459bd89/src/java.desktop/share/native/libfontmanager/sunFont.c#L89
where sun/font/TrueTypeFont is accessed via JNI. Unfortunately it's not registered for JNI access thus leading to the described behavior above.

@olpaw
Copy link
Member

olpaw commented Jun 13, 2022

When building with USE_NATIVE_IMAGE_JAVA_PLATFORM_MODULE_SYSTEM=false I can observe that we do pass sun.font.SunFontManager::initIDs at image runtime just fine. So it appears when we run with image-builder on module-path something prevents e.g. sun/font/TrueTypeFont to get registered for JNI access. @zakkak does this ring a bell? Could it be that io.quarkus.runner.AutoFeature tries to register classes for JNI access but fails silently for some reason? Does quarkus have options to make it chatty in regard to what it tries to register for JNI?

@zakkak
Copy link
Collaborator

zakkak commented Jun 14, 2022

So it appears when we run with image-builder on module-path something prevents e.g. sun/font/TrueTypeFont to get registered for JNI access. @zakkak does this ring a bell? Could it be that io.quarkus.runner.AutoFeature tries to register classes for JNI access but fails silently for some reason?

I think that's exactly what's happening.

The generated AutoFeature registers TrueType as follows:

// Source code recreated from a .class file by IntelliJ IDEA
// (powered by FernFlower decompiler)

    private static void registerJniAccessibleClass70() {
        try {
            ClassLoader var0 = Thread.currentThread().getContextClassLoader();
            Class var1 = Class.forName("sun.font.TrueTypeFont", (boolean)0, var0);
            Constructor[] var3 = var1.getDeclaredConstructors();
            Method[] var4 = var1.getDeclaredMethods();
            Field[] var5 = var1.getDeclaredFields();
            Class[] var2 = new Class[]{var1};
            JNIRuntimeAccess.register(var2);
            JNIRuntimeAccess.register((Executable[])var3);
            JNIRuntimeAccess.register((Executable[])var4);
            JNIRuntimeAccess.register((boolean)0, var5);
        } catch (Throwable var6) {
        }
    }

However looking at the GraalVM code I see that when registering a constructor or a method it first looks it up using the ReflectionUtil class which when performing the lookup "ensures that this class is allowed to call setAccessible for an element of the provided declaring class".

/**
* Ensure that this class is allowed to call setAccessible for an element of the provided
* declaring class.
*/
private static void openModule(Class<?> declaringClass) {
ModuleSupport.accessModuleByClass(ModuleSupport.Access.OPEN, ReflectionUtil.class, declaringClass);
}

Which apparently Quarkus doesn't do, which might be what's causing the issue. I will try a quick patch tomorrow if including that part makes it work. Thanks a lot @olpaw !

Does quarkus have options to make it chatty in regard to what it tries to register for JNI?

Unfortunately not.

@olpaw
Copy link
Member

olpaw commented Jun 15, 2022

@zakkak thanks for confirming. Btw ...

    private static void registerJniAccessibleClass70() {
        try {
           ...
        } catch (Throwable var6) {
        }
    }

Swallowing this Throwable is dangerous. How would you ever know which registrations succeeded and which did not?

You will save yourself a lot of trouble if you fix this 😉

@zakkak
Copy link
Collaborator

zakkak commented Jun 15, 2022

You will save yourself a lot of trouble if you fix this wink

Indeed, after fixing this it became clear what the issue was and the fix was as simple as adding -J--add-exports=org.graalvm.nativeimage.builder/com.oracle.svm.core.jni=ALL-UNNAMED

Thanks again @olpaw that really saved me a lot of time :)

zakkak added a commit to zakkak/quarkus that referenced this pull request Jun 15, 2022
Once oracle/graal#4468 gets merged, native-image
will run on module-path requiring some additional exports to be passed
to it in order to access internal APIs.
melix added a commit to micronaut-projects/micronaut-gradle-plugin that referenced this pull request Jul 20, 2022
This commit adds a workaround for GraalVM 22.2 native-image builder running
on module path. Because Micronaut uses features which use internal APIs (and
no workaround for that), building on 22.2 fails. This commit adds a flag
which will let users build by adding a module export.

See oracle/graal#4468
melix added a commit to micronaut-projects/micronaut-gradle-plugin that referenced this pull request Jul 20, 2022
* Fix support for GraalVM 22.2

This commit adds a workaround for GraalVM 22.2 native-image builder running
on module path. Because Micronaut uses features which use internal APIs (and
no workaround for that), building on 22.2 fails. This commit adds a flag
which will let users build by adding a module export.

See oracle/graal#4468

* Test against latest GraalVM
beatngu13 added a commit to beatngu13/pdf-zoom-wizard that referenced this pull request Sep 10, 2022
* `--allow-incomplete-classpath` is now default.
* `VersionEnum` is now loaded during build time.
* Native image now runs on the module path, obviously missing an export
  (see also: oracle/graal#4468)
beatngu13 added a commit to beatngu13/pdf-zoom-wizard that referenced this pull request Sep 10, 2022
* `--allow-incomplete-classpath` is now default.
* `VersionEnum` is now loaded during build time.
* Native image now runs on the module path, obviously missing an export
  (see also: oracle/graal#4468)
beatngu13 added a commit to beatngu13/pdf-zoom-wizard that referenced this pull request Sep 27, 2023
* `--allow-incomplete-classpath` is now default.
* `VersionEnum` is now loaded during build time.
* Native image now runs on the module path, obviously missing an export
  (see also: oracle/graal#4468)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants