Re: RFR: 8283340: Remove support for -Dsun.misc.URLClassPath.disableJarChecking
On Thu, 17 Mar 2022 23:44:32 GMT, Liam Miller-Cushon wrote: > This change removes support for `-Dsun.misc.URLClassPath.disableJarChecking`. > As discussed in the bug the feature doesn't current work, and only ever > supported scenarios where a security manager is installed, so it seems safe > to remove. src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 806: > 804: > 805: /* Throws if the given jar file is does not start with the > correct LOC */ > 806: @SuppressWarnings("removal") I noticed the @SuppressWarnings("removal") when looking at the PR. It is added by https://github.com/openjdk/jdk/commit/6765f902505fbdd02f25b599f942437cd805cad1 for [JDK-8266459](https://bugs.openjdk.java.net/browse/JDK-8266459). So looks like the method has been targeted for deprcating/removal. - PR: https://git.openjdk.java.net/jdk/pull/7861
Integrated: 8282407: Missing ')' in MacResources.properties
On Sat, 12 Mar 2022 03:12:30 GMT, Alexander Matveev wrote: > - Fixed by adding missing ']'. > - I changed '()' to '[]', since other error messages use '[]' and not '()'. This pull request has now been integrated. Changeset: d83cee98 Author:Alexander Matveev URL: https://git.openjdk.java.net/jdk/commit/d83cee98b5e6628f19f1b5dea11038079dd0c758 Stats: 6 lines in 3 files changed: 0 ins; 0 del; 6 mod 8282407: Missing ')' in MacResources.properties Reviewed-by: naoto - PR: https://git.openjdk.java.net/jdk/pull/7797
Re: RFR: 8283059: Uninitialized warning in check_code.c with GCC 11.2 [v2]
On Fri, 18 Mar 2022 02:39:23 GMT, Mikael Vidstedt wrote: >> Note: this PR replaces the one I messed up earlier. >> >> Background, from JBS: >> >> src/java.base/share/native/libverify/check_code.c: In function >> 'read_all_code': >> src/java.base/share/native/libverify/check_code.c:942:5: error: 'lengths' >> may be used uninitialized [-Werror=maybe-uninitialized] >> 942 | check_and_push(context, lengths, VM_MALLOC_BLK); >> | ^~~ >> src/java.base/share/native/libverify/check_code.c:4145:13: note: by argument >> 2 of type 'const void *' to 'check_and_push' declared here >> 4145 | static void check_and_push(context_type *context, const void *ptr, >> int kind) >> | ^~ >> >> Because the second argument of check_and_push is "const void*" GCC assumes >> that the malloc:ed data, which has not yet been initialized, will not be/can >> not be modified later which in turn suggests it may be used without ever >> being initialized. >> >> The same general issue was addressed in JDK-8266168, presumably for GCC 11.1. >> >> Details: >> >> Instead of sprinkling more calloc calls around or using pragmas/gcc >> attributes I chose to change the check_and_push function to take a >> (non-const) void* argument, and provide a new wrapper function >> check_and_push_const which handles the const argument case. For the >> (non-const) VM_MALLOC_BKP that means the pointer never needs to go through a >> const conversion. >> >> To avoid having multiple ways of solving the same problem I also chose to >> revert the change made in JDK-8266168, reverting the calloc back to a malloc >> call. >> >> Testing: >> >> tier1 + builds-tier{2,3,4,5} > > Mikael Vidstedt has updated the pull request incrementally with one > additional commit since the last revision: > > Use const char for check_and_push_string_utf Looks good! - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7859
Re: RFR: 8283059: Uninitialized warning in check_code.c with GCC 11.2 [v2]
On Fri, 18 Mar 2022 02:10:32 GMT, David Holmes wrote: >> Mikael Vidstedt has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Use const char for check_and_push_string_utf > > src/java.base/share/native/libverify/check_code.c line 472: > >> 470: >> 471: static void check_and_push_malloc_block(context_type *context, void >> *ptr); >> 472: static void check_and_push_string_utf(context_type *context, const void >> *ptr); > > Can't this be: > > `static void check_and_push_string_utf(context_type *context, const char* > str);` Indeed it can. Fixed. - PR: https://git.openjdk.java.net/jdk/pull/7859
Re: RFR: 8283059: Uninitialized warning in check_code.c with GCC 11.2 [v2]
> Note: this PR replaces the one I messed up earlier. > > Background, from JBS: > > src/java.base/share/native/libverify/check_code.c: In function > 'read_all_code': > src/java.base/share/native/libverify/check_code.c:942:5: error: 'lengths' may > be used uninitialized [-Werror=maybe-uninitialized] > 942 | check_and_push(context, lengths, VM_MALLOC_BLK); > | ^~~ > src/java.base/share/native/libverify/check_code.c:4145:13: note: by argument > 2 of type 'const void *' to 'check_and_push' declared here > 4145 | static void check_and_push(context_type *context, const void *ptr, int > kind) > | ^~ > > Because the second argument of check_and_push is "const void*" GCC assumes > that the malloc:ed data, which has not yet been initialized, will not be/can > not be modified later which in turn suggests it may be used without ever > being initialized. > > The same general issue was addressed in JDK-8266168, presumably for GCC 11.1. > > Details: > > Instead of sprinkling more calloc calls around or using pragmas/gcc > attributes I chose to change the check_and_push function to take a > (non-const) void* argument, and provide a new wrapper function > check_and_push_const which handles the const argument case. For the > (non-const) VM_MALLOC_BKP that means the pointer never needs to go through a > const conversion. > > To avoid having multiple ways of solving the same problem I also chose to > revert the change made in JDK-8266168, reverting the calloc back to a malloc > call. > > Testing: > > tier1 + builds-tier{2,3,4,5} Mikael Vidstedt has updated the pull request incrementally with one additional commit since the last revision: Use const char for check_and_push_string_utf - Changes: - all: https://git.openjdk.java.net/jdk/pull/7859/files - new: https://git.openjdk.java.net/jdk/pull/7859/files/c5c60cb2..9f412d0b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7859=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7859=00-01 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/7859.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7859/head:pull/7859 PR: https://git.openjdk.java.net/jdk/pull/7859
Re: When to initialize the method's class for MethodHandles.Lookup.findStatic()?
On 18/03/2022 10:51 am, Cheng Jin wrote: Hi Raffaello, My concern is why the verification happens even without initialization in the test without mh.invoke() in the main(), which I don't think is covered or explained in the JVM Spec. Put it in another way, my understanding is, when the class gets loaded, it is verified which doesn't necessarily lead to initialization, am I correct? From Section 5.4 of the JVMS: This specification allows an implementation flexibility as to when linking activities (and, because of recursion, loading) take place, provided that all of the following properties are maintained: • A class or interface is completely loaded before it is linked. • A class or interface is completely verified and prepared before it is initialized. --- Hotspot does loading, verification and preparing upfront/eagerly. HTH. David - Best Regards Cheng -Original Message- From: core-libs-dev On Behalf Of Raffaello Giulietti Sent: March 17, 2022 8:35 PM To: core-libs-dev@openjdk.java.net Subject: [EXTERNAL] Re: When to initialize the method's class for MethodHandles.Lookup.findStatic()? Cheng, initialization is the last thing that happens because it's where user provided code gets executed. This has always been this way, as long as I can remember. See the JVMS for the gory details. Greetings Raffaello On 2022-03-18 01:21, Cheng Jin wrote: Hi David, 1) for the test with mh.invoke() in main(), the log shows: [0.262s][info][class,init] Start class verification for: Test_1 [0.262s][info][class,init] End class verification for: Test_1 [0.263s][info][class,init] 282 Initializing 'Test_1' (0x000800c00800) [0.263s][info][class,init] Start class verification for: Test_2 [0.263s][info][class,init] End class verification for: Test_2 [0.272s][info][class,init] 366 Initializing 'Test_2' (0x000800c00a08) <-- 2) for the test without mh.invoke() in main(), the log shows: [0.296s][info][class,init] Start class verification for: Test_1 [0.296s][info][class,init] End class verification for: Test_1 [0.297s][info][class,init] 282 Initializing 'Test_1' (0x000800c00800) [0.297s][info][class,init] Start class verification for: Test_2 [0.297s][info][class,init] End class verification for: Test_2 (Test_2 was verified but didn't get initialized) The comparison above literally surprised me that the bytecode verification happened prior to the class initialization, which means the class got verified at first even without initialization coz I previously thought the initialization should trigger the verification rather than in the reversed order. Could you explain a little more about why it goes in this way? Best Regards Cheng -Original Message- From: core-libs-dev On Behalf Of David Holmes Sent: March 17, 2022 7:46 PM To: Raffaello Giulietti ; core-libs-dev@openjdk.java.net Subject: [EXTERNAL] Re: When to initialize the method's class for MethodHandles.Lookup.findStatic()? Run with -Xlog:class+init=info to see the classes that get initialized and in what order. David On 18/03/2022 5:53 am, Raffaello Giulietti wrote: Hi again, here's code that shows that initialization doesn't happen during lookup but only upon invoking the method handle. (I'm on Java 17.) As long as the 2nd line in main() is commented, you don't see the message "Test_2 initialized", which shows that the lookup doesn't initialize Test_2. When you uncomment the line in main(), the message will appear. So, as advertised, it's the invocation of the method handle that can trigger initialization, not the lookup. HTH Raffaello import java.lang.invoke.*; public class Test_1 { static MethodHandle mh; static { try { mh = MethodHandles.lookup().findStatic(Test_2.class, "testMethod", MethodType.methodType(int.class, int.class)); } catch (NoSuchMethodException | IllegalAccessException e) { e.printStackTrace(); } } public static void main(String[] args) throws Throwable { System.out.println(mh); // System.out.println(Test_1.mh.invoke(0)); } } public class Test_2 { static { System.out.println("Test_2 initialized"); } static int testMethod(int value) { return (value + 1); } } On 2022-03-17 20:38, Raffaello Giulietti wrote: Hi, as far as I can see, the code should not even compile, as there's a static field in Test_1 which is initialized with an expression that throws checked exceptions (findStatic(..)). In addition, it seems to me that there's nothing in your code that reveals whether Test_2 has been initialized during the lookup. How can you tell? Finally, the method handle invocation in Test_1 will throw, as you don't pass any argument to a handle that expects one. Can you perhaps add more details? Greetings Raffaello On 2022-03-17 17:42, Cheng Jin wrote: Hi there, The document of INVALID URI REMOVED
Re: RFR: 8283225: ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 (aix) [v4]
On Thu, 17 Mar 2022 16:23:14 GMT, Tyler Steele wrote: >> As described in the linked issue, NullClassBytesTest fails due an >> OutOfMemoryError produced on AIX when the test calls defineClass with a byte >> array of size of 0. The native implementation of defineClass then calls >> malloc with a size of 0. On AIX malloc(0) returns NULL, while on other >> platforms it return a valid address. When NULL is produced by malloc for >> this reason, ClassLoader.c incorrectly interprets this as a failure due to a >> lack of memory. >> >> ~~This PR modifies ClassLoader.c to produce an OutOfMemoryError only when >> `errno == ENOMEM` and to produce a ClassFormatError with the message >> "ClassLoader internal allocation failure" in all other cases (in which >> malloc returns NULL).~~ [edit: The above no longer describes the PR's >> proposed fix. See discussion below] >> >> In addition, I performed some minor tidy-up work in ClassLoader.c by >> changing instances of `return 0` to `return NULL`, and `if (some_ptr == 0)` >> to `if (some_ptr == NULL)`. This was done to improve the clarity of the code >> in ClassLoader.c, but didn't feel worthy of opening a separate issue. >> >> ### Alternatives >> >> It would be possible to address this failure by modifying the test to accept >> the OutOfMemoryError on AIX. I thought it was a better solution to modify >> ClassLoader.c to produce an OutOfMemoryError only when the system is >> actually out of memory. >> >> ### Testing >> >> This change has been tested on AIX and Linux/x86. > > Tyler Steele has updated the pull request incrementally with one additional > commit since the last revision: > > Improve comment > > - Reword to avoid double use of malloc(X) > - Remove bug id Update looks good. Sorry for the AIX_ONLY misdirect. Thanks, David - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283059: Uninitialized warning in check_code.c with GCC 11.2
On Thu, 17 Mar 2022 20:56:34 GMT, Mikael Vidstedt wrote: > Note: this PR replaces the one I messed up earlier. > > Background, from JBS: > > src/java.base/share/native/libverify/check_code.c: In function > 'read_all_code': > src/java.base/share/native/libverify/check_code.c:942:5: error: 'lengths' may > be used uninitialized [-Werror=maybe-uninitialized] > 942 | check_and_push(context, lengths, VM_MALLOC_BLK); > | ^~~ > src/java.base/share/native/libverify/check_code.c:4145:13: note: by argument > 2 of type 'const void *' to 'check_and_push' declared here > 4145 | static void check_and_push(context_type *context, const void *ptr, int > kind) > | ^~ > > Because the second argument of check_and_push is "const void*" GCC assumes > that the malloc:ed data, which has not yet been initialized, will not be/can > not be modified later which in turn suggests it may be used without ever > being initialized. > > The same general issue was addressed in JDK-8266168, presumably for GCC 11.1. > > Details: > > Instead of sprinkling more calloc calls around or using pragmas/gcc > attributes I chose to change the check_and_push function to take a > (non-const) void* argument, and provide a new wrapper function > check_and_push_const which handles the const argument case. For the > (non-const) VM_MALLOC_BKP that means the pointer never needs to go through a > const conversion. > > To avoid having multiple ways of solving the same problem I also chose to > revert the change made in JDK-8266168, reverting the calloc back to a malloc > call. > > Testing: > > tier1 + builds-tier{2,3,4,5} Changes requested by dholmes (Reviewer). src/java.base/share/native/libverify/check_code.c line 472: > 470: > 471: static void check_and_push_malloc_block(context_type *context, void > *ptr); > 472: static void check_and_push_string_utf(context_type *context, const void > *ptr); Can't this be: `static void check_and_push_string_utf(context_type *context, const char* str);` - PR: https://git.openjdk.java.net/jdk/pull/7859
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v19]
On Wed, 16 Mar 2022 14:54:41 GMT, Yasser Bazzi wrote: >> Hi, could i get a review on this implementation proposed by Stuart Marks, i >> decided to use the >> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html >> interface to create the default method `asRandom()` that wraps around the >> newer algorithms to be used on classes that do not accept the new interface. >> >> Some things to note as proposed by the bug report, the protected method >> next(int bits) is not overrided and setSeed() method if left blank up to >> discussion on what to do with it. >> >> Small test done on >> https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4 > > Yasser Bazzi has updated the pull request incrementally with one additional > commit since the last revision: > > add since in javadoc > The current API docs on `Random.setSeed` look like: > > ```java > /** > * Sets the seed of this random number generator using a single > * {@code long} seed. The general contract of {@code setSeed} is > * that it alters the state of this random number generator object > * so as to be in exactly the same state as if it had just been > * created with the argument {@code seed} as a seed. The method > * {@code setSeed} is implemented by class {@code Random} by > * atomically updating the seed to > * {@code (seed ^ 0x5DEECE66DL) & ((1L << 48) - 1)} > * and clearing the {@code haveNextNextGaussian} flag used by {@link > * #nextGaussian}. > * > * The implementation of {@code setSeed} by class {@code Random} > * happens to use only 48 bits of the given seed. In general, however, > * an overriding method may use all 64 bits of the {@code long} > * argument as a seed value. > * > * @param seed the initial seed > */ > ``` > > An updated version would possibly look like: > > ```java > /** > * Sets the seed of this random number generator using a single > * {@code long} seed (optional operation). > * > * @implSpec The general contract of {@code setSeed} is > * that it alters the state of this random number generator object > * so as to be in exactly the same state as if it had just been > * created with the argument {@code seed} as a seed. > * > * The method {@code setSeed} is implemented by class > * {@code Random} by atomically updating the seed to > * {@code (seed ^ 0x5DEECE66DL) & ((1L << 48) - 1)} > * and clearing the {@code haveNextNextGaussian} flag used by {@link > * #nextGaussian}. > * > * The implementation of {@code setSeed} by class {@code Random} > * happens to use only 48 bits of the given seed. In general, however, > * an overriding method may use all 64 bits of the {@code long} > * argument as a seed value. > * > * @param seed the initial seed > * @throws UnsupportedOperationException if the {@code setSeed} > * operation is not supported by this random number generator > */ > ``` > > This moves pretty much everything besides the first sentence to `@implSpec`, > as they are only applicable to the `Random` class, but not its subclasses. > The rest is an emulation of `List.add` javadoc on optional operations. Please also update the Random.setSeed spec to accommodate the behavior of SecureRandom.setSeed: "Reseeds this random object, using the eight bytes contained in the given long seed. The given seed supplements, rather than replaces, the existing seed. Thus, repeated calls are guaranteed never to reduce randomness. This method is defined for compatibility with java.util.Random." https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/security/SecureRandom.html#setSeed(long) Thanks. - PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v19]
On Wed, 16 Mar 2022 14:54:41 GMT, Yasser Bazzi wrote: >> Hi, could i get a review on this implementation proposed by Stuart Marks, i >> decided to use the >> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html >> interface to create the default method `asRandom()` that wraps around the >> newer algorithms to be used on classes that do not accept the new interface. >> >> Some things to note as proposed by the bug report, the protected method >> next(int bits) is not overrided and setSeed() method if left blank up to >> discussion on what to do with it. >> >> Small test done on >> https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4 > > Yasser Bazzi has updated the pull request incrementally with one additional > commit since the last revision: > > add since in javadoc The current API docs on `Random.setSeed` look like: /** * Sets the seed of this random number generator using a single * {@code long} seed. The general contract of {@code setSeed} is * that it alters the state of this random number generator object * so as to be in exactly the same state as if it had just been * created with the argument {@code seed} as a seed. The method * {@code setSeed} is implemented by class {@code Random} by * atomically updating the seed to * {@code (seed ^ 0x5DEECE66DL) & ((1L << 48) - 1)} * and clearing the {@code haveNextNextGaussian} flag used by {@link * #nextGaussian}. * * The implementation of {@code setSeed} by class {@code Random} * happens to use only 48 bits of the given seed. In general, however, * an overriding method may use all 64 bits of the {@code long} * argument as a seed value. * * @param seed the initial seed */ An updated version would possibly look like: /** * Sets the seed of this random number generator using a single * {@code long} seed (optional operation). * * @implSpec The general contract of {@code setSeed} is * that it alters the state of this random number generator object * so as to be in exactly the same state as if it had just been * created with the argument {@code seed} as a seed. * * The method {@code setSeed} is implemented by class * {@code Random} by atomically updating the seed to * {@code (seed ^ 0x5DEECE66DL) & ((1L << 48) - 1)} * and clearing the {@code haveNextNextGaussian} flag used by {@link * #nextGaussian}. * * The implementation of {@code setSeed} by class {@code Random} * happens to use only 48 bits of the given seed. In general, however, * an overriding method may use all 64 bits of the {@code long} * argument as a seed value. * * @param seed the initial seed * @throws UnsupportedOperationException if the {@code setSeed} * operation is not supported by this random number generator */ This moves pretty much everything besides the first sentence to `@implSpec`, as they are only applicable to the `Random` class, but not its subclasses. The rest is an emulation of `List.add` javadoc on optional operations. - PR: https://git.openjdk.java.net/jdk/pull/7001
Re: When to initialize the method's class for MethodHandles.Lookup.findStatic()?
OK, from what I read so far from you is that you were surprised about the order, not about why verification happens even without initialization. Verification is part of linking, which surely must happen before user code (e.g., initialization) is executed, which explains the order. But once linked (thus, verified), there's no pressure to have initialization code (user code) executed. This can wait until the very last moment, when some other code of the class is invoked, or some field is accessed, or an instance is created, etc. It's a lazy execution strategy, which the JVM spec allows and which HotSpot implements this way. HTH Raffaello On 2022-03-18 01:51, Cheng Jin wrote: Hi Raffaello, My concern is why the verification happens even without initialization in the test without mh.invoke() in the main(), which I don't think is covered or explained in the JVM Spec. Put it in another way, my understanding is, when the class gets loaded, it is verified which doesn't necessarily lead to initialization, am I correct? Best Regards Cheng -Original Message- From: core-libs-dev On Behalf Of Raffaello Giulietti Sent: March 17, 2022 8:35 PM To: core-libs-dev@openjdk.java.net Subject: [EXTERNAL] Re: When to initialize the method's class for MethodHandles.Lookup.findStatic()? Cheng, initialization is the last thing that happens because it's where user provided code gets executed. This has always been this way, as long as I can remember. See the JVMS for the gory details. Greetings Raffaello On 2022-03-18 01:21, Cheng Jin wrote: Hi David, 1) for the test with mh.invoke() in main(), the log shows: [0.262s][info][class,init] Start class verification for: Test_1 [0.262s][info][class,init] End class verification for: Test_1 [0.263s][info][class,init] 282 Initializing 'Test_1' (0x000800c00800) [0.263s][info][class,init] Start class verification for: Test_2 [0.263s][info][class,init] End class verification for: Test_2 [0.272s][info][class,init] 366 Initializing 'Test_2' (0x000800c00a08) <-- 2) for the test without mh.invoke() in main(), the log shows: [0.296s][info][class,init] Start class verification for: Test_1 [0.296s][info][class,init] End class verification for: Test_1 [0.297s][info][class,init] 282 Initializing 'Test_1' (0x000800c00800) [0.297s][info][class,init] Start class verification for: Test_2 [0.297s][info][class,init] End class verification for: Test_2 (Test_2 was verified but didn't get initialized) The comparison above literally surprised me that the bytecode verification happened prior to the class initialization, which means the class got verified at first even without initialization coz I previously thought the initialization should trigger the verification rather than in the reversed order. Could you explain a little more about why it goes in this way? Best Regards Cheng -Original Message- From: core-libs-dev On Behalf Of David Holmes Sent: March 17, 2022 7:46 PM To: Raffaello Giulietti ; core-libs-dev@openjdk.java.net Subject: [EXTERNAL] Re: When to initialize the method's class for MethodHandles.Lookup.findStatic()? Run with -Xlog:class+init=info to see the classes that get initialized and in what order. David On 18/03/2022 5:53 am, Raffaello Giulietti wrote: Hi again, here's code that shows that initialization doesn't happen during lookup but only upon invoking the method handle. (I'm on Java 17.) As long as the 2nd line in main() is commented, you don't see the message "Test_2 initialized", which shows that the lookup doesn't initialize Test_2. When you uncomment the line in main(), the message will appear. So, as advertised, it's the invocation of the method handle that can trigger initialization, not the lookup. HTH Raffaello import java.lang.invoke.*; public class Test_1 { static MethodHandle mh; static { try { mh = MethodHandles.lookup().findStatic(Test_2.class, "testMethod", MethodType.methodType(int.class, int.class)); } catch (NoSuchMethodException | IllegalAccessException e) { e.printStackTrace(); } } public static void main(String[] args) throws Throwable { System.out.println(mh); // System.out.println(Test_1.mh.invoke(0)); } } public class Test_2 { static { System.out.println("Test_2 initialized"); } static int testMethod(int value) { return (value + 1); } } On 2022-03-17 20:38, Raffaello Giulietti wrote: Hi, as far as I can see, the code should not even compile, as there's a static field in Test_1 which is initialized with an expression that throws checked exceptions (findStatic(..)). In addition, it seems to me that there's nothing in your code that reveals whether Test_2 has been initialized during the lookup. How can you tell? Finally, the method handle invocation in Test_1 will throw, as you don't pass any argument to a
RE: When to initialize the method's class for MethodHandles.Lookup.findStatic()?
Hi Raffaello, My concern is why the verification happens even without initialization in the test without mh.invoke() in the main(), which I don't think is covered or explained in the JVM Spec. Put it in another way, my understanding is, when the class gets loaded, it is verified which doesn't necessarily lead to initialization, am I correct? Best Regards Cheng -Original Message- From: core-libs-dev On Behalf Of Raffaello Giulietti Sent: March 17, 2022 8:35 PM To: core-libs-dev@openjdk.java.net Subject: [EXTERNAL] Re: When to initialize the method's class for MethodHandles.Lookup.findStatic()? Cheng, initialization is the last thing that happens because it's where user provided code gets executed. This has always been this way, as long as I can remember. See the JVMS for the gory details. Greetings Raffaello On 2022-03-18 01:21, Cheng Jin wrote: > Hi David, > > 1) for the test with mh.invoke() in main(), the log shows: > [0.262s][info][class,init] Start class verification for: Test_1 > [0.262s][info][class,init] End class verification for: Test_1 > [0.263s][info][class,init] 282 Initializing 'Test_1' (0x000800c00800) > [0.263s][info][class,init] Start class verification for: Test_2 > [0.263s][info][class,init] End class verification for: Test_2 > [0.272s][info][class,init] 366 Initializing 'Test_2' (0x000800c00a08) > <-- > > 2) for the test without mh.invoke() in main(), the log shows: > [0.296s][info][class,init] Start class verification for: Test_1 > [0.296s][info][class,init] End class verification for: Test_1 > [0.297s][info][class,init] 282 Initializing 'Test_1' (0x000800c00800) > [0.297s][info][class,init] Start class verification for: Test_2 > [0.297s][info][class,init] End class verification for: Test_2 > (Test_2 was verified but didn't get initialized) > > The comparison above literally surprised me that the bytecode verification > happened prior to the class initialization, which means > the class got verified at first even without initialization coz I previously > thought the initialization should trigger the verification rather than in the > reversed order. > > Could you explain a little more about why it goes in this way? > > Best Regards > Cheng > > > -Original Message- > From: core-libs-dev On Behalf Of David > Holmes > Sent: March 17, 2022 7:46 PM > To: Raffaello Giulietti ; > core-libs-dev@openjdk.java.net > Subject: [EXTERNAL] Re: When to initialize the method's class for > MethodHandles.Lookup.findStatic()? > > Run with -Xlog:class+init=info to see the classes that get initialized and in > what order. > > David > > On 18/03/2022 5:53 am, Raffaello Giulietti wrote: >> Hi again, >> >> here's code that shows that initialization doesn't happen during >> lookup but only upon invoking the method handle. (I'm on Java 17.) >> >> As long as the 2nd line in main() is commented, you don't see the >> message "Test_2 initialized", which shows that the lookup doesn't >> initialize Test_2. >> When you uncomment the line in main(), the message will appear. >> >> So, as advertised, it's the invocation of the method handle that can >> trigger initialization, not the lookup. >> >> >> HTH >> Raffaello >> >> >> >> import java.lang.invoke.*; >> >> public class Test_1 { >> >> static MethodHandle mh; >> >> static { >> try { >> mh = MethodHandles.lookup().findStatic(Test_2.class, >> "testMethod", MethodType.methodType(int.class, int.class)); >> } catch (NoSuchMethodException | IllegalAccessException e) { >> e.printStackTrace(); >> } >> } >> >> public static void main(String[] args) throws Throwable { >> System.out.println(mh); >> // System.out.println(Test_1.mh.invoke(0)); >> } >> >> } >> >> >> >> >> public class Test_2 { >> >> static { >> System.out.println("Test_2 initialized"); >> } >> >> static int testMethod(int value) { return (value + 1); } >> >> } >> >> >> >> >> On 2022-03-17 20:38, Raffaello Giulietti wrote: >>> Hi, >>> >>> as far as I can see, the code should not even compile, as there's a >>> static field in Test_1 which is initialized with an expression that >>> throws checked exceptions (findStatic(..)). >>> >>> In addition, it seems to me that there's nothing in your code that >>> reveals whether Test_2 has been initialized during the lookup. How >>> can you tell? >>> >>> Finally, the method handle invocation in Test_1 will throw, as you >>> don't pass any argument to a handle that expects one. >>> >>> Can you perhaps add more details? >>> >>> >>> Greetings >>> Raffaello >>> >>> >>> >>> On 2022-03-17 17:42, Cheng Jin wrote: Hi there, The document of INVALID URI REMOVED _en_java_javase_17_docs_api_java.base_java_lang_invoke_MethodHandles .Lookup.html-23findStatic-28java.lang.Class-2Cjava.lang.String-2Cjav
Re: When to initialize the method's class for MethodHandles.Lookup.findStatic()?
Cheng, initialization is the last thing that happens because it's where user provided code gets executed. This has always been this way, as long as I can remember. See the JVMS for the gory details. Greetings Raffaello On 2022-03-18 01:21, Cheng Jin wrote: Hi David, 1) for the test with mh.invoke() in main(), the log shows: [0.262s][info][class,init] Start class verification for: Test_1 [0.262s][info][class,init] End class verification for: Test_1 [0.263s][info][class,init] 282 Initializing 'Test_1' (0x000800c00800) [0.263s][info][class,init] Start class verification for: Test_2 [0.263s][info][class,init] End class verification for: Test_2 [0.272s][info][class,init] 366 Initializing 'Test_2' (0x000800c00a08) <-- 2) for the test without mh.invoke() in main(), the log shows: [0.296s][info][class,init] Start class verification for: Test_1 [0.296s][info][class,init] End class verification for: Test_1 [0.297s][info][class,init] 282 Initializing 'Test_1' (0x000800c00800) [0.297s][info][class,init] Start class verification for: Test_2 [0.297s][info][class,init] End class verification for: Test_2 (Test_2 was verified but didn't get initialized) The comparison above literally surprised me that the bytecode verification happened prior to the class initialization, which means the class got verified at first even without initialization coz I previously thought the initialization should trigger the verification rather than in the reversed order. Could you explain a little more about why it goes in this way? Best Regards Cheng -Original Message- From: core-libs-dev On Behalf Of David Holmes Sent: March 17, 2022 7:46 PM To: Raffaello Giulietti ; core-libs-dev@openjdk.java.net Subject: [EXTERNAL] Re: When to initialize the method's class for MethodHandles.Lookup.findStatic()? Run with -Xlog:class+init=info to see the classes that get initialized and in what order. David On 18/03/2022 5:53 am, Raffaello Giulietti wrote: Hi again, here's code that shows that initialization doesn't happen during lookup but only upon invoking the method handle. (I'm on Java 17.) As long as the 2nd line in main() is commented, you don't see the message "Test_2 initialized", which shows that the lookup doesn't initialize Test_2. When you uncomment the line in main(), the message will appear. So, as advertised, it's the invocation of the method handle that can trigger initialization, not the lookup. HTH Raffaello import java.lang.invoke.*; public class Test_1 { static MethodHandle mh; static { try { mh = MethodHandles.lookup().findStatic(Test_2.class, "testMethod", MethodType.methodType(int.class, int.class)); } catch (NoSuchMethodException | IllegalAccessException e) { e.printStackTrace(); } } public static void main(String[] args) throws Throwable { System.out.println(mh); // System.out.println(Test_1.mh.invoke(0)); } } public class Test_2 { static { System.out.println("Test_2 initialized"); } static int testMethod(int value) { return (value + 1); } } On 2022-03-17 20:38, Raffaello Giulietti wrote: Hi, as far as I can see, the code should not even compile, as there's a static field in Test_1 which is initialized with an expression that throws checked exceptions (findStatic(..)). In addition, it seems to me that there's nothing in your code that reveals whether Test_2 has been initialized during the lookup. How can you tell? Finally, the method handle invocation in Test_1 will throw, as you don't pass any argument to a handle that expects one. Can you perhaps add more details? Greetings Raffaello On 2022-03-17 17:42, Cheng Jin wrote: Hi there, The document of INVALID URI REMOVED _en_java_javase_17_docs_api_java.base_java_lang_invoke_MethodHandles .Lookup.html-23findStatic-28java.lang.Class-2Cjava.lang.String-2Cjav a.lang.invoke.MethodType-29=DwIDaQ=jf_iaSHvJObTbx-siA1ZOg=X90f 3XIRXAH8hbNam6bIUlWfF_qUAezL9ue7M7bFuPQ=RvhguidNJ90V-HK-3Ctl-kUZE5 cIfo_nt3_r8VZ0Fcc=tw_ph6oUkS0eCvzITWi9zEkarss5yNeHDrAIfvd3s3g= in the Java API is ambiguous in terms of when to initialize the method's class as follows (the same description as in other OpenJDK versions) If the returned method handle is invoked, the method's class will be initialized, if it has not already been initialized. It occurs to me that the method's class should be initialized when invoking the method handle but OpenJDK actually chooses to do the initialization in lookup.findStatic() rather than in mh.invoke() e.g. import java.lang.invoke.*; public class Test_1 { static MethodHandle mh = MethodHandles.lookup().findStatic(Test_2.class, "testMethod", MethodType.methodType(int.class, int.class)); <--- Test_2.class gets initialized and verified. public static void main(String[] args) throws Throwable { Test_1.mh.invoke(); } }
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v19]
On Wed, 16 Mar 2022 14:54:41 GMT, Yasser Bazzi wrote: >> Hi, could i get a review on this implementation proposed by Stuart Marks, i >> decided to use the >> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html >> interface to create the default method `asRandom()` that wraps around the >> newer algorithms to be used on classes that do not accept the new interface. >> >> Some things to note as proposed by the bug report, the protected method >> next(int bits) is not overrided and setSeed() method if left blank up to >> discussion on what to do with it. >> >> Small test done on >> https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4 > > Yasser Bazzi has updated the pull request incrementally with one additional > commit since the last revision: > > add since in javadoc The toString method of both classes do not implement anything so i dont think we should override it, also as per request of Joe Darcy how should we proceed with the annotation of implSpec? - PR: https://git.openjdk.java.net/jdk/pull/7001
Re: When to initialize the method's class for MethodHandles.Lookup.findStatic()?
Sure, which again shows that Test_2 is not initialized by the lookup of the mh but only upon its invocation. Raffaello On 2022-03-18 00:46, David Holmes wrote: Run with -Xlog:class+init=info to see the classes that get initialized and in what order. David On 18/03/2022 5:53 am, Raffaello Giulietti wrote: Hi again, here's code that shows that initialization doesn't happen during lookup but only upon invoking the method handle. (I'm on Java 17.) As long as the 2nd line in main() is commented, you don't see the message "Test_2 initialized", which shows that the lookup doesn't initialize Test_2. When you uncomment the line in main(), the message will appear. So, as advertised, it's the invocation of the method handle that can trigger initialization, not the lookup. HTH Raffaello import java.lang.invoke.*; public class Test_1 { static MethodHandle mh; static { try { mh = MethodHandles.lookup().findStatic(Test_2.class, "testMethod", MethodType.methodType(int.class, int.class)); } catch (NoSuchMethodException | IllegalAccessException e) { e.printStackTrace(); } } public static void main(String[] args) throws Throwable { System.out.println(mh); // System.out.println(Test_1.mh.invoke(0)); } } public class Test_2 { static { System.out.println("Test_2 initialized"); } static int testMethod(int value) { return (value + 1); } } On 2022-03-17 20:38, Raffaello Giulietti wrote: Hi, as far as I can see, the code should not even compile, as there's a static field in Test_1 which is initialized with an expression that throws checked exceptions (findStatic(..)). In addition, it seems to me that there's nothing in your code that reveals whether Test_2 has been initialized during the lookup. How can you tell? Finally, the method handle invocation in Test_1 will throw, as you don't pass any argument to a handle that expects one. Can you perhaps add more details? Greetings Raffaello On 2022-03-17 17:42, Cheng Jin wrote: Hi there, The document of https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#findStatic(java.lang.Class,java.lang.String,java.lang.invoke.MethodType) in the Java API is ambiguous in terms of when to initialize the method's class as follows (the same description as in other OpenJDK versions) If the returned method handle is invoked, the method's class will be initialized, if it has not already been initialized. It occurs to me that the method's class should be initialized when invoking the method handle but OpenJDK actually chooses to do the initialization in lookup.findStatic() rather than in mh.invoke() e.g. import java.lang.invoke.*; public class Test_1 { static MethodHandle mh = MethodHandles.lookup().findStatic(Test_2.class, "testMethod", MethodType.methodType(int.class, int.class)); <--- Test_2.class gets initialized and verified. public static void main(String[] args) throws Throwable { Test_1.mh.invoke(); } } public class Test_2 { static int testMethod(int value) { return (value + 1); } } So there should be more clear explanation what is the correct or expected behaviour at this point and why OpenJDK doesn't comply with the document to delay the initialization of the method's class to mh.invoke(). Best Regards Cheng Jin
RE: When to initialize the method's class for MethodHandles.Lookup.findStatic()?
Hi David, 1) for the test with mh.invoke() in main(), the log shows: [0.262s][info][class,init] Start class verification for: Test_1 [0.262s][info][class,init] End class verification for: Test_1 [0.263s][info][class,init] 282 Initializing 'Test_1' (0x000800c00800) [0.263s][info][class,init] Start class verification for: Test_2 [0.263s][info][class,init] End class verification for: Test_2 [0.272s][info][class,init] 366 Initializing 'Test_2' (0x000800c00a08) <-- 2) for the test without mh.invoke() in main(), the log shows: [0.296s][info][class,init] Start class verification for: Test_1 [0.296s][info][class,init] End class verification for: Test_1 [0.297s][info][class,init] 282 Initializing 'Test_1' (0x000800c00800) [0.297s][info][class,init] Start class verification for: Test_2 [0.297s][info][class,init] End class verification for: Test_2 (Test_2 was verified but didn't get initialized) The comparison above literally surprised me that the bytecode verification happened prior to the class initialization, which means the class got verified at first even without initialization coz I previously thought the initialization should trigger the verification rather than in the reversed order. Could you explain a little more about why it goes in this way? Best Regards Cheng -Original Message- From: core-libs-dev On Behalf Of David Holmes Sent: March 17, 2022 7:46 PM To: Raffaello Giulietti ; core-libs-dev@openjdk.java.net Subject: [EXTERNAL] Re: When to initialize the method's class for MethodHandles.Lookup.findStatic()? Run with -Xlog:class+init=info to see the classes that get initialized and in what order. David On 18/03/2022 5:53 am, Raffaello Giulietti wrote: > Hi again, > > here's code that shows that initialization doesn't happen during > lookup but only upon invoking the method handle. (I'm on Java 17.) > > As long as the 2nd line in main() is commented, you don't see the > message "Test_2 initialized", which shows that the lookup doesn't > initialize Test_2. > When you uncomment the line in main(), the message will appear. > > So, as advertised, it's the invocation of the method handle that can > trigger initialization, not the lookup. > > > HTH > Raffaello > > > > import java.lang.invoke.*; > > public class Test_1 { > > static MethodHandle mh; > > static { > try { > mh = MethodHandles.lookup().findStatic(Test_2.class, > "testMethod", MethodType.methodType(int.class, int.class)); > } catch (NoSuchMethodException | IllegalAccessException e) { > e.printStackTrace(); > } > } > > public static void main(String[] args) throws Throwable { > System.out.println(mh); > // System.out.println(Test_1.mh.invoke(0)); > } > > } > > > > > public class Test_2 { > > static { > System.out.println("Test_2 initialized"); > } > > static int testMethod(int value) { return (value + 1); } > > } > > > > > On 2022-03-17 20:38, Raffaello Giulietti wrote: >> Hi, >> >> as far as I can see, the code should not even compile, as there's a >> static field in Test_1 which is initialized with an expression that >> throws checked exceptions (findStatic(..)). >> >> In addition, it seems to me that there's nothing in your code that >> reveals whether Test_2 has been initialized during the lookup. How >> can you tell? >> >> Finally, the method handle invocation in Test_1 will throw, as you >> don't pass any argument to a handle that expects one. >> >> Can you perhaps add more details? >> >> >> Greetings >> Raffaello >> >> >> >> On 2022-03-17 17:42, Cheng Jin wrote: >>> Hi there, >>> >>> The document of >>> INVALID URI REMOVED >>> _en_java_javase_17_docs_api_java.base_java_lang_invoke_MethodHandles >>> .Lookup.html-23findStatic-28java.lang.Class-2Cjava.lang.String-2Cjav >>> a.lang.invoke.MethodType-29=DwIDaQ=jf_iaSHvJObTbx-siA1ZOg=X90f >>> 3XIRXAH8hbNam6bIUlWfF_qUAezL9ue7M7bFuPQ=RvhguidNJ90V-HK-3Ctl-kUZE5 >>> cIfo_nt3_r8VZ0Fcc=tw_ph6oUkS0eCvzITWi9zEkarss5yNeHDrAIfvd3s3g= >>> in the Java API is ambiguous in terms of when to initialize the >>> method's class as follows (the same description as in other OpenJDK >>> versions) >>> >>> If the returned method handle is invoked, the method's class will be >>> initialized, if it has not already been initialized. >>> >>> >>> It occurs to me that the method's class should be initialized when >>> invoking the method handle but OpenJDK actually chooses to do the >>> initialization in lookup.findStatic() rather than in mh.invoke() >>> e.g. >>> import java.lang.invoke.*; >>> >>> public class Test_1 { >>> static MethodHandle mh = >>> MethodHandles.lookup().findStatic(Test_2.class, "testMethod", >>> MethodType.methodType(int.class, int.class)); <--- >>> Test_2.class gets initialized and verified. >>> >>> public static void main(String[] args) throws Throwable { >>>
Re: When to initialize the method's class for MethodHandles.Lookup.findStatic()?
Hi Cheng, I'm not sure I understand your point. By class initialization I mean what's specified in the JVMS [1]. Are you concerned with initialization or with verification (which precedes initialization)? Raffaello [1] https://docs.oracle.com/javase/specs/jvms/se17/html/jvms-5.html#jvms-5.5 On 2022-03-18 00:02, Cheng Jin wrote: Hi Raffaello, The code snippet I posted previously was not the original test I verified on Java 17 (which was generated via asmtools to trigger verification) Here's the pretty much the test I used: Test_1.java import java.lang.invoke.*; public class Test_1 { static MethodHandle mh; static { try { mh = MethodHandles.lookup().findStatic(Test_2.class, "testMethod", MethodType.methodType(int[].class)); } catch (NoSuchMethodException | IllegalAccessException e) { e.printStackTrace(); } } public static void main(String[] args) throws Throwable { //Test_1.mh.invoke(); } } Test_2.jasm super public class Test_2 version 52:0 { static Method "":"()V" stack 5 locals 12 { bipush -2; istore 7; iload 7; sipush 997; if_icmplt L127; iconst_0; istore 8; L127: stack_frame_type full; locals_map int, class "[B", class "[B", class "[Ljava/lang/Class;", class "[I", class "[I", class "[I", int, bogus, bogus, float, float; iload 8; istore 8; return; } public Method "":"()V" stack 1 locals 1 { aload_0; invokespecial Method java/lang/Object."":"()V"; return; } static Method testMethod:"()[I" stack 6 locals 1 { getstatic Field testArray:"[I"; areturn; } } // end Class Test_2 Jdk17/bin/java -jar asmtools.jar jasm Test_2.jasm // create the .class file of Test_2 Jdk17/bin/java Test_1 java.lang.IllegalAccessException: no such method: Test_2.testMethod()int[]/invokeStatic at java.base/java.lang.invoke.MemberName.makeAccessException(MemberName.java:972) at java.base/java.lang.invoke.MemberName$Factory.resolveOrFail(MemberName.java:1117) at java.base/java.lang.invoke.MethodHandles$Lookup.resolveOrFail(MethodHandles.java:3649) at java.base/java.lang.invoke.MethodHandles$Lookup.findStatic(MethodHandles.java:2588) at Test_1.(Test_1.java:9) Caused by: java.lang.VerifyError: Inconsistent stackmap frames at branch target 15 Exception Details: Location: Test_2.()V @15: iload Reason: Type top (current frame, locals[0]) is not assignable to integer (stack map, locals[0]) Current Frame: bci: @9 flags: { } locals: { top, top, top, top, top, top, top, integer } stack: { integer, integer } Stackmap Frame: bci: @15 flags: { } locals: { integer, '[B', '[B', '[Ljava/lang/Class;', '[I', '[I', '[I', integer, top, top, float, float } stack: { } Bytecode: 000: 10fe 3607 1507 1103 e5a1 0006 0336 0815 010: 0836 08b1 Stackmap Table: full_frame(@15,{Integer,Object[#11],Object[#11],Object[#20],Object[#5],Object[#5],Object[#5],Integer,Top,Top,Float,Float},{}) at java.base/java.lang.invoke.MethodHandleNatives.resolve(Native Method) at java.base/java.lang.invoke.MemberName$Factory.resolve(MemberName.java:1085) at java.base/java.lang.invoke.MemberName$Factory.resolveOrFail(MemberName.java:1114) ... 3 more Test_2 was intentionally modified to throw VerifyError if initialized. As you see above, mh.inovke() was commented out in Test_1.main() but the Test_2 was still verified in the test. So it is impossible to verify Test_2 if it is not initialized, which only means Test_2 is initialized during the lookup.findstatic rather than mh.invoke(). Best Regards Cheng - Original Message - From: "Cheng Jin" To: "core-libs-dev" Sent: Thursday, March 17, 2022 5:42:57 PM Subject: When to initialize the method's class for MethodHandles.Lookup.findStatic()? Hi there, The document of INVALID URI REMOVED n_java_javase_17_docs_api_java.base_java_lang_invoke_MethodHandles.Loo kup.html-23findStatic-28java.lang.Class-2Cjava.lang.String-2Cjava.lang .invoke.MethodType-29=DwIFaQ=jf_iaSHvJObTbx-siA1ZOg=X90f3XIRXAH8 hbNam6bIUlWfF_qUAezL9ue7M7bFuPQ=Xt-10pHYoPWnY6dByKowR4yeLtEs7kZkKUgt bxKUGvM=dPAMGMxphLLXT9N4ZdukiNvWyvRPAGkcGCBLTy_sm0c= in the Java API is ambiguous in terms of when to initialize the method's class as follows (the same description as in other OpenJDK versions) If the returned method handle is invoked, the method's class will be initialized, if it has not already been initialized. It occurs to me that the method's class should be initialized when invoking the method handle but
RFR: 8283340: Remove support for -Dsun.misc.URLClassPath.disableJarChecking
This change removes support for `-Dsun.misc.URLClassPath.disableJarChecking`. As discussed in the bug the feature doesn't current work, and only ever supported scenarios where a security manager is installed, so it seems safe to remove. - Commit messages: - 8283340: Remove support for -Dsun.misc.URLClassPath.disableJarChecking Changes: https://git.openjdk.java.net/jdk/pull/7861/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7861=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283340 Stats: 25 lines in 1 file changed: 0 ins; 20 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/7861.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7861/head:pull/7861 PR: https://git.openjdk.java.net/jdk/pull/7861
Re: When to initialize the method's class for MethodHandles.Lookup.findStatic()?
Run with -Xlog:class+init=info to see the classes that get initialized and in what order. David On 18/03/2022 5:53 am, Raffaello Giulietti wrote: Hi again, here's code that shows that initialization doesn't happen during lookup but only upon invoking the method handle. (I'm on Java 17.) As long as the 2nd line in main() is commented, you don't see the message "Test_2 initialized", which shows that the lookup doesn't initialize Test_2. When you uncomment the line in main(), the message will appear. So, as advertised, it's the invocation of the method handle that can trigger initialization, not the lookup. HTH Raffaello import java.lang.invoke.*; public class Test_1 { static MethodHandle mh; static { try { mh = MethodHandles.lookup().findStatic(Test_2.class, "testMethod", MethodType.methodType(int.class, int.class)); } catch (NoSuchMethodException | IllegalAccessException e) { e.printStackTrace(); } } public static void main(String[] args) throws Throwable { System.out.println(mh); // System.out.println(Test_1.mh.invoke(0)); } } public class Test_2 { static { System.out.println("Test_2 initialized"); } static int testMethod(int value) { return (value + 1); } } On 2022-03-17 20:38, Raffaello Giulietti wrote: Hi, as far as I can see, the code should not even compile, as there's a static field in Test_1 which is initialized with an expression that throws checked exceptions (findStatic(..)). In addition, it seems to me that there's nothing in your code that reveals whether Test_2 has been initialized during the lookup. How can you tell? Finally, the method handle invocation in Test_1 will throw, as you don't pass any argument to a handle that expects one. Can you perhaps add more details? Greetings Raffaello On 2022-03-17 17:42, Cheng Jin wrote: Hi there, The document of https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#findStatic(java.lang.Class,java.lang.String,java.lang.invoke.MethodType) in the Java API is ambiguous in terms of when to initialize the method's class as follows (the same description as in other OpenJDK versions) If the returned method handle is invoked, the method's class will be initialized, if it has not already been initialized. It occurs to me that the method's class should be initialized when invoking the method handle but OpenJDK actually chooses to do the initialization in lookup.findStatic() rather than in mh.invoke() e.g. import java.lang.invoke.*; public class Test_1 { static MethodHandle mh = MethodHandles.lookup().findStatic(Test_2.class, "testMethod", MethodType.methodType(int.class, int.class)); <--- Test_2.class gets initialized and verified. public static void main(String[] args) throws Throwable { Test_1.mh.invoke(); } } public class Test_2 { static int testMethod(int value) { return (value + 1); } } So there should be more clear explanation what is the correct or expected behaviour at this point and why OpenJDK doesn't comply with the document to delay the initialization of the method's class to mh.invoke(). Best Regards Cheng Jin
RE: When to initialize the method's class for MethodHandles.Lookup.findStatic()?
Hi Raffaello, The code snippet I posted previously was not the original test I verified on Java 17 (which was generated via asmtools to trigger verification) Here's the pretty much the test I used: Test_1.java import java.lang.invoke.*; public class Test_1 { static MethodHandle mh; static { try { mh = MethodHandles.lookup().findStatic(Test_2.class, "testMethod", MethodType.methodType(int[].class)); } catch (NoSuchMethodException | IllegalAccessException e) { e.printStackTrace(); } } public static void main(String[] args) throws Throwable { //Test_1.mh.invoke(); } } Test_2.jasm super public class Test_2 version 52:0 { static Method "":"()V" stack 5 locals 12 { bipush -2; istore 7; iload 7; sipush 997; if_icmplt L127; iconst_0; istore 8; L127: stack_frame_type full; locals_map int, class "[B", class "[B", class "[Ljava/lang/Class;", class "[I", class "[I", class "[I", int, bogus, bogus, float, float; iload 8; istore 8; return; } public Method "":"()V" stack 1 locals 1 { aload_0; invokespecial Method java/lang/Object."":"()V"; return; } static Method testMethod:"()[I" stack 6 locals 1 { getstatic Field testArray:"[I"; areturn; } } // end Class Test_2 Jdk17/bin/java -jar asmtools.jar jasm Test_2.jasm // create the .class file of Test_2 Jdk17/bin/java Test_1 java.lang.IllegalAccessException: no such method: Test_2.testMethod()int[]/invokeStatic at java.base/java.lang.invoke.MemberName.makeAccessException(MemberName.java:972) at java.base/java.lang.invoke.MemberName$Factory.resolveOrFail(MemberName.java:1117) at java.base/java.lang.invoke.MethodHandles$Lookup.resolveOrFail(MethodHandles.java:3649) at java.base/java.lang.invoke.MethodHandles$Lookup.findStatic(MethodHandles.java:2588) at Test_1.(Test_1.java:9) Caused by: java.lang.VerifyError: Inconsistent stackmap frames at branch target 15 Exception Details: Location: Test_2.()V @15: iload Reason: Type top (current frame, locals[0]) is not assignable to integer (stack map, locals[0]) Current Frame: bci: @9 flags: { } locals: { top, top, top, top, top, top, top, integer } stack: { integer, integer } Stackmap Frame: bci: @15 flags: { } locals: { integer, '[B', '[B', '[Ljava/lang/Class;', '[I', '[I', '[I', integer, top, top, float, float } stack: { } Bytecode: 000: 10fe 3607 1507 1103 e5a1 0006 0336 0815 010: 0836 08b1 Stackmap Table: full_frame(@15,{Integer,Object[#11],Object[#11],Object[#20],Object[#5],Object[#5],Object[#5],Integer,Top,Top,Float,Float},{}) at java.base/java.lang.invoke.MethodHandleNatives.resolve(Native Method) at java.base/java.lang.invoke.MemberName$Factory.resolve(MemberName.java:1085) at java.base/java.lang.invoke.MemberName$Factory.resolveOrFail(MemberName.java:1114) ... 3 more Test_2 was intentionally modified to throw VerifyError if initialized. As you see above, mh.inovke() was commented out in Test_1.main() but the Test_2 was still verified in the test. So it is impossible to verify Test_2 if it is not initialized, which only means Test_2 is initialized during the lookup.findstatic rather than mh.invoke(). Best Regards Cheng - Original Message - > From: "Cheng Jin" > To: "core-libs-dev" > Sent: Thursday, March 17, 2022 5:42:57 PM > Subject: When to initialize the method's class for > MethodHandles.Lookup.findStatic()? > Hi there, > > The document of > INVALID URI REMOVED > n_java_javase_17_docs_api_java.base_java_lang_invoke_MethodHandles.Loo > kup.html-23findStatic-28java.lang.Class-2Cjava.lang.String-2Cjava.lang > .invoke.MethodType-29=DwIFaQ=jf_iaSHvJObTbx-siA1ZOg=X90f3XIRXAH8 > hbNam6bIUlWfF_qUAezL9ue7M7bFuPQ=Xt-10pHYoPWnY6dByKowR4yeLtEs7kZkKUgt > bxKUGvM=dPAMGMxphLLXT9N4ZdukiNvWyvRPAGkcGCBLTy_sm0c= > in the Java API is ambiguous in terms of when to initialize the > method's class as follows (the same description as in other OpenJDK > versions) > > If the returned method handle is invoked, the method's class will be > initialized, if it has not already been initialized. > > > It occurs to me that the method's class should be initialized when > invoking the method handle but OpenJDK actually chooses to do the > initialization in > lookup.findStatic() rather than in mh.invoke() e.g. > import java.lang.invoke.*; > > public class Test_1 { >static MethodHandle mh = MethodHandles.lookup().findStatic(Test_2.class, >"testMethod", MethodType.methodType(int.class, int.class)); <--- >Test_2.class gets
Re: RFR: 8283059: Uninitialized warning in check_code.c with GCC 11.2
On Thu, 17 Mar 2022 20:56:34 GMT, Mikael Vidstedt wrote: > Note: this PR replaces the one I messed up earlier. > > Background, from JBS: > > src/java.base/share/native/libverify/check_code.c: In function > 'read_all_code': > src/java.base/share/native/libverify/check_code.c:942:5: error: 'lengths' may > be used uninitialized [-Werror=maybe-uninitialized] > 942 | check_and_push(context, lengths, VM_MALLOC_BLK); > | ^~~ > src/java.base/share/native/libverify/check_code.c:4145:13: note: by argument > 2 of type 'const void *' to 'check_and_push' declared here > 4145 | static void check_and_push(context_type *context, const void *ptr, int > kind) > | ^~ > > Because the second argument of check_and_push is "const void*" GCC assumes > that the malloc:ed data, which has not yet been initialized, will not be/can > not be modified later which in turn suggests it may be used without ever > being initialized. > > The same general issue was addressed in JDK-8266168, presumably for GCC 11.1. > > Details: > > Instead of sprinkling more calloc calls around or using pragmas/gcc > attributes I chose to change the check_and_push function to take a > (non-const) void* argument, and provide a new wrapper function > check_and_push_const which handles the const argument case. For the > (non-const) VM_MALLOC_BKP that means the pointer never needs to go through a > const conversion. > > To avoid having multiple ways of solving the same problem I also chose to > revert the change made in JDK-8266168, reverting the calloc back to a malloc > call. > > Testing: > > tier1 + builds-tier{2,3,4,5} Added hotspot-runtime to the new version of the PR. - PR: https://git.openjdk.java.net/jdk/pull/7859
Re: RFR: 8283277: ISO 4217 Amendment 171 Update
On Thu, 17 Mar 2022 18:10:17 GMT, Naoto Sato wrote: > This is to incorporate the ISO 4217 amendment 171 for Sierra Leonean LEONE > redenomination (removing 3 zeros). Its effective date is 4/1, but I went > ahead as JDK19 won't be released by 4/1. Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7857
Re: RFR: 8283277: ISO 4217 Amendment 171 Update
On Thu, 17 Mar 2022 18:10:17 GMT, Naoto Sato wrote: > This is to incorporate the ISO 4217 amendment 171 for Sierra Leonean LEONE > redenomination (removing 3 zeros). Its effective date is 4/1, but I went > ahead as JDK19 won't be released by 4/1. Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7857
Integrated: 8283325: US_ASCII decoder relies on String.decodeASCII being exhaustive
On Thu, 17 Mar 2022 16:08:42 GMT, Claes Redestad wrote: > Adjust `String.decodeASCII` to deal with the possibility that > `StringCoding.countPositives` can return a value less than the exact number > of leading positive bytes. This is the likely cause of a number of > intermittent but somewhat common errors on aarch64 tests in our CI. This pull request has now been integrated. Changeset: 002e3667 Author:Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/002e3667443d94e2303c875daf72cf1ccbbb0099 Stats: 61 lines in 2 files changed: 61 ins; 0 del; 0 mod 8283325: US_ASCII decoder relies on String.decodeASCII being exhaustive Reviewed-by: rriggs, dcubed - PR: https://git.openjdk.java.net/jdk/pull/7855
Re: RFR: 8283325: US_ASCII decoder relies on String.decodeASCII being exhaustive [v2]
On Thu, 17 Mar 2022 19:47:14 GMT, Claes Redestad wrote: >> Adjust `String.decodeASCII` to deal with the possibility that >> `StringCoding.countPositives` can return a value less than the exact number >> of leading positive bytes. This is the likely cause of a number of >> intermittent but somewhat common errors on aarch64 tests in our CI. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Add comment, remove redundant Arrays.fill Thanks for reviews! - PR: https://git.openjdk.java.net/jdk/pull/7855
RFR: 8283059: Uninitialized warning in check_code.c with GCC 11.2
Note: this PR replaces the one I messed up earlier. Background, from JBS: src/java.base/share/native/libverify/check_code.c: In function 'read_all_code': src/java.base/share/native/libverify/check_code.c:942:5: error: 'lengths' may be used uninitialized [-Werror=maybe-uninitialized] 942 | check_and_push(context, lengths, VM_MALLOC_BLK); | ^~~ src/java.base/share/native/libverify/check_code.c:4145:13: note: by argument 2 of type 'const void *' to 'check_and_push' declared here 4145 | static void check_and_push(context_type *context, const void *ptr, int kind) | ^~ Because the second argument of check_and_push is "const void*" GCC assumes that the malloc:ed data, which has not yet been initialized, will not be/can not be modified later which in turn suggests it may be used without ever being initialized. The same general issue was addressed in JDK-8266168, presumably for GCC 11.1. Details: Instead of sprinkling more calloc calls around or using pragmas/gcc attributes I chose to change the check_and_push function to take a (non-const) void* argument, and provide a new wrapper function check_and_push_const which handles the const argument case. For the (non-const) VM_MALLOC_BKP that means the pointer never needs to go through a const conversion. To avoid having multiple ways of solving the same problem I also chose to revert the change made in JDK-8266168, reverting the calloc back to a malloc call. Testing: tier1 + builds-tier{2,3,4,5} - Commit messages: - 8283059: Uninitialized warning in check_code.c with GCC 11.2 Changes: https://git.openjdk.java.net/jdk/pull/7859/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7859=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283059 Stats: 30 lines in 1 file changed: 9 ins; 0 del; 21 mod Patch: https://git.openjdk.java.net/jdk/pull/7859.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7859/head:pull/7859 PR: https://git.openjdk.java.net/jdk/pull/7859
Re: RFR: 8279488: ProcessBuilder inherits contextClassLoader when spawning a process reaper thread
On Tue, 18 Jan 2022 15:57:58 GMT, Roger Riggs wrote: > The thread factory used to create the process reaper threads unnecessarily > inherits the callers thread context classloader. > The result is retention of the class loader. > > The thread factory used for the pool of process reaper threads is modified to > use an InnocuousThread with a given stacksize. > The test verifies that the process reaper threads have a null context > classloader. Please Review. - PR: https://git.openjdk.java.net/jdk/pull/7131
Re: RFR: 8283325: US_ASCII decoder relies on String.decodeASCII being exhaustive [v2]
On Thu, 17 Mar 2022 19:47:14 GMT, Claes Redestad wrote: >> Adjust `String.decodeASCII` to deal with the possibility that >> `StringCoding.countPositives` can return a value less than the exact number >> of leading positive bytes. This is the likely cause of a number of >> intermittent but somewhat common errors on aarch64 tests in our CI. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Add comment, remove redundant Arrays.fill Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7855
Withdrawn: 8283059: Uninitialized warning in check_code.c with GCC 11.2
On Fri, 11 Mar 2022 23:38:10 GMT, Mikael Vidstedt wrote: > Background, from JBS: > > src/java.base/share/native/libverify/check_code.c: In function > 'read_all_code': > src/java.base/share/native/libverify/check_code.c:942:5: error: 'lengths' may > be used uninitialized [-Werror=maybe-uninitialized] > 942 | check_and_push(context, lengths, VM_MALLOC_BLK); > | ^~~ > src/java.base/share/native/libverify/check_code.c:4145:13: note: by argument > 2 of type 'const void *' to 'check_and_push' declared here > 4145 | static void check_and_push(context_type *context, const void *ptr, > int kind) > | ^~ > > > Because the second argument of check_and_push is "const void*" GCC assumes > that the malloc:ed data, which has not yet been initialized, will not be/can > not be modified later which in turn suggests it may be used without ever > being initialized. > > The same general issue was addressed in > [JDK-8266168](https://bugs.openjdk.java.net/browse/JDK-8266168), presumably > for GCC 11.1. > > > Details: > > Instead of sprinkling more calloc calls around or using pragmas/gcc > attributes I chose to change the check_and_push function to take a > (non-const) void* argument, and provide a new wrapper function > `check_and_push_const` which handles the const argument case. For the > (non-const) VM_MALLOC_BKP that means the pointer never needs to go through a > const conversion. > > To avoid having multiple ways of solving the same problem I also chose to > revert the change made in JDK-8266168, reverting the calloc back to a malloc > call. > > Testing: > > tier1 + builds-tier{2,3,4,5} This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/7794
Re: RFR: 8283059: Uninitialized warning in check_code.c with GCC 11.2 [v2]
On Thu, 17 Mar 2022 20:36:31 GMT, Mikael Vidstedt wrote: >> Background, from JBS: >> >> src/java.base/share/native/libverify/check_code.c: In function >> 'read_all_code': >> src/java.base/share/native/libverify/check_code.c:942:5: error: 'lengths' >> may be used uninitialized [-Werror=maybe-uninitialized] >> 942 | check_and_push(context, lengths, VM_MALLOC_BLK); >> | ^~~ >> src/java.base/share/native/libverify/check_code.c:4145:13: note: by argument >> 2 of type 'const void *' to 'check_and_push' declared here >> 4145 | static void check_and_push(context_type *context, const void *ptr, >> int kind) >> | ^~ >> >> >> Because the second argument of check_and_push is "const void*" GCC assumes >> that the malloc:ed data, which has not yet been initialized, will not be/can >> not be modified later which in turn suggests it may be used without ever >> being initialized. >> >> The same general issue was addressed in >> [JDK-8266168](https://bugs.openjdk.java.net/browse/JDK-8266168), presumably >> for GCC 11.1. >> >> >> Details: >> >> Instead of sprinkling more calloc calls around or using pragmas/gcc >> attributes I chose to change the check_and_push function to take a >> (non-const) void* argument, and provide a new wrapper function >> `check_and_push_const` which handles the const argument case. For the >> (non-const) VM_MALLOC_BKP that means the pointer never needs to go through a >> const conversion. >> >> To avoid having multiple ways of solving the same problem I also chose to >> revert the change made in JDK-8266168, reverting the calloc back to a malloc >> call. >> >> Testing: >> >> tier1 + builds-tier{2,3,4,5} > > Mikael Vidstedt has updated the pull request incrementally with 50 additional > commits since the last revision: > > - Spell out check_and_push_malloc_block > - Fix code > - 8282773: Refactor parsing of integer VM options > >Reviewed-by: dholmes, kbarrett > - 8283274: Improve @jvms usage in java.base > >Reviewed-by: iris > - 8283188: Build time regression caused by JDK-8278917 > >Reviewed-by: kbarrett, tschatzl > - 8283320: Error message for Windows libraries always points to > --with-msvcr-dll no matter the actual file name > >Reviewed-by: erikj, ihse > - 8283056: show abstract machine code in hs-err for all VM crashes > >Reviewed-by: thartmann, dholmes > - 8282727: Parallel: Remove PSPromotionManager::_totally_drain > >Reviewed-by: tschatzl, kbarrett > - 8281146: Replace StringCoding.hasNegatives with countPositives > >Co-authored-by: Lutz Schmidt >Co-authored-by: Martin Doerr >Reviewed-by: kvn, lucy, rriggs > - 8282602: Refactor awt classes javadoc to use @throws instead of @exception > >Reviewed-by: aivanov, prr > - ... and 40 more: > https://git.openjdk.java.net/jdk/compare/baf72097...eb04d78a Um.. I think I managed to mess up this PR when I brought in latest master *and* tried it both with the current and new version of gcc. Sorry about that. I'll withdraw this one and open a new PR - PR: https://git.openjdk.java.net/jdk/pull/7794
Re: RFR: 8283059: Uninitialized warning in check_code.c with GCC 11.2 [v2]
> Background, from JBS: > > src/java.base/share/native/libverify/check_code.c: In function > 'read_all_code': > src/java.base/share/native/libverify/check_code.c:942:5: error: 'lengths' may > be used uninitialized [-Werror=maybe-uninitialized] > 942 | check_and_push(context, lengths, VM_MALLOC_BLK); > | ^~~ > src/java.base/share/native/libverify/check_code.c:4145:13: note: by argument > 2 of type 'const void *' to 'check_and_push' declared here > 4145 | static void check_and_push(context_type *context, const void *ptr, > int kind) > | ^~ > > > Because the second argument of check_and_push is "const void*" GCC assumes > that the malloc:ed data, which has not yet been initialized, will not be/can > not be modified later which in turn suggests it may be used without ever > being initialized. > > The same general issue was addressed in > [JDK-8266168](https://bugs.openjdk.java.net/browse/JDK-8266168), presumably > for GCC 11.1. > > > Details: > > Instead of sprinkling more calloc calls around or using pragmas/gcc > attributes I chose to change the check_and_push function to take a > (non-const) void* argument, and provide a new wrapper function > `check_and_push_const` which handles the const argument case. For the > (non-const) VM_MALLOC_BKP that means the pointer never needs to go through a > const conversion. > > To avoid having multiple ways of solving the same problem I also chose to > revert the change made in JDK-8266168, reverting the calloc back to a malloc > call. > > Testing: > > tier1 + builds-tier{2,3,4,5} Mikael Vidstedt has updated the pull request incrementally with 50 additional commits since the last revision: - Spell out check_and_push_malloc_block - Fix code - 8282773: Refactor parsing of integer VM options Reviewed-by: dholmes, kbarrett - 8283274: Improve @jvms usage in java.base Reviewed-by: iris - 8283188: Build time regression caused by JDK-8278917 Reviewed-by: kbarrett, tschatzl - 8283320: Error message for Windows libraries always points to --with-msvcr-dll no matter the actual file name Reviewed-by: erikj, ihse - 8283056: show abstract machine code in hs-err for all VM crashes Reviewed-by: thartmann, dholmes - 8282727: Parallel: Remove PSPromotionManager::_totally_drain Reviewed-by: tschatzl, kbarrett - 8281146: Replace StringCoding.hasNegatives with countPositives Co-authored-by: Lutz Schmidt Co-authored-by: Martin Doerr Reviewed-by: kvn, lucy, rriggs - 8282602: Refactor awt classes javadoc to use @throws instead of @exception Reviewed-by: aivanov, prr - ... and 40 more: https://git.openjdk.java.net/jdk/compare/baf72097...eb04d78a - Changes: - all: https://git.openjdk.java.net/jdk/pull/7794/files - new: https://git.openjdk.java.net/jdk/pull/7794/files/baf72097..eb04d78a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7794=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7794=00-01 Stats: 5146 lines in 353 files changed: 2567 ins; 553 del; 2026 mod Patch: https://git.openjdk.java.net/jdk/pull/7794.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7794/head:pull/7794 PR: https://git.openjdk.java.net/jdk/pull/7794
Re: RFR: 8283059: Uninitialized warning in check_code.c with GCC 11.2
On Thu, 17 Mar 2022 08:44:57 GMT, Kim Barrett wrote: >> src/java.base/share/native/libverify/check_code.c line 472: >> >>> 470: >>> 471: static void check_and_push(context_type *context, void *ptr, int kind); >>> 472: static void check_and_push_const(context_type *context, const void >>> *ptr, int kind); >> >> This really needs a comment explaining why two functions are needed. > > I think it would be clearer to drop the kind argument and have two functions > check_and_push_vm_string(context_type*, const char*) > check_and_push_malloc_block(context_type*, void*) > with a shared helper that is given the appropriate kind. I had that at some point and then dropped it. I don't know why and it does seem like the better option so I'll try it again. - PR: https://git.openjdk.java.net/jdk/pull/7794
Re: RFR: 8283325: US_ASCII decoder relies on String.decodeASCII being exhaustive [v2]
On Thu, 17 Mar 2022 19:47:14 GMT, Claes Redestad wrote: >> Adjust `String.decodeASCII` to deal with the possibility that >> `StringCoding.countPositives` can return a value less than the exact number >> of leading positive bytes. This is the likely cause of a number of >> intermittent but somewhat common errors on aarch64 tests in our CI. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Add comment, remove redundant Arrays.fill Thumbs up. - Marked as reviewed by dcubed (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7855
Re: When to initialize the method's class for MethodHandles.Lookup.findStatic()?
Hi again, here's code that shows that initialization doesn't happen during lookup but only upon invoking the method handle. (I'm on Java 17.) As long as the 2nd line in main() is commented, you don't see the message "Test_2 initialized", which shows that the lookup doesn't initialize Test_2. When you uncomment the line in main(), the message will appear. So, as advertised, it's the invocation of the method handle that can trigger initialization, not the lookup. HTH Raffaello import java.lang.invoke.*; public class Test_1 { static MethodHandle mh; static { try { mh = MethodHandles.lookup().findStatic(Test_2.class, "testMethod", MethodType.methodType(int.class, int.class)); } catch (NoSuchMethodException | IllegalAccessException e) { e.printStackTrace(); } } public static void main(String[] args) throws Throwable { System.out.println(mh); // System.out.println(Test_1.mh.invoke(0)); } } public class Test_2 { static { System.out.println("Test_2 initialized"); } static int testMethod(int value) { return (value + 1); } } On 2022-03-17 20:38, Raffaello Giulietti wrote: Hi, as far as I can see, the code should not even compile, as there's a static field in Test_1 which is initialized with an expression that throws checked exceptions (findStatic(..)). In addition, it seems to me that there's nothing in your code that reveals whether Test_2 has been initialized during the lookup. How can you tell? Finally, the method handle invocation in Test_1 will throw, as you don't pass any argument to a handle that expects one. Can you perhaps add more details? Greetings Raffaello On 2022-03-17 17:42, Cheng Jin wrote: Hi there, The document of https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#findStatic(java.lang.Class,java.lang.String,java.lang.invoke.MethodType) in the Java API is ambiguous in terms of when to initialize the method's class as follows (the same description as in other OpenJDK versions) If the returned method handle is invoked, the method's class will be initialized, if it has not already been initialized. It occurs to me that the method's class should be initialized when invoking the method handle but OpenJDK actually chooses to do the initialization in lookup.findStatic() rather than in mh.invoke() e.g. import java.lang.invoke.*; public class Test_1 { static MethodHandle mh = MethodHandles.lookup().findStatic(Test_2.class, "testMethod", MethodType.methodType(int.class, int.class)); <--- Test_2.class gets initialized and verified. public static void main(String[] args) throws Throwable { Test_1.mh.invoke(); } } public class Test_2 { static int testMethod(int value) { return (value + 1); } } So there should be more clear explanation what is the correct or expected behaviour at this point and why OpenJDK doesn't comply with the document to delay the initialization of the method's class to mh.invoke(). Best Regards Cheng Jin
Re: RFR: 8283325: US_ASCII decoder relies on String.decodeASCII being exhaustive [v2]
> Adjust `String.decodeASCII` to deal with the possibility that > `StringCoding.countPositives` can return a value less than the exact number > of leading positive bytes. This is the likely cause of a number of > intermittent but somewhat common errors on aarch64 tests in our CI. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Add comment, remove redundant Arrays.fill - Changes: - all: https://git.openjdk.java.net/jdk/pull/7855/files - new: https://git.openjdk.java.net/jdk/pull/7855/files/aa0470cf..ef9fbaa7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7855=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7855=00-01 Stats: 6 lines in 1 file changed: 5 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7855.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7855/head:pull/7855 PR: https://git.openjdk.java.net/jdk/pull/7855
Re: When to initialize the method's class for MethodHandles.Lookup.findStatic()?
Hi, as far as I can see, the code should not even compile, as there's a static field in Test_1 which is initialized with an expression that throws checked exceptions (findStatic(..)). In addition, it seems to me that there's nothing in your code that reveals whether Test_2 has been initialized during the lookup. How can you tell? Finally, the method handle invocation in Test_1 will throw, as you don't pass any argument to a handle that expects one. Can you perhaps add more details? Greetings Raffaello On 2022-03-17 17:42, Cheng Jin wrote: Hi there, The document of https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#findStatic(java.lang.Class,java.lang.String,java.lang.invoke.MethodType) in the Java API is ambiguous in terms of when to initialize the method's class as follows (the same description as in other OpenJDK versions) If the returned method handle is invoked, the method's class will be initialized, if it has not already been initialized. It occurs to me that the method's class should be initialized when invoking the method handle but OpenJDK actually chooses to do the initialization in lookup.findStatic() rather than in mh.invoke() e.g. import java.lang.invoke.*; public class Test_1 { static MethodHandle mh = MethodHandles.lookup().findStatic(Test_2.class, "testMethod", MethodType.methodType(int.class, int.class)); <--- Test_2.class gets initialized and verified. public static void main(String[] args) throws Throwable { Test_1.mh.invoke(); } } public class Test_2 { static int testMethod(int value) { return (value + 1); } } So there should be more clear explanation what is the correct or expected behaviour at this point and why OpenJDK doesn't comply with the document to delay the initialization of the method's class to mh.invoke(). Best Regards Cheng Jin
RFR: 8283277: ISO 4217 Amendment 171 Update
This is to incorporate the ISO 4217 amendment 171 for Sierra Leonean LEONE redenomination (removing 3 zeros). Its effective date is 4/1, but I went ahead as JDK19 won't be released by 4/1. - Commit messages: - 8283277: ISO 4217 Amendment 171 Update Changes: https://git.openjdk.java.net/jdk/pull/7857/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7857=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283277 Stats: 17 lines in 6 files changed: 3 ins; 0 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/7857.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7857/head:pull/7857 PR: https://git.openjdk.java.net/jdk/pull/7857
Re: RFR: 8283325: US_ASCII decoder relies on String.decodeASCII being exhaustive
On Thu, 17 Mar 2022 16:08:42 GMT, Claes Redestad wrote: > Adjust `String.decodeASCII` to deal with the possibility that > `StringCoding.countPositives` can return a value less than the exact number > of leading positive bytes. This is the likely cause of a number of > intermittent but somewhat common errors on aarch64 tests in our CI. @cl4es - are you planning to integrate your fix today? - PR: https://git.openjdk.java.net/jdk/pull/7855
Re: RFR: 8278794: Infinite loop in DeflaterOutputStream.finish() [v3]
On Thu, 17 Mar 2022 16:56:15 GMT, Ravi Reddy wrote: >> Hi All, >> >> This review request contains fix for infinite loop issue in >> DeflaterOutputStream.finish() in an exception scenario. >> 1. The issue is with 'finished' flag not getting set to correct value when >> there is an IOException in >> DeflaterOutputStream.finish() which will result in Infinite loops for >> next write operations on the same deflater. >> 2. Tighten the condition(to close deflater) in ZipOutputStream using an >> already existing 'finish' flag in Deflater class. >> 3. Added Inflater exception scenarios also to the test case, renaming test >> case to CloseInflaterDeflaterTest.java from CloseDeflaterTest.java >> >> Thanks, >> Ravi > > Ravi Reddy has updated the pull request incrementally with one additional > commit since the last revision: > > Modified the format of comments across the test case and also done clean up > of test case by using less data providers Hi Ravi, Thank you for cleaning up the comments. Much appreciated! - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7804
Re: Making enum hashcodes consistent across JVM's
Hi, Our use case is that we have different instances of an application running in a cloud environment and a precomputed hash code is used downstream to see if object instances generated in different application instances are the same or not. An enum was later added to our model which caused the weird behavior that instances generated by the same application instance had the same hash, but instances generated by another application instance did not. So it was weird to me that all of the other fields which either consisted of Strings, primitives, collections or elements ending in Strings and primitives generated the same hash code each time between applications and executions and enums did not. I understand now that relying on a specific hashcode implementation is foolish, but because String worked this way I thought it would be nice if enum worked this way too. But I realize now that the way String does it, is in fact wrong (but part of unchangeable history) instead of the other way around. Kind regards, Dave On Thu, 2022-03-17 at 15:59 +0100, Raffaello Giulietti wrote: > Hi, > > specifying a fixed hashCode() algorithm has the important drawback > that > it prevents evolution. > > For example, as you note, String has a strictly specified algorithm > [1] > that was perhaps a good one in the early days of Java (around 1995) > but > has shown its deficiencies over the years. In particular, it's easy > to > construct collisions, which might impact the performance of popular > and > heavily used hash based data structures, like HashMap and HashSet, > under > some forms of cyberattack. > > Unfortunately, while better String hash algorithms have been > proposed, > they cannot be adopted, as too much code out there relies on the > specific details of String::hashCode. (Even bytecode for switches > over > String relies on that particular spec.) It's far too late now to > change > the spec, even for the better. > > In retrospect and IMO, fixing String::hashCode in the spec was a > (understandable) mistake. > > > More generally, client code that relies on details of any popular > library hashCode() implementation has a dangerous dependency on such > details and impairs evolution of the library itself (as the example > with > String shows). > > To leave the door open for better implementations proposed in some > future, it is best to avoid specifying the details of hashCode(), as > done in Record::hashCode [2] ("Implementation Requirements"). After > all, > the general contract reads [3]: "This integer [the hash] need not > remain > consistent from one execution of an application to another execution > of > the same application." > > > But what is the use case you have for which you would like a firmly > specified Enum::hashCode? > > > Greetings > Raffaello > > > > [1] > https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/String.html#hashCode() > [2] > https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/Record.html#hashCode() > [3] > https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/Object.html#hashCode() > > > > On 2022-03-17 13:49, dfranken@gmail.com wrote: > > Dear all, > > > > Currently enums do not have a well-defined hashCode() > > implementation so > > they defer to Object.hashCode() which just uses an internal > > mechanism > > to determine the hashcode, likely based on the object's place in > > the > > heap. > > > > This may confuse a lot of developers as other classes such as > > String do > > have a well-defined hashcode which is consistent across multiple > > JVM's. > > > > Could it be a good idea to make enums' hashCode() method more > > reliably > > and predictable? For instance by using the hashcode of the name of > > its > > value? > > > > For example: > > > > class MyEnum { A, B } > > -> A.hashCode() == A.name().hashCode() > > > > Kind regards, > > > > Dave Franken
Re: When to initialize the method's class for MethodHandles.Lookup.findStatic()?
- Original Message - > From: "Cheng Jin" > To: "core-libs-dev" > Sent: Thursday, March 17, 2022 5:42:57 PM > Subject: When to initialize the method's class for > MethodHandles.Lookup.findStatic()? > Hi there, > > The document of > https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#findStatic(java.lang.Class,java.lang.String,java.lang.invoke.MethodType) > in the Java API is ambiguous in terms of when to initialize the method's class > as follows (the same description as in other OpenJDK versions) > > If the returned method handle is invoked, the method's class will be > initialized, if it has not already been initialized. > > > It occurs to me that the method's class should be initialized when invoking > the > method handle but OpenJDK actually chooses to do the initialization in > lookup.findStatic() rather than in mh.invoke() > e.g. > import java.lang.invoke.*; > > public class Test_1 { >static MethodHandle mh = MethodHandles.lookup().findStatic(Test_2.class, >"testMethod", MethodType.methodType(int.class, int.class)); <--- >Test_2.class gets initialized and verified. > >public static void main(String[] args) throws Throwable { >Test_1.mh.invoke(); >} > } > > public class Test_2 { >static int testMethod(int value) { return (value + 1); } > } > > So there should be more clear explanation what is the correct or expected > behaviour at this point and why OpenJDK doesn't comply with the document to > delay the initialization of the method's class to mh.invoke(). Hi, if by initialization you mean running the static block, then it's a bug. As far as i remember, the JVMS spec cleanly separate the Linking exceptions from the Runtime exceptions. https://docs.oracle.com/javase/specs/jvms/se17/html/jvms-6.html#jvms-6.5.invokestatic The linking exceptions occurs when calling findStatic() while the runtime exceptions will appear at runtime when calling invokeExact()/invoke(). > > Best Regards > Cheng Jin regards, Rémi
Re: RFR: 8283287: ClassLoader.c cleanups [v2]
On Thu, 17 Mar 2022 16:08:11 GMT, Tyler Steele wrote: >> As mentioned in the issue, I'd like to perform the following tidying on >> ClassLoader.c >> >> - Alphabetize includes. >> - Replace 'if (ptr == 0)' with 'if (ptr == NULL)'. >> - Replace 'return 0' with 'return NULL'. > > Tyler Steele has updated the pull request incrementally with one additional > commit since the last revision: > > removes unneeded errno.h All good. Cheers, Thomas - Marked as reviewed by stuefe (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7846
Re: RFR: 8283237: CallSite should be a sealed class [v2]
On Thu, 17 Mar 2022 08:07:23 GMT, liach wrote: >> Change `CallSite` to a sealed class, as `CallSite` is an abstract class >> which does not allow direct subclassing by users per its documentation. >> Since I don't have a JBS account, I posted the content for the CSR in a >> GitHub Gist at https://gist.github.com/150d5aa7f8b13a4deddf95969ad39d73 and >> wish someone can submit a CSR for me. > > liach has updated the pull request incrementally with one additional commit > since the last revision: > > Swap mutable and volatile in the permits list @jddarcy Could you help me file a CSR for this change? - PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: 8278794: Infinite loop in DeflaterOutputStream.finish() [v3]
> Hi All, > > This review request contains fix for infinite loop issue in > DeflaterOutputStream.finish() in an exception scenario. > 1. The issue is with 'finished' flag not getting set to correct value when > there is an IOException in > DeflaterOutputStream.finish() which will result in Infinite loops for > next write operations on the same deflater. > 2. Tighten the condition(to close deflater) in ZipOutputStream using an > already existing 'finish' flag in Deflater class. > 3. Added Inflater exception scenarios also to the test case, renaming test > case to CloseInflaterDeflaterTest.java from CloseDeflaterTest.java > > Thanks, > Ravi Ravi Reddy has updated the pull request incrementally with one additional commit since the last revision: Modified the format of comments across the test case and also done clean up of test case by using less data providers - Changes: - all: https://git.openjdk.java.net/jdk/pull/7804/files - new: https://git.openjdk.java.net/jdk/pull/7804/files/c50aa1bd..ac867083 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7804=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7804=01-02 Stats: 94 lines in 1 file changed: 32 ins; 20 del; 42 mod Patch: https://git.openjdk.java.net/jdk/pull/7804.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7804/head:pull/7804 PR: https://git.openjdk.java.net/jdk/pull/7804
Re: RFR: 8278794: Infinite loop in DeflaterOutputStream.finish() [v2]
On Wed, 16 Mar 2022 16:34:37 GMT, Ravi Reddy wrote: >> Hi All, >> >> This review request contains fix for infinite loop issue in >> DeflaterOutputStream.finish() in an exception scenario. >> 1. The issue is with 'finished' flag not getting set to correct value when >> there is an IOException in >> DeflaterOutputStream.finish() which will result in Infinite loops for >> next write operations on the same deflater. >> 2. Tighten the condition(to close deflater) in ZipOutputStream using an >> already existing 'finish' flag in Deflater class. >> 3. Added Inflater exception scenarios also to the test case, renaming test >> case to CloseInflaterDeflaterTest.java from CloseDeflaterTest.java >> >> Thanks, >> Ravi > > Ravi Reddy has updated the pull request incrementally with one additional > commit since the last revision: > > Modified write(byte []b) to write(byte[] b) to maintain same signature > across the test case > Thanks Lance for the review , In the latest commit , I have formatted the comments in the test case. - PR: https://git.openjdk.java.net/jdk/pull/7804
Re: RFR: 8283225: ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 (aix) [v4]
On Thu, 17 Mar 2022 16:23:14 GMT, Tyler Steele wrote: >> As described in the linked issue, NullClassBytesTest fails due an >> OutOfMemoryError produced on AIX when the test calls defineClass with a byte >> array of size of 0. The native implementation of defineClass then calls >> malloc with a size of 0. On AIX malloc(0) returns NULL, while on other >> platforms it return a valid address. When NULL is produced by malloc for >> this reason, ClassLoader.c incorrectly interprets this as a failure due to a >> lack of memory. >> >> ~~This PR modifies ClassLoader.c to produce an OutOfMemoryError only when >> `errno == ENOMEM` and to produce a ClassFormatError with the message >> "ClassLoader internal allocation failure" in all other cases (in which >> malloc returns NULL).~~ [edit: The above no longer describes the PR's >> proposed fix. See discussion below] >> >> In addition, I performed some minor tidy-up work in ClassLoader.c by >> changing instances of `return 0` to `return NULL`, and `if (some_ptr == 0)` >> to `if (some_ptr == NULL)`. This was done to improve the clarity of the code >> in ClassLoader.c, but didn't feel worthy of opening a separate issue. >> >> ### Alternatives >> >> It would be possible to address this failure by modifying the test to accept >> the OutOfMemoryError on AIX. I thought it was a better solution to modify >> ClassLoader.c to produce an OutOfMemoryError only when the system is >> actually out of memory. >> >> ### Testing >> >> This change has been tested on AIX and Linux/x86. > > Tyler Steele has updated the pull request incrementally with one additional > commit since the last revision: > > Improve comment > > - Reword to avoid double use of malloc(X) > - Remove bug id Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283225: ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 (aix) [v3]
On Thu, 17 Mar 2022 14:48:09 GMT, Roger Riggs wrote: >> src/java.base/share/native/libjava/ClassLoader.c line 102: >> >>> 100: } >>> 101: >>> 102: // On malloc(0), implementators of malloc(3) have the choice to >>> return either >> >> It is confusing to mix `malloc(0)`, where you are passing an argument zero >> to malloc, with `malloc(3)` which actually means the definition of malloc as >> per the man page in section 3. >> >> Given this is only an issue on AIX the comment can simply say: >> >> `// On AIX malloc(0) returns NULL which looks like an out-of-memory >> condition; so adjust it to malloc(1)`. > > I would omit the bug number reference, they get stale and do not age well, > cluttering up the source. > Git blame can be used to find the origin of the comment if needed. I changed to the streamlined comment and removed the bug id. Thanks for your suggestions. - PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283225: ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 (aix) [v3]
On Thu, 17 Mar 2022 07:46:21 GMT, Thomas Stuefe wrote: >> src/java.base/share/native/libjava/ClassLoader.c line 106: >> >>> 104: // we chose the latter. (see 8283225) >>> 105: #ifdef _AIX >>> 106: body = (jbyte *)malloc(length == 0 ? 1 : length); >> >> Using AIX_ONLY this can be simplified: >> >> `body = (jbyte *)malloc(length AIX_ONLY( == 0 ? 1 : length));` > >> Using AIX_ONLY this can be simplified: >> >> `body = (jbyte *)malloc(length AIX_ONLY( == 0 ? 1 : length));` > > This is jdk, not hotspot. Do we have AIX_ONLY in the JDK? As Thomas mentioned, this does not seem to work here. - PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283325: US_ASCII decoder relies on String.decodeASCII being exhaustive
On Thu, 17 Mar 2022 16:08:42 GMT, Claes Redestad wrote: > Adjust `String.decodeASCII` to deal with the possibility that > `StringCoding.countPositives` can return a value less than the exact number > of leading positive bytes. This is the likely cause of a number of > intermittent but somewhat common errors on aarch64 tests in our CI. In the test, it might be worth a comment that the 100_000 repeats is enough to warm up the compiler and use the intrinsic. - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7855
When to initialize the method's class for MethodHandles.Lookup.findStatic()?
Hi there, The document of https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#findStatic(java.lang.Class,java.lang.String,java.lang.invoke.MethodType) in the Java API is ambiguous in terms of when to initialize the method's class as follows (the same description as in other OpenJDK versions) If the returned method handle is invoked, the method's class will be initialized, if it has not already been initialized. It occurs to me that the method's class should be initialized when invoking the method handle but OpenJDK actually chooses to do the initialization in lookup.findStatic() rather than in mh.invoke() e.g. import java.lang.invoke.*; public class Test_1 { static MethodHandle mh = MethodHandles.lookup().findStatic(Test_2.class, "testMethod", MethodType.methodType(int.class, int.class)); <--- Test_2.class gets initialized and verified. public static void main(String[] args) throws Throwable { Test_1.mh.invoke(); } } public class Test_2 { static int testMethod(int value) { return (value + 1); } } So there should be more clear explanation what is the correct or expected behaviour at this point and why OpenJDK doesn't comply with the document to delay the initialization of the method's class to mh.invoke(). Best Regards Cheng Jin
Integrated: JDK-8283274: Improve @jvms usage in java.base
On Wed, 16 Mar 2022 16:57:12 GMT, Joe Darcy wrote: > As was done for JLS references under JDK-8283234, now the analogous update > for JVMS references. This pull request has now been integrated. Changeset: 5ef1990d Author:Joe Darcy URL: https://git.openjdk.java.net/jdk/commit/5ef1990d6ce35a85c86689badba465b6c8f9f4a1 Stats: 12 lines in 6 files changed: 0 ins; 0 del; 12 mod 8283274: Improve @jvms usage in java.base Reviewed-by: iris - PR: https://git.openjdk.java.net/jdk/pull/7844
Re: RFR: 8283225: ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 (aix) [v4]
> As described in the linked issue, NullClassBytesTest fails due an > OutOfMemoryError produced on AIX when the test calls defineClass with a byte > array of size of 0. The native implementation of defineClass then calls > malloc with a size of 0. On AIX malloc(0) returns NULL, while on other > platforms it return a valid address. When NULL is produced by malloc for this > reason, ClassLoader.c incorrectly interprets this as a failure due to a lack > of memory. > > ~~This PR modifies ClassLoader.c to produce an OutOfMemoryError only when > `errno == ENOMEM` and to produce a ClassFormatError with the message > "ClassLoader internal allocation failure" in all other cases (in which malloc > returns NULL).~~ [edit: The above no longer describes the PR's proposed fix. > See discussion below] > > In addition, I performed some minor tidy-up work in ClassLoader.c by changing > instances of `return 0` to `return NULL`, and `if (some_ptr == 0)` to `if > (some_ptr == NULL)`. This was done to improve the clarity of the code in > ClassLoader.c, but didn't feel worthy of opening a separate issue. > > ### Alternatives > > It would be possible to address this failure by modifying the test to accept > the OutOfMemoryError on AIX. I thought it was a better solution to modify > ClassLoader.c to produce an OutOfMemoryError only when the system is actually > out of memory. > > ### Testing > > This change has been tested on AIX and Linux/x86. Tyler Steele has updated the pull request incrementally with one additional commit since the last revision: Improve comment - Reword to avoid double use of malloc(X) - Remove bug id - Changes: - all: https://git.openjdk.java.net/jdk/pull/7829/files - new: https://git.openjdk.java.net/jdk/pull/7829/files/f05c8d85..f0e2800e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7829=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7829=02-03 Stats: 6 lines in 1 file changed: 0 ins; 4 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7829.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7829/head:pull/7829 PR: https://git.openjdk.java.net/jdk/pull/7829
RFR: 8283325: US_ASCII decoder relies on String.decodeASCII being exhaustive
Adjust `String.decodeASCII` to deal with the possibility that `StringCoding.countPositives` can return a value less than the exact number of leading positive bytes. This is the likely cause of a number of intermittent but somewhat common errors on aarch64 tests in our CI. - Commit messages: - Exhaustively consume ASCII bytes in String.decodeASCII Changes: https://git.openjdk.java.net/jdk/pull/7855/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7855=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283325 Stats: 57 lines in 2 files changed: 57 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7855.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7855/head:pull/7855 PR: https://git.openjdk.java.net/jdk/pull/7855
Re: RFR: 8257733: Move module-specific data from make to respective module [v9]
On Thu, 17 Mar 2022 00:12:38 GMT, Magnus Ihse Bursie wrote: >> A lot (but not all) of the data in make/data is tied to a specific module. >> For instance, the publicsuffixlist is used by java.base, and fontconfig by >> java.desktop. (A few directories, like mainmanifest, is *actually* used by >> make for the whole build.) >> >> These data files should move to the module they belong to. The are, after >> all, "source code" for that module that is "compiler" into resulting >> deliverables, for that module. (But the "source code" language is not Java >> or C, but typically a highly domain specific language or data format, and >> the "compilation" is, often, a specialized transformation.) >> >> This misplacement of the data directory is most visible at code review time. >> When such data is changed, most of the time build-dev (or the new build >> label) is involved, even though this has nothing to do with the build. While >> this is annoying, a worse problem is if the actual team that needs to review >> the patch (i.e., the team owning the module) is missed in the review. >> >> ### Modules reviewed >> >> - [x] java.base >> - [x] java.desktop >> - [x] jdk.compiler >> - [x] java.se > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Fix typos LGTM. Thanks for the change, Magnus! - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8283287: ClassLoader.c cleanups [v2]
> As mentioned in the issue, I'd like to perform the following tidying on > ClassLoader.c > > - Alphabetize includes. > - Replace 'if (ptr == 0)' with 'if (ptr == NULL)'. > - Replace 'return 0' with 'return NULL'. Tyler Steele has updated the pull request incrementally with one additional commit since the last revision: removes unneeded errno.h - Changes: - all: https://git.openjdk.java.net/jdk/pull/7846/files - new: https://git.openjdk.java.net/jdk/pull/7846/files/57a92415..920e76fb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7846=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7846=00-01 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7846.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7846/head:pull/7846 PR: https://git.openjdk.java.net/jdk/pull/7846
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v19]
On Wed, 16 Mar 2022 14:54:41 GMT, Yasser Bazzi wrote: >> Hi, could i get a review on this implementation proposed by Stuart Marks, i >> decided to use the >> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html >> interface to create the default method `asRandom()` that wraps around the >> newer algorithms to be used on classes that do not accept the new interface. >> >> Some things to note as proposed by the bug report, the protected method >> next(int bits) is not overrided and setSeed() method if left blank up to >> discussion on what to do with it. >> >> Small test done on >> https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4 > > Yasser Bazzi has updated the pull request incrementally with one additional > commit since the last revision: > > add since in javadoc Should RandomWrapper implement toString to include toString representation of the RandomGenerator? I would think that implementations of equals and hashCode are not needed but toString might be useful. - PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8283287: ClassLoader.c cleanups
On Thu, 17 Mar 2022 10:46:35 GMT, Thomas Stuefe wrote: >> As mentioned in the issue, I'd like to perform the following tidying on >> ClassLoader.c >> >> - Alphabetize includes. >> - Replace 'if (ptr == 0)' with 'if (ptr == NULL)'. >> - Replace 'return 0' with 'return NULL'. > > src/java.base/share/native/libjava/ClassLoader.c line 27: > >> 25: >> 26: #include >> 27: #include > > Whats errno.h for? Thanks for catching that. It was for [the other PR](https://github.com/openjdk/jdk/pull/7829). - PR: https://git.openjdk.java.net/jdk/pull/7846
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v19]
On Wed, 16 Mar 2022 14:54:41 GMT, Yasser Bazzi wrote: >> Hi, could i get a review on this implementation proposed by Stuart Marks, i >> decided to use the >> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html >> interface to create the default method `asRandom()` that wraps around the >> newer algorithms to be used on classes that do not accept the new interface. >> >> Some things to note as proposed by the bug report, the protected method >> next(int bits) is not overrided and setSeed() method if left blank up to >> discussion on what to do with it. >> >> Small test done on >> https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4 > > Yasser Bazzi has updated the pull request incrementally with one additional > commit since the last revision: > > add since in javadoc FYI in the CSR review, Joe Darcy points out: > Okay; I've looked over this CSR in more detail and think some additional > supporting changes in java.util.Random should be made to address some > existing shortcoming in the spec there. > > In particular, I think Random.setSeed's spec should be changed to have a > general contract that it is an optional operation, i.e. may throw > UnsupportedOperationException as ThreadLocalRandom.setSeed already does. The > particulars of Random.setSeed should be moved into an implSpec, which would > only apply to that non-overridden method. > > (More generally, java.util.Random would likely benefit from a pass to use > implNote, implSpec, etc., but I think only setSeed needs to be adjusted for > the purposes of this CSR.) > > Moving back to Provisional. So need an update to `Random.setSeed`'s API documentation and update the CSR correspondingly. - PR: https://git.openjdk.java.net/jdk/pull/7001
Re: Making enum hashcodes consistent across JVM's
Hi, specifying a fixed hashCode() algorithm has the important drawback that it prevents evolution. For example, as you note, String has a strictly specified algorithm [1] that was perhaps a good one in the early days of Java (around 1995) but has shown its deficiencies over the years. In particular, it's easy to construct collisions, which might impact the performance of popular and heavily used hash based data structures, like HashMap and HashSet, under some forms of cyberattack. Unfortunately, while better String hash algorithms have been proposed, they cannot be adopted, as too much code out there relies on the specific details of String::hashCode. (Even bytecode for switches over String relies on that particular spec.) It's far too late now to change the spec, even for the better. In retrospect and IMO, fixing String::hashCode in the spec was a (understandable) mistake. More generally, client code that relies on details of any popular library hashCode() implementation has a dangerous dependency on such details and impairs evolution of the library itself (as the example with String shows). To leave the door open for better implementations proposed in some future, it is best to avoid specifying the details of hashCode(), as done in Record::hashCode [2] ("Implementation Requirements"). After all, the general contract reads [3]: "This integer [the hash] need not remain consistent from one execution of an application to another execution of the same application." But what is the use case you have for which you would like a firmly specified Enum::hashCode? Greetings Raffaello [1] https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/String.html#hashCode() [2] https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/Record.html#hashCode() [3] https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/Object.html#hashCode() On 2022-03-17 13:49, dfranken@gmail.com wrote: Dear all, Currently enums do not have a well-defined hashCode() implementation so they defer to Object.hashCode() which just uses an internal mechanism to determine the hashcode, likely based on the object's place in the heap. This may confuse a lot of developers as other classes such as String do have a well-defined hashcode which is consistent across multiple JVM's. Could it be a good idea to make enums' hashCode() method more reliably and predictable? For instance by using the hashcode of the name of its value? For example: class MyEnum { A, B } -> A.hashCode() == A.name().hashCode() Kind regards, Dave Franken
Re: RFR: 8283225: [AIX] ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 [v3]
On Thu, 17 Mar 2022 07:44:35 GMT, David Holmes wrote: >> Tyler Steele has refreshed the contents of this pull request, and previous >> commits have been removed. The incremental views will show differences >> compared to the previous content of the PR. The pull request contains one >> new commit since the last revision: >> >> Addresses failure in NullClassTest on AIX. >> >> - Changes malloc(0) call to malloc(1) on AIX. > > src/java.base/share/native/libjava/ClassLoader.c line 102: > >> 100: } >> 101: >> 102: // On malloc(0), implementators of malloc(3) have the choice to >> return either > > It is confusing to mix `malloc(0)`, where you are passing an argument zero to > malloc, with `malloc(3)` which actually means the definition of malloc as per > the man page in section 3. > > Given this is only an issue on AIX the comment can simply say: > > `// On AIX malloc(0) returns NULL which looks like an out-of-memory > condition; so adjust it to malloc(1)`. I would omit the bug number reference, they get stale and do not age well, cluttering up the source. Git blame can be used to find the origin of the comment if needed. - PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8279508: Auto-vectorize Math.round API [v17]
On Sun, 13 Mar 2022 06:36:15 GMT, Jatin Bhateja wrote: >> Summary of changes: >> - Intrinsify Math.round(float) and Math.round(double) APIs. >> - Extend auto-vectorizer to infer vector operations on encountering scalar >> IR nodes for above intrinsics. >> - Test creation using new IR testing framework. >> >> Following are the performance number of a JMH micro included with the patch >> >> Test System: Intel(R) Xeon(R) Platinum 8380 CPU @ 2.30GHz (Icelake Server) >> >> >> Benchmark | TESTSIZE | Baseline AVX3 (ops/ms) | Withopt AVX3 (ops/ms) | Gain >> ratio | Baseline AVX2 (ops/ms) | Withopt AVX2 (ops/ms) | Gain ratio >> -- | -- | -- | -- | -- | -- | -- | -- >> FpRoundingBenchmark.test_round_double | 1024.00 | 504.15 | 2209.54 | 4.38 | >> 510.36 | 548.39 | 1.07 >> FpRoundingBenchmark.test_round_double | 2048.00 | 293.64 | 1271.98 | 4.33 | >> 293.48 | 274.01 | 0.93 >> FpRoundingBenchmark.test_round_float | 1024.00 | 825.99 | 4754.66 | 5.76 | >> 751.83 | 2274.13 | 3.02 >> FpRoundingBenchmark.test_round_float | 2048.00 | 412.22 | 2490.09 | 6.04 | >> 388.52 | 1334.18 | 3.43 >> >> >> Kindly review and share your feedback. >> >> Best Regards, >> Jatin > > Jatin Bhateja has updated the pull request incrementally with one additional > commit since the last revision: > > 8279508: Windows build failure fix. src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 4143: > 4141: ldmxcsr(new_mxcsr); > 4142: // Move raw bits corresponding to double value 0.5 into scratch > register. > 4143: mov64(scratch, 4602678819172646912L); Suggestion: mov64(scratch, julong_cast(0.5)); - PR: https://git.openjdk.java.net/jdk/pull/7094
RFR: 8283279: [Testbug] Improve TestGetSwapSpaceSize
Please review this container test improvement. The test in question only makes sense iff the total swap space size as reported by the container aware OperatingSystemMXBean is `0`. If that's not the case, then something else might be amiss, e.g. OperatingSystemMXBean reporting the host swap limits. In such a case a passing test tells us nothing. Certainly not if the fix from [JDK-8242480](https://bugs.openjdk.java.net/browse/JDK-8242480) is present or not. Testing: - [x] Manual with and without the code fix of JDK-8242480. Still passes with the fix, and fails without. Tested the test on cgroups v1 and cgroups v2. - Commit messages: - 8283279: [Testbug] Improve TestGetSwapSpaceSize Changes: https://git.openjdk.java.net/jdk/pull/7854/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7854=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283279 Stats: 29 lines in 2 files changed: 22 ins; 1 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/7854.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7854/head:pull/7854 PR: https://git.openjdk.java.net/jdk/pull/7854
Re: RFR: 8257733: Move module-specific data from make to respective module [v9]
On Thu, 17 Mar 2022 00:12:38 GMT, Magnus Ihse Bursie wrote: >> A lot (but not all) of the data in make/data is tied to a specific module. >> For instance, the publicsuffixlist is used by java.base, and fontconfig by >> java.desktop. (A few directories, like mainmanifest, is *actually* used by >> make for the whole build.) >> >> These data files should move to the module they belong to. The are, after >> all, "source code" for that module that is "compiler" into resulting >> deliverables, for that module. (But the "source code" language is not Java >> or C, but typically a highly domain specific language or data format, and >> the "compilation" is, often, a specialized transformation.) >> >> This misplacement of the data directory is most visible at code review time. >> When such data is changed, most of the time build-dev (or the new build >> label) is involved, even though this has nothing to do with the build. While >> this is annoying, a worse problem is if the actual team that needs to review >> the patch (i.e., the team owning the module) is missed in the review. >> >> ### Modules reviewed >> >> - [x] java.base >> - [x] java.desktop >> - [x] jdk.compiler >> - [x] java.se > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Fix typos Marked as reviewed by erikj (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: Making enum hashcodes consistent across JVM's
- Original Message - > From: "dfranken jdk" > To: "core-libs-dev" > Sent: Thursday, March 17, 2022 1:49:08 PM > Subject: Making enum hashcodes consistent across JVM's > Dear all, > > Currently enums do not have a well-defined hashCode() implementation so > they defer to Object.hashCode() which just uses an internal mechanism > to determine the hashcode, likely based on the object's place in the > heap. No, and no. Enum.hashCode() is well defined, it is equivalent to super.hashCode() and it can not be overridden by a specific enum (see the javadoc). > > This may confuse a lot of developers as other classes such as String do > have a well-defined hashcode which is consistent across multiple JVM's. One of the reasons why String.hashCode() is defined is because the compiler (javac) compiles a switch on String using String.hashCode() so the hashCode at compile time and at runtime has to be the same. Having a well defined hashCode was a mistake, first it means that we can not change the implementation later (the current hashCode of String has too many collisions) and it also make DDOS attacks easier because you can pre-compute a list of strings with the same hashCode. > > Could it be a good idea to make enums' hashCode() method more reliably > and predictable? For instance by using the hashcode of the name of its > value? Enum already have a kind of perfect hash, ordinal() which is reliable and predictable but it's not a great value as hashCode. > > For example: > > class MyEnum { A, B } > -> A.hashCode() == A.name().hashCode() This is not a good hashCode, enum Foo { Aa, BB } Foo.Aa.hashCode() == Foo.BB.hashCode() > > Kind regards, > > Dave Franken regards, Rémi
Making enum hashcodes consistent across JVM's
Dear all, Currently enums do not have a well-defined hashCode() implementation so they defer to Object.hashCode() which just uses an internal mechanism to determine the hashcode, likely based on the object's place in the heap. This may confuse a lot of developers as other classes such as String do have a well-defined hashcode which is consistent across multiple JVM's. Could it be a good idea to make enums' hashCode() method more reliably and predictable? For instance by using the hashcode of the name of its value? For example: class MyEnum { A, B } -> A.hashCode() == A.name().hashCode() Kind regards, Dave Franken
Re: RFR: 8283287: Spring Cleaning for ClassLoader.c
On Wed, 16 Mar 2022 21:25:37 GMT, Tyler Steele wrote: > As mentioned in the issue, I'd like to perform the following tidying on > ClassLoader.c > > - Alphabetize includes. > - Replace 'if (ptr == 0)' with 'if (ptr == NULL)'. > - Replace 'return 0' with 'return NULL'. Looks good in general. Question inline. src/java.base/share/native/libjava/ClassLoader.c line 27: > 25: > 26: #include > 27: #include Whats errno.h for? - PR: https://git.openjdk.java.net/jdk/pull/7846
Integrated: 8281146: Replace StringCoding.hasNegatives with countPositives
On Wed, 26 Jan 2022 12:51:31 GMT, Claes Redestad wrote: > I'm requesting comments and, hopefully, some help with this patch to replace > `StringCoding.hasNegatives` with `countPositives`. The new method does a very > similar pass, but alters the intrinsic to return the number of leading bytes > in the `byte[]` range which only has positive bytes. This allows for dealing > much more efficiently with those `byte[]`s that has a ASCII prefix, with no > measurable cost on ASCII-only or latin1/UTF16-mostly input. > > Microbenchmark results: > https://jmh.morethan.io/?gists=428b487e92e3e47ccb7f169501600a88,3c585de7435506d3a3bdb32160fe8904 > > - Only implemented on x86 for now, but I want to verify that implementations > of `countPositives` can be implemented with similar efficiency on all > platforms that today implement a `hasNegatives` intrinsic (aarch64, ppc etc) > before moving ahead. This pretty much means holding up this until it's > implemented on all platforms, which can either contributed to this PR or as > dependent follow-ups. > > - An alternative to holding up until all platforms are on board is to allow > the implementation of `StringCoding.hasNegatives` and `countPositives` to be > implemented so that the non-intrinsified method calls into the intrinsified. > This requires structuring the implementations differently based on which > intrinsic - if any - is actually implemented. One way to do this could be to > mimic how `java.nio` handles unaligned accesses and expose which intrinsic is > available via `Unsafe` into a `static final` field. > > - There are a few minor regressions (~5%) in the x86 implementation on > `encode-/decodeLatin1Short`. Those regressions disappear when mixing inputs, > for example `encode-/decodeShortMixed` even see a minor improvement, which > makes me consider those corner case regressions with little real world > implications (if you have latin1 Strings, you're likely to also have > ASCII-only strings in your mix). This pull request has now been integrated. Changeset: beedae11 Author:Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/beedae1141b6b650dc4cedf1f038afc1c8b460dd Stats: 619 lines in 36 files changed: 278 ins; 61 del; 280 mod 8281146: Replace StringCoding.hasNegatives with countPositives Co-authored-by: Lutz Schmidt Co-authored-by: Martin Doerr Reviewed-by: kvn, lucy, rriggs - PR: https://git.openjdk.java.net/jdk/pull/7231
Re: RFR: 8281146: Replace StringCoding.hasNegatives with countPositives [v16]
> I'm requesting comments and, hopefully, some help with this patch to replace > `StringCoding.hasNegatives` with `countPositives`. The new method does a very > similar pass, but alters the intrinsic to return the number of leading bytes > in the `byte[]` range which only has positive bytes. This allows for dealing > much more efficiently with those `byte[]`s that has a ASCII prefix, with no > measurable cost on ASCII-only or latin1/UTF16-mostly input. > > Microbenchmark results: > https://jmh.morethan.io/?gists=428b487e92e3e47ccb7f169501600a88,3c585de7435506d3a3bdb32160fe8904 > > - Only implemented on x86 for now, but I want to verify that implementations > of `countPositives` can be implemented with similar efficiency on all > platforms that today implement a `hasNegatives` intrinsic (aarch64, ppc etc) > before moving ahead. This pretty much means holding up this until it's > implemented on all platforms, which can either contributed to this PR or as > dependent follow-ups. > > - An alternative to holding up until all platforms are on board is to allow > the implementation of `StringCoding.hasNegatives` and `countPositives` to be > implemented so that the non-intrinsified method calls into the intrinsified. > This requires structuring the implementations differently based on which > intrinsic - if any - is actually implemented. One way to do this could be to > mimic how `java.nio` handles unaligned accesses and expose which intrinsic is > available via `Unsafe` into a `static final` field. > > - There are a few minor regressions (~5%) in the x86 implementation on > `encode-/decodeLatin1Short`. Those regressions disappear when mixing inputs, > for example `encode-/decodeShortMixed` even see a minor improvement, which > makes me consider those corner case regressions with little real world > implications (if you have latin1 Strings, you're likely to also have > ASCII-only strings in your mix). Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Disallow negative values in TestCountPositives test - Changes: - all: https://git.openjdk.java.net/jdk/pull/7231/files - new: https://git.openjdk.java.net/jdk/pull/7231/files/bc5a8c80..6f22e1aa Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7231=15 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7231=14-15 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7231.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7231/head:pull/7231 PR: https://git.openjdk.java.net/jdk/pull/7231
Re: RFR: 8283059: Uninitialized warning in check_code.c with GCC 11.2
On Fri, 11 Mar 2022 23:38:10 GMT, Mikael Vidstedt wrote: > Background, from JBS: > > src/java.base/share/native/libverify/check_code.c: In function > 'read_all_code': > src/java.base/share/native/libverify/check_code.c:942:5: error: 'lengths' may > be used uninitialized [-Werror=maybe-uninitialized] > 942 | check_and_push(context, lengths, VM_MALLOC_BLK); > | ^~~ > src/java.base/share/native/libverify/check_code.c:4145:13: note: by argument > 2 of type 'const void *' to 'check_and_push' declared here > 4145 | static void check_and_push(context_type *context, const void *ptr, > int kind) > | ^~ > > > Because the second argument of check_and_push is "const void*" GCC assumes > that the malloc:ed data, which has not yet been initialized, will not be/can > not be modified later which in turn suggests it may be used without ever > being initialized. > > The same general issue was addressed in > [JDK-8266168](https://bugs.openjdk.java.net/browse/JDK-8266168), presumably > for GCC 11.1. > > > Details: > > Instead of sprinkling more calloc calls around or using pragmas/gcc > attributes I chose to change the check_and_push function to take a > (non-const) void* argument, and provide a new wrapper function > `check_and_push_const` which handles the const argument case. For the > (non-const) VM_MALLOC_BKP that means the pointer never needs to go through a > const conversion. > > To avoid having multiple ways of solving the same problem I also chose to > revert the change made in JDK-8266168, reverting the calloc back to a malloc > call. > > Testing: > > tier1 + builds-tier{2,3,4,5} Changes requested by kbarrett (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7794
Re: RFR: 8283059: Uninitialized warning in check_code.c with GCC 11.2
On Thu, 17 Mar 2022 08:08:07 GMT, Kim Barrett wrote: >> Background, from JBS: >> >> src/java.base/share/native/libverify/check_code.c: In function >> 'read_all_code': >> src/java.base/share/native/libverify/check_code.c:942:5: error: 'lengths' >> may be used uninitialized [-Werror=maybe-uninitialized] >> 942 | check_and_push(context, lengths, VM_MALLOC_BLK); >> | ^~~ >> src/java.base/share/native/libverify/check_code.c:4145:13: note: by argument >> 2 of type 'const void *' to 'check_and_push' declared here >> 4145 | static void check_and_push(context_type *context, const void *ptr, >> int kind) >> | ^~ >> >> >> Because the second argument of check_and_push is "const void*" GCC assumes >> that the malloc:ed data, which has not yet been initialized, will not be/can >> not be modified later which in turn suggests it may be used without ever >> being initialized. >> >> The same general issue was addressed in >> [JDK-8266168](https://bugs.openjdk.java.net/browse/JDK-8266168), presumably >> for GCC 11.1. >> >> >> Details: >> >> Instead of sprinkling more calloc calls around or using pragmas/gcc >> attributes I chose to change the check_and_push function to take a >> (non-const) void* argument, and provide a new wrapper function >> `check_and_push_const` which handles the const argument case. For the >> (non-const) VM_MALLOC_BKP that means the pointer never needs to go through a >> const conversion. >> >> To avoid having multiple ways of solving the same problem I also chose to >> revert the change made in JDK-8266168, reverting the calloc back to a malloc >> call. >> >> Testing: >> >> tier1 + builds-tier{2,3,4,5} > > src/java.base/share/native/libverify/check_code.c line 472: > >> 470: >> 471: static void check_and_push(context_type *context, void *ptr, int kind); >> 472: static void check_and_push_const(context_type *context, const void >> *ptr, int kind); > > This really needs a comment explaining why two functions are needed. I think it would be clearer to drop the kind argument and have two functions check_and_push_vm_string(context_type*, const char*) check_and_push_malloc_block(context_type*, void*) with a shared helper that is given the appropriate kind. - PR: https://git.openjdk.java.net/jdk/pull/7794
Re: RFR: 8283059: Uninitialized warning in check_code.c with GCC 11.2
On Thu, 17 Mar 2022 06:59:01 GMT, David Holmes wrote: >> Background, from JBS: >> >> src/java.base/share/native/libverify/check_code.c: In function >> 'read_all_code': >> src/java.base/share/native/libverify/check_code.c:942:5: error: 'lengths' >> may be used uninitialized [-Werror=maybe-uninitialized] >> 942 | check_and_push(context, lengths, VM_MALLOC_BLK); >> | ^~~ >> src/java.base/share/native/libverify/check_code.c:4145:13: note: by argument >> 2 of type 'const void *' to 'check_and_push' declared here >> 4145 | static void check_and_push(context_type *context, const void *ptr, >> int kind) >> | ^~ >> >> >> Because the second argument of check_and_push is "const void*" GCC assumes >> that the malloc:ed data, which has not yet been initialized, will not be/can >> not be modified later which in turn suggests it may be used without ever >> being initialized. >> >> The same general issue was addressed in >> [JDK-8266168](https://bugs.openjdk.java.net/browse/JDK-8266168), presumably >> for GCC 11.1. >> >> >> Details: >> >> Instead of sprinkling more calloc calls around or using pragmas/gcc >> attributes I chose to change the check_and_push function to take a >> (non-const) void* argument, and provide a new wrapper function >> `check_and_push_const` which handles the const argument case. For the >> (non-const) VM_MALLOC_BKP that means the pointer never needs to go through a >> const conversion. >> >> To avoid having multiple ways of solving the same problem I also chose to >> revert the change made in JDK-8266168, reverting the calloc back to a malloc >> call. >> >> Testing: >> >> tier1 + builds-tier{2,3,4,5} > > src/java.base/share/native/libverify/check_code.c line 4168: > >> 4166: } >> 4167: >> 4168: static void check_and_push_const(context_type *context, const void >> *ptr, int kind) { > > IIUC the only `const` cases all pass a `const char*` so perhaps we can make > that explicit in the signature? I confess I can't figure out how the passed > in memory eventually gets used and it seems bizarre that it could be a > freshly malloc'd block or an existing string. David - The check_and_push idiom checks if ptr is null, doing the out of memory handling if so. Otherwise, add ptr to a collection for later release. The kind determines how the release is done, either JVM_ReleaseUTF or free. - PR: https://git.openjdk.java.net/jdk/pull/7794
Re: RFR: 8283059: Uninitialized warning in check_code.c with GCC 11.2
On Fri, 11 Mar 2022 23:38:10 GMT, Mikael Vidstedt wrote: > Background, from JBS: > > src/java.base/share/native/libverify/check_code.c: In function > 'read_all_code': > src/java.base/share/native/libverify/check_code.c:942:5: error: 'lengths' may > be used uninitialized [-Werror=maybe-uninitialized] > 942 | check_and_push(context, lengths, VM_MALLOC_BLK); > | ^~~ > src/java.base/share/native/libverify/check_code.c:4145:13: note: by argument > 2 of type 'const void *' to 'check_and_push' declared here > 4145 | static void check_and_push(context_type *context, const void *ptr, > int kind) > | ^~ > > > Because the second argument of check_and_push is "const void*" GCC assumes > that the malloc:ed data, which has not yet been initialized, will not be/can > not be modified later which in turn suggests it may be used without ever > being initialized. > > The same general issue was addressed in > [JDK-8266168](https://bugs.openjdk.java.net/browse/JDK-8266168), presumably > for GCC 11.1. > > > Details: > > Instead of sprinkling more calloc calls around or using pragmas/gcc > attributes I chose to change the check_and_push function to take a > (non-const) void* argument, and provide a new wrapper function > `check_and_push_const` which handles the const argument case. For the > (non-const) VM_MALLOC_BKP that means the pointer never needs to go through a > const conversion. > > To avoid having multiple ways of solving the same problem I also chose to > revert the change made in JDK-8266168, reverting the calloc back to a malloc > call. > > Testing: > > tier1 + builds-tier{2,3,4,5} Changes requested by kbarrett (Reviewer). src/java.base/share/native/libverify/check_code.c line 472: > 470: > 471: static void check_and_push(context_type *context, void *ptr, int kind); > 472: static void check_and_push_const(context_type *context, const void *ptr, > int kind); This really needs a comment explaining why two functions are needed. - PR: https://git.openjdk.java.net/jdk/pull/7794
Re: RFR: 8283237: CallSite should be a sealed class [v2]
> Change `CallSite` to a sealed class, as `CallSite` is an abstract class which > does not allow direct subclassing by users per its documentation. Since I > don't have a JBS account, I posted the content for the CSR in a GitHub Gist > at https://gist.github.com/150d5aa7f8b13a4deddf95969ad39d73 and wish someone > can submit a CSR for me. liach has updated the pull request incrementally with one additional commit since the last revision: Swap mutable and volatile in the permits list - Changes: - all: https://git.openjdk.java.net/jdk/pull/7840/files - new: https://git.openjdk.java.net/jdk/pull/7840/files/ff8996e6..eef61c31 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7840=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7840=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7840.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7840/head:pull/7840 PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: 8283237: CallSite should be a sealed class
On Thu, 17 Mar 2022 07:32:40 GMT, liach wrote: >> src/java.base/share/classes/java/lang/invoke/CallSite.java line 88: >> >>> 86: */ >>> 87: public >>> 88: abstract sealed class CallSite permits ConstantCallSite, >>> VolatileCallSite, MutableCallSite { >> >> Nitpicking with my JSR 292 hat, >> given that the permits clause is reflected in the javadoc, >> the order should be ConstantCS, MutableCS and VolatileCS, >> it's both in the lexical order and in the "memory access" of setTarget() >> order , from stronger access to weaker access. > > I agree that Constant, Mutable, Volatile order is better, ranked by the > respective cost for `setTarget()` and (possibly) invocation, and earlier ones > in the list are more preferable if conditions allow. > > However, in the current API documentation, the order is Constant, Mutable, > and Volatile. Should I update that or leave it? > > /* > * > * If a mutable target is not required, an {@code invokedynamic} > instruction > * may be permanently bound by means of a {@linkplain ConstantCallSite > constant call site}. > * If a mutable target is required which has volatile variable semantics, > * because updates to the target must be immediately and reliably witnessed > by other threads, > * a {@linkplain VolatileCallSite volatile call site} may be used. > * Otherwise, if a mutable target is required, > * a {@linkplain MutableCallSite mutable call site} may be used. > * > */ For me, this is unrelated, for this paragraph it's easier to explain the semantics of MutableCallSite with an otherwise, i.e. it's mutable but you do not want the cost of a volatile acces. - PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: 8283225: [AIX] ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 [v3]
On Wed, 16 Mar 2022 21:10:14 GMT, Tyler Steele wrote: >> As described in the linked issue, NullClassBytesTest fails due an >> OutOfMemoryError produced on AIX when the test calls defineClass with a byte >> array of size of 0. The native implementation of defineClass then calls >> malloc with a size of 0. On AIX malloc(0) returns NULL, while on other >> platforms it return a valid address. When NULL is produced by malloc for >> this reason, ClassLoader.c incorrectly interprets this as a failure due to a >> lack of memory. >> >> ~~This PR modifies ClassLoader.c to produce an OutOfMemoryError only when >> `errno == ENOMEM` and to produce a ClassFormatError with the message >> "ClassLoader internal allocation failure" in all other cases (in which >> malloc returns NULL).~~ [edit: The above no longer describes the PR's >> proposed fix. See discussion below] >> >> In addition, I performed some minor tidy-up work in ClassLoader.c by >> changing instances of `return 0` to `return NULL`, and `if (some_ptr == 0)` >> to `if (some_ptr == NULL)`. This was done to improve the clarity of the code >> in ClassLoader.c, but didn't feel worthy of opening a separate issue. >> >> ### Alternatives >> >> It would be possible to address this failure by modifying the test to accept >> the OutOfMemoryError on AIX. I thought it was a better solution to modify >> ClassLoader.c to produce an OutOfMemoryError only when the system is >> actually out of memory. >> >> ### Testing >> >> This change has been tested on AIX and Linux/x86. > > Tyler Steele has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. The pull request contains one new > commit since the last revision: > > Addresses failure in NullClassTest on AIX. > > - Changes malloc(0) call to malloc(1) on AIX. p.s. this issue puts the finger on a sore point. We have corrected mallocs in the JDK in a number of places, e.g. in libverify: (https://github.com/openjdk/jdk/blob/3da5204b3c3a3f95bddcdcfe2628c2e0ed8a12d9/src/java.base/share/native/libverify/check_code.c#L91-L102) (which is also strictly speaking incorrect since the spec says to return a *non-unique* pointer). More generally, we have with AIX the problem that APIs have different behavior than the more usual nixes or are missing, and we need a porting layer to provide missing or change different APIs. Beside AIX-ish malloc, one example is dladdr, which is missing. In hotspot, we have os/aix, so we are fine. See `os/aix/hotspot/os/aix/porting_aix.cpp`. In the JDK I never found a good place for a porting layer, since the different JDK binaries don't have a common layer. So we have multiple versions of aix malloc and dladdr sprinkled across the libraries, which I always found embarrassing. (outside hotspot we implement dladdr at least in java.desktop/aix/native/libawt/porting_aix.c and java.base/aix/native/libjli/java_md_aix.c). If you find a way to commonize that code across JDK libraries, that would be cool. I even thought about providing a porting library completely independent from the OpenJDK itself, more like a system library. We did this for our internal iSeries port but the logistics were annoying, so we did not do it for AIX. But you at IBM may have a better idea. Cheers, Thomas - PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283225: [AIX] ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 [v3]
On Thu, 17 Mar 2022 07:44:39 GMT, David Holmes wrote: > Using AIX_ONLY this can be simplified: > > `body = (jbyte *)malloc(length AIX_ONLY( == 0 ? 1 : length));` This is jdk, not hotspot. Do we have AIX_ONLY in the JDK? - PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283225: [AIX] ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 [v3]
On Wed, 16 Mar 2022 21:10:14 GMT, Tyler Steele wrote: >> As described in the linked issue, NullClassBytesTest fails due an >> OutOfMemoryError produced on AIX when the test calls defineClass with a byte >> array of size of 0. The native implementation of defineClass then calls >> malloc with a size of 0. On AIX malloc(0) returns NULL, while on other >> platforms it return a valid address. When NULL is produced by malloc for >> this reason, ClassLoader.c incorrectly interprets this as a failure due to a >> lack of memory. >> >> ~~This PR modifies ClassLoader.c to produce an OutOfMemoryError only when >> `errno == ENOMEM` and to produce a ClassFormatError with the message >> "ClassLoader internal allocation failure" in all other cases (in which >> malloc returns NULL).~~ [edit: The above no longer describes the PR's >> proposed fix. See discussion below] >> >> In addition, I performed some minor tidy-up work in ClassLoader.c by >> changing instances of `return 0` to `return NULL`, and `if (some_ptr == 0)` >> to `if (some_ptr == NULL)`. This was done to improve the clarity of the code >> in ClassLoader.c, but didn't feel worthy of opening a separate issue. >> >> ### Alternatives >> >> It would be possible to address this failure by modifying the test to accept >> the OutOfMemoryError on AIX. I thought it was a better solution to modify >> ClassLoader.c to produce an OutOfMemoryError only when the system is >> actually out of memory. >> >> ### Testing >> >> This change has been tested on AIX and Linux/x86. > > Tyler Steele has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. The pull request contains one new > commit since the last revision: > > Addresses failure in NullClassTest on AIX. > > - Changes malloc(0) call to malloc(1) on AIX. src/java.base/share/native/libjava/ClassLoader.c line 102: > 100: } > 101: > 102: // On malloc(0), implementators of malloc(3) have the choice to > return either It is confusing to mix `malloc(0)`, where you are passing an argument zero to malloc, with `malloc(3)` which actually means the definition of malloc as per the man page in section 3. Given this is only an issue on AIX the comment can simply say: `// On AIX malloc(0) returns NULL which looks like an out-of-memory condition; so adjust it to malloc(1)`. src/java.base/share/native/libjava/ClassLoader.c line 106: > 104: // we chose the latter. (see 8283225) > 105: #ifdef _AIX > 106: body = (jbyte *)malloc(length == 0 ? 1 : length); Using AIX_ONLY this can be simplified: `body = (jbyte *)malloc(length AIX_ONLY( == 0 ? 1 : length));` - PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283237: CallSite should be a sealed class
On Thu, 17 Mar 2022 06:47:13 GMT, Rémi Forax wrote: >> Change `CallSite` to a sealed class, as `CallSite` is an abstract class >> which does not allow direct subclassing by users per its documentation. >> Since I don't have a JBS account, I posted the content for the CSR in a >> GitHub Gist at https://gist.github.com/150d5aa7f8b13a4deddf95969ad39d73 and >> wish someone can submit a CSR for me. > > src/java.base/share/classes/java/lang/invoke/CallSite.java line 88: > >> 86: */ >> 87: public >> 88: abstract sealed class CallSite permits ConstantCallSite, >> VolatileCallSite, MutableCallSite { > > Nitpicking with my JSR 292 hat, > given that the permits clause is reflected in the javadoc, > the order should be ConstantCS, MutableCS and VolatileCS, > it's both in the lexical order and in the "memory access" of setTarget() > order , from stronger access to weaker access. I agree that Constant, Mutable, Volatile order is better, ranked by the respective cost for `setTarget()` and (possibly) invocation, and earlier ones in the list are more preferable if conditions allow. However, in the current API documentation, the order is Constant, Mutable, and Volatile. Should I update that or leave it? /* * * If a mutable target is not required, an {@code invokedynamic} instruction * may be permanently bound by means of a {@linkplain ConstantCallSite constant call site}. * If a mutable target is required which has volatile variable semantics, * because updates to the target must be immediately and reliably witnessed by other threads, * a {@linkplain VolatileCallSite volatile call site} may be used. * Otherwise, if a mutable target is required, * a {@linkplain MutableCallSite mutable call site} may be used. * */ - PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: 8283225: [AIX] ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 [v3]
On Wed, 16 Mar 2022 21:10:14 GMT, Tyler Steele wrote: >> As described in the linked issue, NullClassBytesTest fails due an >> OutOfMemoryError produced on AIX when the test calls defineClass with a byte >> array of size of 0. The native implementation of defineClass then calls >> malloc with a size of 0. On AIX malloc(0) returns NULL, while on other >> platforms it return a valid address. When NULL is produced by malloc for >> this reason, ClassLoader.c incorrectly interprets this as a failure due to a >> lack of memory. >> >> ~~This PR modifies ClassLoader.c to produce an OutOfMemoryError only when >> `errno == ENOMEM` and to produce a ClassFormatError with the message >> "ClassLoader internal allocation failure" in all other cases (in which >> malloc returns NULL).~~ [edit: The above no longer describes the PR's >> proposed fix. See discussion below] >> >> In addition, I performed some minor tidy-up work in ClassLoader.c by >> changing instances of `return 0` to `return NULL`, and `if (some_ptr == 0)` >> to `if (some_ptr == NULL)`. This was done to improve the clarity of the code >> in ClassLoader.c, but didn't feel worthy of opening a separate issue. >> >> ### Alternatives >> >> It would be possible to address this failure by modifying the test to accept >> the OutOfMemoryError on AIX. I thought it was a better solution to modify >> ClassLoader.c to produce an OutOfMemoryError only when the system is >> actually out of memory. >> >> ### Testing >> >> This change has been tested on AIX and Linux/x86. > > Tyler Steele has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. The pull request contains one new > commit since the last revision: > > Addresses failure in NullClassTest on AIX. > > - Changes malloc(0) call to malloc(1) on AIX. This is much better, thanks. Cheers, Thomas - Marked as reviewed by stuefe (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283059: Uninitialized warning in check_code.c with GCC 11.2
On Fri, 11 Mar 2022 23:38:10 GMT, Mikael Vidstedt wrote: > Background, from JBS: > > src/java.base/share/native/libverify/check_code.c: In function > 'read_all_code': > src/java.base/share/native/libverify/check_code.c:942:5: error: 'lengths' may > be used uninitialized [-Werror=maybe-uninitialized] > 942 | check_and_push(context, lengths, VM_MALLOC_BLK); > | ^~~ > src/java.base/share/native/libverify/check_code.c:4145:13: note: by argument > 2 of type 'const void *' to 'check_and_push' declared here > 4145 | static void check_and_push(context_type *context, const void *ptr, > int kind) > | ^~ > > > Because the second argument of check_and_push is "const void*" GCC assumes > that the malloc:ed data, which has not yet been initialized, will not be/can > not be modified later which in turn suggests it may be used without ever > being initialized. > > The same general issue was addressed in > [JDK-8266168](https://bugs.openjdk.java.net/browse/JDK-8266168), presumably > for GCC 11.1. > > > Details: > > Instead of sprinkling more calloc calls around or using pragmas/gcc > attributes I chose to change the check_and_push function to take a > (non-const) void* argument, and provide a new wrapper function > `check_and_push_const` which handles the const argument case. For the > (non-const) VM_MALLOC_BKP that means the pointer never needs to go through a > const conversion. > > To avoid having multiple ways of solving the same problem I also chose to > revert the change made in JDK-8266168, reverting the calloc back to a malloc > call. > > Testing: > > tier1 + builds-tier{2,3,4,5} src/java.base/share/native/libverify/check_code.c line 4168: > 4166: } > 4167: > 4168: static void check_and_push_const(context_type *context, const void > *ptr, int kind) { IIUC the only `const` cases all pass a `const char*` so perhaps we can make that explicit in the signature? I confess I can't figure out how the passed in memory eventually gets used and it seems bizarre that it could be a freshly malloc'd block or an existing string. - PR: https://git.openjdk.java.net/jdk/pull/7794
Re: RFR: 8283237: CallSite should be a sealed class
On Wed, 16 Mar 2022 13:09:30 GMT, liach wrote: > Change `CallSite` to a sealed class, as `CallSite` is an abstract class which > does not allow direct subclassing by users per its documentation. Since I > don't have a JBS account, I posted the content for the CSR in a GitHub Gist > at https://gist.github.com/150d5aa7f8b13a4deddf95969ad39d73 and wish someone > can submit a CSR for me. src/java.base/share/classes/java/lang/invoke/CallSite.java line 88: > 86: */ > 87: public > 88: abstract sealed class CallSite permits ConstantCallSite, > VolatileCallSite, MutableCallSite { Nitpicking with my JSR 292 hat, given that the permits clause is reflected in the javadoc, the order should be ConstantCS, MutableCS and VolatileCS, it's both in the lexical order and in the "memory access" of setTarget() order , from stronger access to weaker access. - PR: https://git.openjdk.java.net/jdk/pull/7840