Report warning for unsupported system properties #2834

ziyilin · 2020-09-08T13:45:33Z

Overview

The Java system properties are not consistent between traditional JDK version and native image version. Some system properties are missing and some are different. User may expect the same behavior between traditional and native image, or at least explicit message to warn them about the change. This patch checks the usage of such system properties and reports warning at both native image build time and run time. This patch can fix issue #2835.

Features

Check the usage of the missing system properties at native image build time and report warning messages as follows before writing image.
Report using missing system property exception at run time

System Property Differences

Here lists the detailed differences between traditional and native image version on JDK11.

Property Name	Traditional JDK value	Native Image value
awt.toolkit	sun.awt.X11.XToolkit	N/A
file.encoding	UTF-8	UTF-8
file.separator	/	/
java.awt.graphicsenv	sun.awt.X11GraphicsEnvironment	N/A
java.awt.printerjob	sun.print.PSPrinterJob	N/A
java.class.path	bin	EMPTY
java.class.version	55	55
java.home	/home/cengfeng.lzy/tools/labsjdk-ce-11.0.8-jvmci-20.2-b03	N/A
java.io.tmpdir	/tmp	/tmp
java.library.path	/usr/java/packages/lib:/usr/lib64:/lib64:/lib:/usr/lib	EMPTY
java.runtime.name	OpenJDK Runtime Environment	N/A
java.runtime.version	11.0.8+10-jvmci-20.2-b03	N/A
java.specification.name	Java Platform API Specification	Java Platform API Specification
java.specification.vendor	Oracle Corporation	Oracle Corporation
java.specification.version	11	11
java.vendor	GraalVM Community	Oracle Corporation
java.vendor.url	https://www.graalvm.org/	https://www.graalvm.org/
java.vendor.url.bug	https://github.com/oracle/graal/issues	N/A
java.version	11.0.8	11.0.8
java.version.date	2020/7/14	N/A
java.vm.compressedOopsMode	Zero based	N/A
java.vm.info	mixed mode	N/A
java.vm.name	OpenJDK 64-Bit Server VM	Substrate VM
java.vm.specification.name	Java Virtual Machine Specification	Java Virtual Machine Specification
java.vm.specification.vendor	Oracle Corporation	Oracle Corporation
java.vm.specification.version	11	11
java.vm.vendor	GraalVM Community	Oracle Corporation
java.vm.version	11.0.8+10-jvmci-20.2-b03	GraalVM dev Java 11
jdk.debug	release	N/A
line.separator	\r\n	\r\n
os.arch	amd64	amd64
os.name	Linux	Linux
os.version	3.10.0-327.ali2012.alios7.x86_64	3.10.0-327.ali2012.alios7.x86_64
path.separator	:	:
sun.arch.data.model	64	64
sun.boot.library.path	/home/cengfeng.lzy/tools/labsjdk-ce-11.0.8-jvmci-20.2-b03/lib	N/A
sun.cpu.endian	little	N/A
sun.cpu.isalist	EMPTY	N/A
sun.io.unicode.encoding	UnicodeLittle	N/A
sun.java.command	PrintAllProperties	N/A
sun.java.launcher	SUN_STANDARD	N/A
sun.jnu.encoding	UTF-8	UTF-8
sun.management.compiler	HotSpot 64-Bit Tiered Compilers	N/A
sun.os.patch.level	unknown	N/A
user.country	US	N/A
user.dir	/home/cengfeng.lzy/svmTestsForCommunity/systemPropertyTest	/home/cengfeng.lzy/svmTestsForCommunity/systemPropertyTest
user.home	/home/cengfeng.lzy	/home/cengfeng.lzy
user.language	en	N/A
user.name	cengfeng.lzy	cengfeng.lzy
user.timezone	EMPTY	N/A

Legends:

EMPTY stands for empty string
N/A means not supported, calling System.getProperty with corresponding property name results null.

The contents in this table are from the result of running following code:

 public static void main(String[] args) {
        Properties p = System.getProperties();
        Enumeration keys = p.keys();
        while (keys.hasMoreElements()) {
            String key = (String) keys.nextElement();
            String value = (String) p.get(key);
            if(value.length()==0) value = "EMPTY";
            System.out.println(key + "=" + value);
        }
    }

