Skip to content

Conversation

@ziyilin
Copy link
Collaborator

@ziyilin ziyilin commented Aug 11, 2021

The standalone pointsto project needs to accept options defined by PointstoOptions, and other options such as -H:+PringFlags. But the option's meta data and pasring code are in svm.core which should not be dependented by pointsto.
This PR extracts the common part of option definition and parsing code into a seperated project com.oracle.svm.option , so that both svm and pointsto can dependent on it.
This PR is related with featue request:#3418

@ziyilin ziyilin force-pushed the parsePointstoOptions branch from fad87ac to d1b4ad3 Compare August 11, 2021 12:25
@ziyilin ziyilin force-pushed the parsePointstoOptions branch 4 times, most recently from c4bc0eb to 3d6b21e Compare August 24, 2021 06:53
@christianwimmer christianwimmer requested review from cstancu and removed request for christianwimmer August 24, 2021 20:49
@ziyilin ziyilin force-pushed the parsePointstoOptions branch from 3d6b21e to 95118fe Compare August 25, 2021 03:15
"com.oracle.svm.core.classinitialization to jdk.internal.vm.compiler",
"com.oracle.svm.truffle.api to org.graalvm.truffle",
"* to org.graalvm.nativeimage.driver,org.graalvm.nativeimage.configure,org.graalvm.nativeimage.librarysupport,org.graalvm.nativeimage.llvm,org.graalvm.nativeimage.agent.jvmtibase,org.graalvm.nativeimage.agent.tracing,org.graalvm.nativeimage.agent.diagnostics,com.oracle.svm.svm_enterprise",
"* to org.graalvm.nativeimage.pointsto,org.graalvm.nativeimage.driver,org.graalvm.nativeimage.configure,org.graalvm.nativeimage.librarysupport,org.graalvm.nativeimage.llvm,org.graalvm.nativeimage.agent.jvmtibase,org.graalvm.nativeimage.agent.tracing,org.graalvm.nativeimage.agent.diagnostics,com.oracle.svm.svm_enterprise",
Copy link
Member

@cstancu cstancu Aug 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this export necessary? Seems strange that org.graalvm.nativeimage.builder needs to export * to org.graalvm.nativeimage.pointsto.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this exporting, there is an exception thrown out at libnative-image-diagnostics-agent.so.image build time:

Building libnative-image-agent.so.image... [dependency SVM_CONFIGURE updated]
Building libnative-image-diagnostics-agent.so.image... [dependency JVMTI_AGENT_BASE updated]
Exception in thread "main" com.oracle.svm.core.util.VMError$HostedError: org.graalvm.nativeimage.base.option.OptionParsingException: java.lang.IllegalAccessException: class org.graalvm.nativeimage.base.option.CommonOptionParser (in module org.graalvm.nativeimage.pointsto) cannot access class com.oracle.svm.hosted.SVMHost_OptionDescriptors (in module org.graalvm.nativeimage.builder) because module org.graalvm.nativeimage.builder does not export com.oracle.svm.hosted to module org.graalvm.nativeimage.pointsto
        at org.graalvm.nativeimage.builder/com.oracle.svm.core.util.VMError.shouldNotReachHere(VMError.java:72)
        at org.graalvm.nativeimage.builder/com.oracle.svm.core.option.SubstrateOptionsParser.collectOptions(SubstrateOptionsParser.java:125)
        at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.option.HostedOptionParser.collectOptions(HostedOptionParser.java:63)
        at org.graalvm.nativeimage.driver/com.oracle.svm.driver.APIOptionHandler.extractOptions(APIOptionHandler.java:122)
        at org.graalvm.nativeimage.driver/com.oracle.svm.driver.APIOptionHandler.<init>(APIOptionHandler.java:115)
        at org.graalvm.nativeimage.driver/com.oracle.svm.driver.NativeImage.<init>(NativeImage.java:820)
        at org.graalvm.nativeimage.driver/com.oracle.svm.driver.NativeImage.lambda$static$19(NativeImage.java:1457)
        at org.graalvm.nativeimage.driver/com.oracle.svm.driver.NativeImage.build(NativeImage.java:1490)
        at org.graalvm.nativeimage.driver/com.oracle.svm.driver.NativeImage.performBuild(NativeImage.java:1473)
        at org.graalvm.nativeimage.driver/com.oracle.svm.driver.NativeImage.main(NativeImage.java:1460)
