RFR: 8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines

David Holmes-2
All JRT_ENTRY routines are required to have a parameter, "JavaThread* thread", which must be the current thread. The JRT_ENTRY, and related macros, then use this parameter and also expose it as THREAD for use with exception macros. But the fact "thread" is the current thread is lost in some routines and we see strange calls to other code that pass both "thread" and "THREAD" as distinct parameters - primarily when a TRAPS method is involved.

This should be cleaned up along with a general check on misuse of TRAPS/THREAD.

Testing: tiers 1-3

Thanks,
David

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

Commit messages:
 - 8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines

Changes: https://git.openjdk.java.net/jdk/pull/3062/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3062&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8263709
  Stats: 110 lines in 6 files changed: 12 ins; 15 del; 83 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3062.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3062/head:pull/3062

PR: https://git.openjdk.java.net/jdk/pull/3062
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines

Coleen Phillimore-3
On Thu, 18 Mar 2021 02:48:05 GMT, David Holmes <[hidden email]> wrote:

> All JRT_ENTRY routines are required to have a parameter, "JavaThread* thread", which must be the current thread. The JRT_ENTRY, and related macros, then use this parameter and also expose it as THREAD for use with exception macros. But the fact "thread" is the current thread is lost in some routines and we see strange calls to other code that pass both "thread" and "THREAD" as distinct parameters - primarily when a TRAPS method is involved.
>
> This should be cleaned up along with a general check on misuse of TRAPS/THREAD.
>
> Testing: tiers 1-3
>
> Thanks,
> David

Can you state the convention that you're trying to establish?
I would like it to be when there is TRAPS, use THREAD for Handles, otherwise use current if passed as JavaThread* current.

src/hotspot/share/runtime/sharedRuntime.cpp line 1217:

> 1215:     assert(fr.is_entry_frame(), "must be");
> 1216:     // fr is now pointing to the entry frame.
> 1217:     callee_method = methodHandle(thread, fr.entry_frame_call_wrapper()->callee_method());

Ok, now this just seems arbitrary to me.  I don't see why we should introduce a JavaThread* thread, when there's a perfectly good THREAD available here, and THREAD is fine here.  There's a lot of code that uses THREAD as a Handle parameter and there's nothing wrong  with it.  If it's lower case 'thread', one has to wonder and look around to see if it's the current thread (changing the convention to 'current' was good where you did that).  Don't change these ones.  It seems like a conflict of your preferences to what's in the code, and I vote for what's used in the code everywhere.  Removing TRAPS from functions that don't throw an exception is also good. But I don't see the point of this at all and I think it's worse.

src/hotspot/share/runtime/sharedRuntime.cpp line 1340:

> 1338: // and are patched with the real destination of the call.
> 1339: methodHandle SharedRuntime::resolve_sub_helper(bool is_virtual, bool is_optimized, TRAPS) {
> 1340:   JavaThread* thread = THREAD->as_Java_thread();

Same comment as above.

src/hotspot/share/runtime/sharedRuntime.cpp line 2124:

> 2122:     Atomic::inc(BiasedLocking::slow_path_entry_count_addr());
> 2123:   }
> 2124:   Handle h_obj(thread, obj);

Can you make this one 'current'?

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