Limitation

This patch can only check the calling of System.getProperty() with string literals. Calling with string variables is missed.

christianwimmer

Overall this is an interesting idea. Printing warning messages at image build time sounds helpful. It only works when System.getProperties is called, with a constant string argument. But that is probably a common use case.

I don't see a way to print a warning message at run time. A VM does not produce output on its own. One possibility would be to make a separate "report at run time option" that the user can manually turn on. In that case, the full stack trace should be printed too - just a warning message without any context information is not actionable.

christianwimmer · 2020-09-09T19:27:07Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/SystemPropertiesSupport.java

-        return properties.getProperty(key);
+        String ret = properties.getProperty(key);
+        if (ret == null && isNotSupported(key)) {
+            System.out.println("Java system property " + key + " is not available in native image. Please explicitly assign the expected value via -D" + key + "=, or avoid using this property.");


The VM must never itself print any unsolicited output. So I don't see a way to print a warning message like this.

Fixed by using exception here

christianwimmer · 2020-09-09T19:28:04Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/SystemPropertiesSupport.java

+    }
+
+    public static boolean isNotSupported(String key) {
+        return Arrays.stream(UNSUPPORTED_PROPERTIES).anyMatch(s -> s.equals(key));


General comment: core parts of SVM cannot use streams, because that makes so much of the JDK reachable that the size of a "hello world" explodes.

christianwimmer · 2020-09-09T19:29:38Z

...evm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/UnsupportedSystemPropertyErrors.java

+import java.util.Set;
+
+public class UnsupportedSystemPropertyErrors {
+    private final Set<String> systemPropertyCalls = new HashSet<>();


needs to be a data structure that is thread safe, like ConcurrentHashMap

christianwimmer · 2020-09-09T19:33:22Z

...atevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/snippets/SystemPropertyPlugin.java

+
+public class SystemPropertyPlugin {
+    @SuppressWarnings("unused")
+    public static void registerInvocationPlugins(ImageClassLoader imageClassLoader, SnippetReflectionProvider snippetReflection, AnnotationSubstitutionProcessor annotationSubstitutions,


Why do you pass in all these parameters such as AnnotationSubstitutionProcessor and SVMHost?

Don't need them, removed.

christianwimmer · 2020-09-09T19:36:07Z

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/SystemPropertyFeature.java

+    private AnnotationSubstitutionProcessor annotationSubstitutions;
+
+    public SystemPropertyFeature() {
+        ImageSingletons.add(UnsupportedSystemPropertyErrors.class, new UnsupportedSystemPropertyErrors());


Since all of this checking code is running only during image generation, you

don't need to split it up into the separate "Feature", "Errors", and "Plugins" classes

you don't need to register a separate singleton because the instance of SystemPropertyFeature is automatically available as an image singleton

Merged UnsupportedSystemPropertyErrors and SystemPropertyPlugin classes into SystemPropertyFeature.

christianwimmer · 2020-09-09T19:37:25Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/SystemPropertiesSupport.java

+    /**
+     * System properties are unsupported by default.
+     */
+    private static final String[] UNSUPPORTED_PROPERTIES = {


"unsupported" is in general the wrong term for all of this PR. They are of course supported (since you can manually set them), they just are not set currently by default.

fixed by changing to INCOMPATIBLE_PROPERTIES

christianwimmer · 2020-09-09T19:39:47Z

...atevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/snippets/SystemPropertyPlugin.java

+            }
+
+            @Override
+            public boolean isDecorator() {


No need for this, since the apply method always returns false

christianwimmer · 2020-09-09T19:40:54Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/configure/ConfigurationParser.java

@@ -71,4 +71,4 @@ protected static boolean asBoolean(Object value, String propertyName) {
        }
        throw new JSONParserException("Invalid boolean value '" + value + "' for element '" + propertyName + "'");
    }
-}
+}


Unnecessary change (and also a checkstyle violation)

christianwimmer · 2020-09-09T19:42:03Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/SystemPropertiesSupport.java

+                    "java.vm.info",
+                    "sun.boot.class.path",
+                    "sun.boot.library.path",
+                    "sun.cpu.endian",


that sounds like a property that can be inherited from image build time. Maybe some other too?

ziyilin · 2020-09-11T06:00:21Z

It's necessary to explicitly report the using of incompatible system property, otherwise user may get confused and it's difficult to debug the native image. Proper exception message will help the user to locate the problem.

I add a new option -R:+ReportIncompatibleSystemPropertyError to throw com.oracle.svm.core.jdk.IncompatibleSystemPropertyException when a system property is null. User can manually turn it on.

christianwimmer

Thanks for the cleanups, the code looks pretty good now. I added a few more comments.

christianwimmer · 2020-09-14T16:57:07Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/SystemPropertiesSupport.java

+    }
+
+    public static boolean isIncompatible(String key) {
+        for (int i = 0; i < INCOMPATIBLE_PROPERTIES.length; i++) {


The INCOMPATIBLE_PROPERTIES list is not super-long, but still it is very easy to prepare a Set<String> instead of the String[] array. You just need a new HashSet<>(Arrays.asList(...)) in the INCOMPATIBLE_PROPERTIES definition above.

christianwimmer · 2020-09-14T17:05:04Z

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/SystemPropertyFeature.java

+                if (name.isConstant()) {
+                    String propertyKey = snippetReflection.asObject(String.class, name.asJavaConstant());
+                    if (SystemPropertiesSupport.isIncompatible(propertyKey)) {
+                        systemPropertyCalls.add("Incompatible property:" + propertyKey + " is used at " + b.getCode().asStackTraceElement(b.bci()));


Minor: missing space after : in the message

christianwimmer · 2020-09-14T17:08:44Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/SystemPropertiesSupport.java

+    /**
+     * System properties are unsupported by default.
+     */
+    private static final String[] INCOMPATIBLE_PROPERTIES = {


After thinking about the name for a while, my suggestion is to call it UNDEFINED_PROPERTIES (with the appropriate changes of the option and classes, fields, ...). That name avoids all sorts of legal hassles.

christianwimmer · 2020-09-14T17:46:51Z

...src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/IncompatibleSystemPropertyException.java

@@ -0,0 +1,7 @@
+package com.oracle.svm.core.jdk;


License header missing on this file

You could also just use the UnsupportedFeatureError class that we already have.

vjovanov · 2020-09-15T06:25:03Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/SystemPropertiesSupport.java

@@ -48,6 +50,11 @@
 */
 public abstract class SystemPropertiesSupport {

+    public static class SystemPropertyFeatureOptions{
+        @Option(help = "Report the usage of incompatible property as exception when it is not explicitly set)")//
+        public static final RuntimeOptionKey<Boolean> ReportIncompatibleSystemPropertyError = new RuntimeOptionKey<>(false);


I would name it ReportUnsupportedSystemPropertyErrors.

After thinking about the name for a while, my suggestion is to call it UNDEFINED_PROPERTIES (with the appropriate changes of the option and classes, fields, ...). That name avoids all sorts of legal hassles.

As Chris suggested, I think "undefined" would be a better name.

Correct, please use undefined

vjovanov · 2020-09-15T06:26:10Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/SystemPropertiesSupport.java

-        return properties.getProperty(key);
+        String ret = properties.getProperty(key);
+        if (ret == null && isIncompatible(key) && SystemPropertyFeatureOptions.ReportIncompatibleSystemPropertyError.getValue()) {
+            throw new IncompatibleSystemPropertyException("Java system property " + key + " is incompatible in native image. Please explicitly assign the expected value via -D" + key + "=, or avoid using this property.");


Here as well. I would use UnsupportedSystemPropertyException.

vjovanov · 2020-09-15T06:26:23Z

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/SystemPropertyFeature.java

@@ -0,0 +1,76 @@
+/*
+ * Copyright (c) 2019, 2020, Oracle and/or its affiliates. All rights reserved.


vjovanov · 2020-09-15T06:28:47Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/SystemPropertiesSupport.java

+    private static final String[] INCOMPATIBLE_PROPERTIES = {
+                    "awt.toolkit",
+                    "file.encoding.pkg",
+                    "java.awt.graphicsenv",


@arodionov we should make sure to enable these once AWT is complete.

vjovanov · 2020-09-15T06:30:30Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/SystemPropertiesSupport.java

+                    "java.runtime.name",
+                    "java.runtime.version",
+                    "java.vendor.url.bug",
+                    "java.vm.info",


Can we put native image, native, or AOT here?

christianwimmer

Sorry, did not notice when you pushed the latest version. I think we are getting too complicated now, with the "added" / "removed" properties. Simplifying that is a bit of a refactoring of the existing code, but I think worth it.

christianwimmer · 2020-10-04T03:36:33Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/SystemPropertiesSupport.java

+                    "user.timezone"};
+
+    /** Properties are removed or added since JDK8*/
+    private static final List<PropertyInfo> CHANGED_PROPERTIES_SINCE_8 = new ArrayList<>(


I don't like this "delta" list, since it is difficult to maintain in the long run. Instead, we should change the whole mechansim we have right now (HOSTED_PROPERTIES and initializeProperty()) to a PropertyInfo like you introduced - without the need for "added" and "deleted" but instead with a predicate onlyWith where you can specify the existing classes like JDK8OrEarlier or JDK11OrLater.
Note also that we do not need to support only JDK 8, 11, and 15 - and not any versions in between.

Note also that we do not need to support only JDK 8, 11, and 15 - and not any versions in between.

Do you mean only support 8, 11 and 15, or all the versions in between? I think it should be the later. For example, java.vm.compressedOopsMode is introduced in JDK 9, its onlyWith should be JDK9OrLater.

I mean "only support 8, 11 and 15". Many parts of native image don't support any of the versions in between already. Sticking to the long term support versions plus "latest" keeps the code base manageable.

Refactored.

Many system properties are undefined in native image. Getting such properties with System.getProperty returns null. It may result unexpected behaviors. This patch checks the usage of such properties at native image build, and throw exception at run time if -R:+ReportUndefinedSystemPropertyError option is set.

ziyilin mentioned this pull request Sep 8, 2020

Java system properties are inconsistent between traditional JDK and native image #2835

Open

graalvmbot added the oca-signed label Sep 8, 2020

oubidar-Abderrahim assigned cstancu Sep 9, 2020

christianwimmer reviewed Sep 9, 2020

View reviewed changes

christianwimmer requested a review from vjovanov September 9, 2020 19:45

christianwimmer assigned vjovanov and christianwimmer and unassigned cstancu Sep 9, 2020

ziyilin force-pushed the getProperty branch from 4928fa8 to 350a1ec Compare September 11, 2020 06:05

Jsn2win approved these changes Sep 11, 2020

View reviewed changes

christianwimmer suggested changes Sep 14, 2020

View reviewed changes

christianwimmer reviewed Sep 14, 2020

View reviewed changes

vjovanov reviewed Sep 15, 2020

View reviewed changes

ziyilin force-pushed the getProperty branch from 350a1ec to 2382af5 Compare September 15, 2020 13:01

christianwimmer suggested changes Oct 4, 2020

View reviewed changes

ziyilin force-pushed the getProperty branch from 2382af5 to 8e85bcb Compare October 13, 2020 07:58

scientificware mentioned this pull request Oct 27, 2020

[native-image] Windows with a swing application #1327

Open

kkriske mentioned this pull request Dec 5, 2020

add lazy property initialization api to SystemPropertiesSupport #3049

Closed

		@@ -0,0 +1,76 @@
		/*
		* Copyright (c) 2019, 2020, Oracle and/or its affiliates. All rights reserved.

Report warning for unsupported system properties #2834

Are you sure you want to change the base?

Report warning for unsupported system properties #2834

Conversation

ziyilin commented Sep 8, 2020 • edited Loading

Overview

Features

System Property Differences

Limitation

christianwimmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziyilin commented Sep 11, 2020

christianwimmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vjovanov Sep 15, 2020 • edited Loading

Choose a reason for hiding this comment

christianwimmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziyilin commented Sep 8, 2020 •

edited

Loading

vjovanov Sep 15, 2020 •

edited

Loading