Caused by: org.graalvm.nativeimage.base.option.OptionParsingException: java.lang.IllegalAccessException: class org.graalvm.nativeimage.base.option.CommonOptionParser (in module org.graalvm.nativeimage.pointsto) cannot access class com.oracle.svm.hosted.SVMHost_OptionDescriptors (in module org.graalvm.nativeimage.builder) because module org.graalvm.nativeimage.builder does not export com.oracle.svm.hosted to module org.graalvm.nativeimage.pointsto
        at org.graalvm.nativeimage.pointsto/org.graalvm.nativeimage.base.option.CommonOptionParser.collectOptions(CommonOptionParser.java:169)
        at org.graalvm.nativeimage.builder/com.oracle.svm.core.option.SubstrateOptionsParser.collectOptions(SubstrateOptionsParser.java:123)
        ... 8 more
Caused by: java.lang.IllegalAccessException: class org.graalvm.nativeimage.base.option.CommonOptionParser (in module org.graalvm.nativeimage.pointsto) cannot access class com.oracle.svm.hosted.SVMHost_OptionDescriptors (in module org.graalvm.nativeimage.builder) because module org.graalvm.nativeimage.builder does not export com.oracle.svm.hosted to module org.graalvm.nativeimage.pointsto
        at java.base/jdk.internal.reflect.Reflection.newIllegalAccessException(Reflection.java:361)
        at java.base/java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:591)
        at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:481)
        at org.graalvm.nativeimage.pointsto/org.graalvm.nativeimage.base.option.CommonOptionParser.collectOptions(CommonOptionParser.java:167)
        ... 9 more
Exception in thread "main" com.oracle.svm.core.util.VMError$HostedError: org.graalvm.nativeimage.base.option.OptionParsingException: java.lang.IllegalAccessException: class org.graalvm.nativeimage.base.option.CommonOptionParser (in module org.graalvm.nativeimage.pointsto) cannot access class com.oracle.svm.hosted.SVMHost_OptionDescriptors (in module org.graalvm.nativeimage.builder) because module org.graalvm.nativeimage.builder does not export com.oracle.svm.hosted to module org.graalvm.nativeimage.pointsto
        at org.graalvm.nativeimage.builder/com.oracle.svm.core.util.VMError.shouldNotReachHere(VMError.java:72)
        at org.graalvm.nativeimage.builder/com.oracle.svm.core.option.SubstrateOptionsParser.collectOptions(SubstrateOptionsParser.java:125)
        at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.option.HostedOptionParser.collectOptions(HostedOptionParser.java:63)
        at org.graalvm.nativeimage.driver/com.oracle.svm.driver.APIOptionHandler.extractOptions(APIOptionHandler.java:122)
        at org.graalvm.nativeimage.driver/com.oracle.svm.driver.APIOptionHandler.<init>(APIOptionHandler.java:115)
        at org.graalvm.nativeimage.driver/com.oracle.svm.driver.NativeImage.<init>(NativeImage.java:820)
        at org.graalvm.nativeimage.driver/com.oracle.svm.driver.NativeImage.lambda$static$19(NativeImage.java:1457)
        at org.graalvm.nativeimage.driver/com.oracle.svm.driver.NativeImage.build(NativeImage.java:1490)
        at org.graalvm.nativeimage.driver/com.oracle.svm.driver.NativeImage.performBuild(NativeImage.java:1473)
        at org.graalvm.nativeimage.driver/com.oracle.svm.driver.NativeImage.main(NativeImage.java:1460)
Caused by: org.graalvm.nativeimage.base.option.OptionParsingException: java.lang.IllegalAccessException: class org.graalvm.nativeimage.base.option.CommonOptionParser (in module org.graalvm.nativeimage.pointsto) cannot access class com.oracle.svm.hosted.SVMHost_OptionDescriptors (in module org.graalvm.nativeimage.builder) because module org.graalvm.nativeimage.builder does not export com.oracle.svm.hosted to module org.graalvm.nativeimage.pointsto
        at org.graalvm.nativeimage.pointsto/org.graalvm.nativeimage.base.option.CommonOptionParser.collectOptions(CommonOptionParser.java:169)
        at org.graalvm.nativeimage.builder/com.oracle.svm.core.option.SubstrateOptionsParser.collectOptions(SubstrateOptionsParser.java:123)
        ... 8 more
Caused by: java.lang.IllegalAccessException: class org.graalvm.nativeimage.base.option.CommonOptionParser (in module org.graalvm.nativeimage.pointsto) cannot access class com.oracle.svm.hosted.SVMHost_OptionDescriptors (in module org.graalvm.nativeimage.builder) because module org.graalvm.nativeimage.builder does not export com.oracle.svm.hosted to module org.graalvm.nativeimage.pointsto
        at java.base/jdk.internal.reflect.Reflection.newIllegalAccessException(Reflection.java:361)
        at java.base/java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:591)
        at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:481)
        at org.graalvm.nativeimage.pointsto/org.graalvm.nativeimage.base.option.CommonOptionParser.collectOptions(CommonOptionParser.java:167)
        ... 9 more