Changes requested by coleenp (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3062
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines

David Holmes-2
On Thu, 18 Mar 2021 12:17:25 GMT, Coleen Phillimore <[hidden email]> wrote:

>> All JRT_ENTRY routines are required to have a parameter, "JavaThread* thread", which must be the current thread. The JRT_ENTRY, and related macros, then use this parameter and also expose it as THREAD for use with exception macros. But the fact "thread" is the current thread is lost in some routines and we see strange calls to other code that pass both "thread" and "THREAD" as distinct parameters - primarily when a TRAPS method is involved.
>>
>> This should be cleaned up along with a general check on misuse of TRAPS/THREAD.
>>
>> Testing: tiers 1-3
>>
>> Thanks,
>> David
>
> src/hotspot/share/runtime/sharedRuntime.cpp line 1217:
>
>> 1215:     assert(fr.is_entry_frame(), "must be");
>> 1216:     // fr is now pointing to the entry frame.
>> 1217:     callee_method = methodHandle(thread, fr.entry_frame_call_wrapper()->callee_method());
>
> Ok, now this just seems arbitrary to me.  I don't see why we should introduce a JavaThread* thread, when there's a perfectly good THREAD available here, and THREAD is fine here.  There's a lot of code that uses THREAD as a Handle parameter and there's nothing wrong  with it.  If it's lower case 'thread', one has to wonder and look around to see if it's the current thread (changing the convention to 'current' was good where you did that).  Don't change these ones.  It seems like a conflict of your preferences to what's in the code, and I vote for what's used in the code everywhere.  Removing TRAPS from functions that don't throw an exception is also good. But I don't see the point of this at all and I think it's worse.

The intent is that THREAD, ideally, is only used in relation to exception throwing/catching and you can't tell that if you use it just for convenience everywhere else. I haven't introduced a new local "thread" when there is only a single use, but if there are multiple uses then using "thread" or "current" is IMO much clearer because we can clearly isolate the the code that relates to exceptions and the code that does not. That is what I'm trying to achieve here. It wont be perfect and not every use of THREAD will be replaced, but it will be a great improvement over what we currently have.
In this code I would love to change "thread" to "current" but the JRT macros require the use of "thread" and changing that would be too big a change. Note that in the code above I'm not introducing "thread" it is already there.

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

PR: https://git.openjdk.java.net/jdk/pull/3062
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines

David Holmes-2
In reply to this post by Coleen Phillimore-3
On Thu, 18 Mar 2021 12:19:34 GMT, Coleen Phillimore <[hidden email]> wrote:

>> All JRT_ENTRY routines are required to have a parameter, "JavaThread* thread", which must be the current thread. The JRT_ENTRY, and related macros, then use this parameter and also expose it as THREAD for use with exception macros. But the fact "thread" is the current thread is lost in some routines and we see strange calls to other code that pass both "thread" and "THREAD" as distinct parameters - primarily when a TRAPS method is involved.
>>
>> This should be cleaned up along with a general check on misuse of TRAPS/THREAD.
>>
>> Testing: tiers 1-3
>>
>> Thanks,
>> David
>
> src/hotspot/share/runtime/sharedRuntime.cpp line 2124:
>
>> 2122:     Atomic::inc(BiasedLocking::slow_path_entry_count_addr());
>> 2123:   }
>> 2124:   Handle h_obj(thread, obj);
>
> Can you make this one 'current'?

Unfortunately not - the JRT_BLOCK_NO_ASYNC further down requires "thread".

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

PR: https://git.openjdk.java.net/jdk/pull/3062
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines

David Holmes-2
In reply to this post by David Holmes-2
On Thu, 18 Mar 2021 12:47:08 GMT, David Holmes <[hidden email]> wrote:

>> src/hotspot/share/runtime/sharedRuntime.cpp line 1217:
>>
>>> 1215:     assert(fr.is_entry_frame(), "must be");
>>> 1216:     // fr is now pointing to the entry frame.
>>> 1217:     callee_method = methodHandle(thread, fr.entry_frame_call_wrapper()->callee_method());
>>
>> Ok, now this just seems arbitrary to me.  I don't see why we should introduce a JavaThread* thread, when there's a perfectly good THREAD available here, and THREAD is fine here.  There's a lot of code that uses THREAD as a Handle parameter and there's nothing wrong  with it.  If it's lower case 'thread', one has to wonder and look around to see if it's the current thread (changing the convention to 'current' was good where you did that).  Don't change these ones.  It seems like a conflict of your preferences to what's in the code, and I vote for what's used in the code everywhere.  Removing TRAPS from functions that don't throw an exception is also good. But I don't see the point of this at all and I think it's worse.
>
> The intent is that THREAD, ideally, is only used in relation to exception throwing/catching and you can't tell that if you use it just for convenience everywhere else. I haven't introduced a new local "thread" when there is only a single use, but if there are multiple uses then using "thread" or "current" is IMO much clearer because we can clearly isolate the the code that relates to exceptions and the code that does not. That is what I'm trying to achieve here. It wont be perfect and not every use of THREAD will be replaced, but it will be a great improvement over what we currently have.
> In this code I would love to change "thread" to "current" but the JRT macros require the use of "thread" and changing that would be too big a change. Note that in the code above I'm not introducing "thread" it is already there.

