RFR: 8292302: Windows GetLastError value overwritten by ThreadLocalStorage::thread [v2]

[Daniel D. Daugherty]

On Tue, 6 Sep 2022 08:42:13 GMT, Kevin Walls <kevinw at openjdk.org> wrote:

>> This is an MR which partially reverts JDK-8289091 such that JavaThread::threadObj() does not call Thread::current().
>> 
>> A JVMTI operation could call threadObj() and clear the Windows GetLastError value.
>> 
>> Partial, because I haven't reverted changes in JavaThread::print_on_error(), they aren't connected to the problems seen.
>
> Kevin Walls has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Comment update

I agree with your choices in your partial [BACKOUT] of:

[JDK-8289091](https://bugs.openjdk.org/browse/JDK-8289091) move oop safety check from SharedRuntime::get_java_tid() to JavaThread::threadObj()

Thanks for adding the new test. I like it. 

Concerning:
>  More follow up needed about the last error reset caused by GC. 
Have you filed a follow up bug in hotspot/gc for that issue?

Thumbs up on this fix and new test.

test/jdk/com/sun/jdi/JdbLastErrorTest.java line 42:

> 40: import java.lang.foreign.SymbolLookup;
> 41: import java.lang.invoke.MethodHandle;
> 42: import java.lang.foreign.ValueLayout;

L41 and L42 should be swapped for proper alpha sort order.

test/jdk/com/sun/jdi/JdbLastErrorTest.java line 60:

> 58:             FunctionDescriptor.ofVoid(ValueLayout.JAVA_INT));
> 59: 
> 60:         for (int i=0; i<10; i++) {

nit: should be space around '=' and '<'.

test/jdk/com/sun/jdi/JdbLastErrorTest.java line 61:

> 59: 
> 60:         for (int i=0; i<10; i++) {
> 61:             setLastError.invoke(42);

If you use final variable for the '42', then you can use the variable
here on L61 and below on L64. Then you can call is something cool:

static final int THE_ANSWER = 42;  // ... to life, the universe and everything!

-------------

Marked as reviewed by dcubed (Reviewer).

PR: https://git.openjdk.org/jdk/pull/10147