Building libnative-image-diagnostics-agent.so.image: Failed due to error: 1

Building libnative-image-agent.so.image: Failed due to error: 1
Building libnative-image-agent.so.image failed
Building libnative-image-diagnostics-agent.so.image failed
2 build tasks failed

"name" : "org.graalvm.nativeimage.pointsto",
"exports" : [
"com.oracle.svm.util",
"com.oracle.svm.option",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ziyilin It would be better for com.oracle.svm.option to be in its own distribution thus being a module (e.g. named org.graalvm.nativeimage.options) in itself. Then module org.graalvm.nativeimage.pointsto depends on org.graalvm.nativeimage.options and other modules can depend on org.graalvm.nativeimage.options and have a qualified export of their packages to org.graalvm.nativeimage.options. This way we can avoid having unrelated modules export their packages to org.graalvm.nativeimage.pointsto. Thanks @olpaw for the suggestion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. And maybe I should give a better name for com.oracle.svm.option, for example com.oracle.svm.common so that it can include other common modules, not only options. I'll make the change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. And maybe I should give a better name for com.oracle.svm.option, for example com.oracle.svm.common so that it can include other common modules, not only options. I'll make the change.

I'd go for org.graalvm.nativeimage.base (in line with e.g. java.base)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have renamed the project to org.graalvm.nativeimage.base.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ziyilin that's good, but we need to take it one step further for a cleaner design: org.graalvm.nativeimage.base has to be a separate distribution, i.e., with a declaration in the "distributions" section, thus being its own module, i.e., with a "moduleInfo" definition, like I described above. Something along the lines of:

"NATIVE_IMAGE_BASE": {
    "subDir": "src",
    "description" : "Native Image base.",
    "dependencies": [
        "org.graalvm.nativeimage.base"
    ],
    "distDependencies": [
        "compiler:GRAAL",
    ],
    "exclude": [
    ],
    "moduleInfo" : {
        "name" : "org.graalvm.nativeimage.base",
        "exports" : [
        ]
        }
    },
},

@olpaw is there any restriction against having the module use the same name as the project?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cstancu I‘ve added new module declarations as suggested.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After talking with @olpaw we decided that the project name should be com.oracle.svm.common, such that it doesn't conflict with the module name, which should remain org.graalvm.nativeimage.base. So please rename only the project name from org.graalvm.nativeimage.base to com.oracle.svm.common.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed as suggested.

@ziyilin ziyilin force-pushed the parsePointstoOptions branch 2 times, most recently from 9bdffba to 11d0dad Compare September 6, 2021 02:08
@ziyilin ziyilin force-pushed the parsePointstoOptions branch 2 times, most recently from fa493e6 to bcc4c25 Compare September 26, 2021 12:20
@ziyilin ziyilin force-pushed the parsePointstoOptions branch from bcc4c25 to 077e5ad Compare October 9, 2021 06:24
import java.util.jar.JarEntry;
import java.util.jar.JarFile;

public class ClassPathSearcher {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see where this class is used. I think it can be removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, this class is outdated since OptionDescriptors classes are found with SPI.

public static final OptionKey<String> AnalysisEntryClass = new OptionKey<>(null);

@Option(help = "Configurations used in native image building time can also be used in analysis. Specify the configurations' directory.")//
public static final OptionKey<String> AnalysisConfigDir = new OptionKey<>(null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these options can stay in your extension project for now since they are not used in the code base.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed.

@ziyilin ziyilin force-pushed the parsePointstoOptions branch from 077e5ad to b684ae2 Compare October 18, 2021 02:33
The standalone pointsto need to take the input options. The options and
parsing should follow the same way as native image's hosted options.
So we extract the commom part of SubstrateOptionsParser to a new project
where both pointsto and svm can share.
@ziyilin ziyilin force-pushed the parsePointstoOptions branch from b684ae2 to aadd1cd Compare October 18, 2021 02:45
@graalvmbot graalvmbot merged commit aadd1cd into oracle:master Oct 20, 2021
@ziyilin ziyilin deleted the parsePointstoOptions branch October 21, 2021 01:43
zakkak added a commit to zakkak/mandrel-packaging that referenced this pull request Oct 21, 2021
oracle/graal#3666 performed some refactoring
introducing a new jar file native-image-base.jar which mandrel depends
on
zakkak added a commit to graalvm/mandrel-packaging that referenced this pull request Oct 21, 2021
oracle/graal#3666 performed some refactoring
introducing a new jar file native-image-base.jar which mandrel depends
on
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.

4 participants