Correction, sorry I was misled by the mis-quoted code in the discussion view, I do introduce the local here because there are multiple none-exception-related uses.

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

PR: https://git.openjdk.java.net/jdk/pull/3062
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines

Ioi Lam-2
In reply to this post by David Holmes-2
On Thu, 18 Mar 2021 02:48:05 GMT, David Holmes <[hidden email]> wrote:

> All JRT_ENTRY routines are required to have a parameter, "JavaThread* thread", which must be the current thread. The JRT_ENTRY, and related macros, then use this parameter and also expose it as THREAD for use with exception macros. But the fact "thread" is the current thread is lost in some routines and we see strange calls to other code that pass both "thread" and "THREAD" as distinct parameters - primarily when a TRAPS method is involved.
>
> This should be cleaned up along with a general check on misuse of TRAPS/THREAD.
>
> Testing: tiers 1-3
>
> Thanks,
> David

Looks good to me in general. I have some comments about coding style.

src/hotspot/share/interpreter/interpreterRuntime.cpp line 319:

> 317:     if (trap_mdo == NULL) {
> 318:       ExceptionMark em(current);
> 319:       JavaThread* THREAD = current; // for exception macros

Need to assert that `current` is indeed the current thread. Maybe we should have an inline function like this which does the current thread check?

JavaThread* THREAD = current->for_exception_handling();

(Same comment for all the other `Thread* THREAD = thread;` in this PR).

src/hotspot/share/interpreter/interpreterRuntime.cpp line 974:

> 972:
> 973:
> 974: nmethod* InterpreterRuntime::frequency_counter_overflow(JavaThread* current, address branch_bcp) {

Need to assert that `current` is indeed current.

(Same comment for all other parameters that you've changed to `current`).

src/hotspot/share/interpreter/interpreterRuntime.cpp line 1117:

> 1115:
> 1116: JRT_ENTRY(MethodCounters*, InterpreterRuntime::build_method_counters(JavaThread* thread, Method* m))
> 1117:   MethodCounters* mcs = Method::build_method_counters(m, THREAD);

Need `ExceptionMark em(THREAD);`

src/hotspot/share/oops/method.cpp line 579:

> 577:
> 578:   if (LogTouchedMethods) {
> 579:     mh->log_touched(THREAD);

When you have THREAD as the last parameter, it's not clear whether

- You are calling a non-throwing function that just wants a thread, you
- You are calling an exception-throwing function but you are ignoring the exception on purpose

It seems the former is the case. I would prefer something like this, which makes it clear what you're doing:

mh->log_touched(THREAD->as_JavaThread());

src/hotspot/share/runtime/sharedRuntime.cpp line 1231:

> 1229: methodHandle SharedRuntime::resolve_helper(bool is_virtual, bool is_optimized, TRAPS) {
> 1230:   methodHandle callee_method;
> 1231:   callee_method = resolve_sub_helper(is_virtual, is_optimized, THREAD);

The two occurrences of  `THREAD` in this function seem fishy. Maybe they can be replaced with CHECK? But it's unrelated to this PR so probably should be done in a separate REF.

src/hotspot/share/runtime/sharedRuntime.cpp line 1661:

> 1659:
> 1660: methodHandle SharedRuntime::handle_ic_miss_helper(TRAPS) {
> 1661:   JavaThread* thread = THREAD->as_Java_thread();

A general comment about the functions that took both `thread` and `TRAPS`, where you removed the `thread`:

Are we sure they cannot be called on a non-current thread?

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

Changes requested by iklam (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3062
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines

David Holmes-2
On Thu, 18 Mar 2021 21:57:23 GMT, Ioi Lam <[hidden email]> wrote:

>> All JRT_ENTRY routines are required to have a parameter, "JavaThread* thread", which must be the current thread. The JRT_ENTRY, and related macros, then use this parameter and also expose it as THREAD for use with exception macros. But the fact "thread" is the current thread is lost in some routines and we see strange calls to other code that pass both "thread" and "THREAD" as distinct parameters - primarily when a TRAPS method is involved.
>>
>> This should be cleaned up along with a general check on misuse of TRAPS/THREAD.
>>
>> Testing: tiers 1-3
>>
>> Thanks,
>> David
>
> src/hotspot/share/runtime/sharedRuntime.cpp line 1661:
>
>> 1659:
>> 1660: methodHandle SharedRuntime::handle_ic_miss_helper(TRAPS) {
>> 1661:   JavaThread* thread = THREAD->as_Java_thread();
>
> A general comment about the functions that took both `thread` and `TRAPS`, where you removed the `thread`:
>
> Are we sure they cannot be called on a non-current thread?

Everything goes back to the JRT macros where THREAD is created from thread, so they are always the same current thread instance.

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

PR: https://git.openjdk.java.net/jdk/pull/3062
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines

Ioi Lam-2
On Thu, 18 Mar 2021 23:57:34 GMT, David Holmes <[hidden email]> wrote:

>> src/hotspot/share/runtime/sharedRuntime.cpp line 1661:
>>
>>> 1659:
>>> 1660: methodHandle SharedRuntime::handle_ic_miss_helper(TRAPS) {
>>> 1661:   JavaThread* thread = THREAD->as_Java_thread();
>>
>> A general comment about the functions that took both `thread` and `TRAPS`, where you removed the `thread`:
>>
>> Are we sure they cannot be called on a non-current thread?
>
> Everything goes back to the JRT macros where THREAD is created from thread, so they are always the same current thread instance.

Have you check all possible call paths? Could one of these functions be called from a place other than the JRT functions?

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

PR: https://git.openjdk.java.net/jdk/pull/3062
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines

Coleen Phillimore-3
In reply to this post by David Holmes-2
On Thu, 18 Mar 2021 12:58:35 GMT, David Holmes <[hidden email]> wrote:

>> The intent is that THREAD, ideally, is only used in relation to exception throwing/catching and you can't tell that if you use it just for convenience everywhere else. I haven't introduced a new local "thread" when there is only a single use, but if there are multiple uses then using "thread" or "current" is IMO much clearer because we can clearly isolate the the code that relates to exceptions and the code that does not. That is what I'm trying to achieve here. It wont be perfect and not every use of THREAD will be replaced, but it will be a great improvement over what we currently have.
>> In this code I would love to change "thread" to "current" but the JRT macros require the use of "thread" and changing that would be too big a change. Note that in the code above I'm not introducing "thread" it is already there.
>
> Correction, sorry I was misled by the mis-quoted code in the discussion view, I do introduce the local here because there are multiple none-exception-related uses.

I don't find passing "thread" cleaner than THREAD in these cases.  We *know* THREAD is the current thread and these functions pass it as the leading parameter is not confusing at all.  These patterns: of Handle(THREAD, obj), MutexLocker ml(THREAD, lock), and methodHandle method(THREAD, m); appear everywhere. I put many of them there to pass the current thread parameter.  Changing these patterns is a huge amount of churn for no benefit to me at least.  I don't think I want to hunt around for a 'thread' that I hope is the current thread.  I will use THREAD when needed.
Most of these changes seem harmless but I object to introducing a 'thread' just to avoid passing THREAD.

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

PR: https://git.openjdk.java.net/jdk/pull/3062
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines

Coleen Phillimore-3
In reply to this post by David Holmes-2
On Thu, 18 Mar 2021 02:48:05 GMT, David Holmes <[hidden email]> wrote:

> All JRT_ENTRY routines are required to have a parameter, "JavaThread* thread", which must be the current thread. The JRT_ENTRY, and related macros, then use this parameter and also expose it as THREAD for use with exception macros. But the fact "thread" is the current thread is lost in some routines and we see strange calls to other code that pass both "thread" and "THREAD" as distinct parameters - primarily when a TRAPS method is involved.
>
> This should be cleaned up along with a general check on misuse of TRAPS/THREAD.
>
> Testing: tiers 1-3
>
> Thanks,
> David

Changes requested by coleenp (Reviewer).

src/hotspot/share/runtime/sharedRuntime.cpp line 1772:

> 1770:   ResourceMark rm(thread);
> 1771:   RegisterMap reg_map(thread, false);
> 1772:   frame stub_frame = thread->last_frame();

The call to thread->last_frame() may require JavaThread.  I think it's a good change to remove the thread parameter.

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

PR: https://git.openjdk.java.net/jdk/pull/3062
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines

Coleen Phillimore-3
In reply to this post by Ioi Lam-2
On Thu, 18 Mar 2021 21:51:30 GMT, Ioi Lam <[hidden email]> wrote:

>> All JRT_ENTRY routines are required to have a parameter, "JavaThread* thread", which must be the current thread. The JRT_ENTRY, and related macros, then use this parameter and also expose it as THREAD for use with exception macros. But the fact "thread" is the current thread is lost in some routines and we see strange calls to other code that pass both "thread" and "THREAD" as distinct parameters - primarily when a TRAPS method is involved.
>>
>> This should be cleaned up along with a general check on misuse of TRAPS/THREAD.
>>
>> Testing: tiers 1-3
>>
>> Thanks,
>> David
>
> src/hotspot/share/runtime/sharedRuntime.cpp line 1231:
>
>> 1229: methodHandle SharedRuntime::resolve_helper(bool is_virtual, bool is_optimized, TRAPS) {
>> 1230:   methodHandle callee_method;
>> 1231:   callee_method = resolve_sub_helper(is_virtual, is_optimized, THREAD);
>
> The two occurrences of  `THREAD` in this function seem fishy. Maybe they can be replaced with CHECK? But it's unrelated to this PR so probably should be done in a separate REF.

This seems like a case where you should have removed the trailing THREAD and not the leading 'thread'.

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

PR: https://git.openjdk.java.net/jdk/pull/3062
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines

Coleen Phillimore-3
In reply to this post by Ioi Lam-2
On Fri, 19 Mar 2021 00:31:32 GMT, Ioi Lam <[hidden email]> wrote:

>> Everything goes back to the JRT macros where THREAD is created from thread, so they are always the same current thread instance.
>
> Have you check all possible call paths? Could one of these functions be called from a place other than the JRT functions?

This is why THREAD is clearly the current thread because that's what the JRT_/JVM_ macros do.  Just use THREAD as the current thread for ResourceMark and whatever else needs it below.

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

PR: https://git.openjdk.java.net/jdk/pull/3062
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines

Coleen Phillimore-3
In reply to this post by David Holmes-2
On Thu, 18 Mar 2021 12:52:43 GMT, David Holmes <[hidden email]> wrote:

>> src/hotspot/share/runtime/sharedRuntime.cpp line 2124:
>>
>>> 2122:     Atomic::inc(BiasedLocking::slow_path_entry_count_addr());
>>> 2123:   }
>>> 2124:   Handle h_obj(thread, obj);
>>
>> Can you make this one 'current'?
>
> Unfortunately not - the JRT_BLOCK_NO_ASYNC requires "thread".

Then keep it THREAD.  That implies current thread.

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

PR: https://git.openjdk.java.net/jdk/pull/3062
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines

Coleen Phillimore-3
In reply to this post by Coleen Phillimore-3
On Fri, 19 Mar 2021 01:48:47 GMT, Coleen Phillimore <[hidden email]> wrote:

>> All JRT_ENTRY routines are required to have a parameter, "JavaThread* thread", which must be the current thread. The JRT_ENTRY, and related macros, then use this parameter and also expose it as THREAD for use with exception macros. But the fact "thread" is the current thread is lost in some routines and we see strange calls to other code that pass both "thread" and "THREAD" as distinct parameters - primarily when a TRAPS method is involved.
>>
>> This should be cleaned up along with a general check on misuse of TRAPS/THREAD.
>>
>> Testing: tiers 1-3
>>
>> Thanks,
>> David
>
> Changes requested by coleenp (Reviewer).

I thought the purpose of this work was to make TRAPS and the JRT/JVM_ENTRY macros take JavaThread instead of Thread.
ie #define TRAPS JavaThread* THREAD

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

PR: https://git.openjdk.java.net/jdk/pull/3062
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines

David Holmes
On 19/03/2021 10:00 pm, Coleen Phillimore wrote:

> On Fri, 19 Mar 2021 01:48:47 GMT, Coleen Phillimore <[hidden email]> wrote:
>
>>> All JRT_ENTRY routines are required to have a parameter, "JavaThread* thread", which must be the current thread. The JRT_ENTRY, and related macros, then use this parameter and also expose it as THREAD for use with exception macros. But the fact "thread" is the current thread is lost in some routines and we see strange calls to other code that pass both "thread" and "THREAD" as distinct parameters - primarily when a TRAPS method is involved.
>>>
>>> This should be cleaned up along with a general check on misuse of TRAPS/THREAD.
>>>
>>> Testing: tiers 1-3
>>>
>>> Thanks,
>>> David
>>
>> Changes requested by coleenp (Reviewer).
>
> I thought the purpose of this work was to make TRAPS and the JRT/JVM_ENTRY macros take JavaThread instead of Thread.
> ie #define TRAPS JavaThread* THREAD

That is the endgame for JDK-8252685. The current issue is a step along
the way to make that one more digestible. The entry macros already work
with the current JavaThread:
- JRT_ENTRY uses the "thread" parameter of the wrapped method
- JVM_ENTRY and JNI_ENTRY extract it from the JNIEnv

and in all cases they then declare an alias:

Thread* THREAD = thread;

for use with TRAPS/CHECK

David
-----

> -------------
>
> PR: https://git.openjdk.java.net/jdk/pull/3062
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines

David Holmes-2
In reply to this post by Coleen Phillimore-3
On Fri, 19 Mar 2021 11:56:43 GMT, Coleen Phillimore <[hidden email]> wrote:

>> Changes requested by coleenp (Reviewer).
>
> I thought the purpose of this work was to make TRAPS and the JRT/JVM_ENTRY macros take JavaThread instead of Thread.
> ie #define TRAPS JavaThread* THREAD

I'm closing this PR and coming back with a new version that we have discussed offline. A clean start will be easier.

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

PR: https://git.openjdk.java.net/jdk/pull/3062
Reply | Threaded
Open this post in threaded view
|

Withdrawn: 8263709: Cleanup THREAD/TRAPS/CHECK usage in JRT_ENTRY routines

David Holmes-2
In reply to this post by David Holmes-2
On Thu, 18 Mar 2021 02:48:05 GMT, David Holmes <[hidden email]> wrote:

> All JRT_ENTRY routines are required to have a parameter, "JavaThread* thread", which must be the current thread. The JRT_ENTRY, and related macros, then use this parameter and also expose it as THREAD for use with exception macros. But the fact "thread" is the current thread is lost in some routines and we see strange calls to other code that pass both "thread" and "THREAD" as distinct parameters - primarily when a TRAPS method is involved.
>
> This should be cleaned up along with a general check on misuse of TRAPS/THREAD.
>
> Testing: tiers 1-3
>
> Thanks,
> David

This pull request has been closed without being integrated.

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

PR: https://git.openjdk.java.net/jdk/pull/3062