RFR(XS): 8215009: GCC 8 compilation eror in libjli
Hello, Please review a small fix in java_md_solinux.c: continuation is not truly compatible with pthread_create start_routine's signature but we control what actually happens. So it makes sense to add intermediate void* cast to silence the error. bug: https://bugs.openjdk.java.net/browse/JDK-8215009 webrev: http://cr.openjdk.java.net/~dchuyko/8215009/webrev.00/ testing: submit repo (mach5-one-dchuyko-JDK-8215009-20181207-1625-13615: PASSED) -Dmitry
Hi Dmitry, On 11/12/2018 12:16 am, Dmitry Chuyko wrote:
Hello,
Please review a small fix in java_md_solinux.c: continuation is not truly compatible with pthread_create start_routine's signature but we control what actually happens. So it makes sense to add intermediate void* cast to silence the error.
I'd be tempted to fix the signature and get rid of all the casts. Cheers, David
bug: https://bugs.openjdk.java.net/browse/JDK-8215009 webrev: http://cr.openjdk.java.net/~dchuyko/8215009/webrev.00/ testing: submit repo (mach5-one-dchuyko-JDK-8215009-20181207-1625-13615: PASSED)
-Dmitry
On 12/11/18 4:03 AM, David Holmes wrote:
Hi Dmitry,
On 11/12/2018 12:16 am, Dmitry Chuyko wrote:
Hello,
Please review a small fix in java_md_solinux.c: continuation is not truly compatible with pthread_create start_routine's signature but we control what actually happens. So it makes sense to add intermediate void* cast to silence the error.
I'd be tempted to fix the signature and get rid of all the casts.
David, the signature is a signature of int JNICALL JavaMain(void * _args) It would be fun to change it. But still on Windows it is correctly passed to _beginthreadex() and then return code is extracted with GetExitCodeThread(). In case we want it to return void* the cast will move there. -Dmitry
Cheers, David
bug: https://bugs.openjdk.java.net/browse/JDK-8215009 webrev: http://cr.openjdk.java.net/~dchuyko/8215009/webrev.00/ testing: submit repo (mach5-one-dchuyko-JDK-8215009-20181207-1625-13615: PASSED)
-Dmitry
On 11/12/2018 9:30 pm, Dmitry Chuyko wrote:
On 12/11/18 4:03 AM, David Holmes wrote:
Hi Dmitry,
On 11/12/2018 12:16 am, Dmitry Chuyko wrote:
Hello,
Please review a small fix in java_md_solinux.c: continuation is not truly compatible with pthread_create start_routine's signature but we control what actually happens. So it makes sense to add intermediate void* cast to silence the error.
I'd be tempted to fix the signature and get rid of all the casts.
David, the signature is a signature of
int JNICALL JavaMain(void * _args)
It would be fun to change it. But still on Windows it is correctly passed to _beginthreadex() and then return code is extracted with GetExitCodeThread(). In case we want it to return void* the cast will move there.
I think the current double cast is truly ugly and an ifdef for windows, or a cast for Windows only would be an improvement. But I won't impose that on you just to silence gcc 8. Cheers, David
-Dmitry
Cheers, David
bug: https://bugs.openjdk.java.net/browse/JDK-8215009 webrev: http://cr.openjdk.java.net/~dchuyko/8215009/webrev.00/ testing: submit repo (mach5-one-dchuyko-JDK-8215009-20181207-1625-13615: PASSED)
-Dmitry
On 12/18/18 3:39 AM, David Holmes wrote:
On 11/12/2018 9:30 pm, Dmitry Chuyko wrote:
On 12/11/18 4:03 AM, David Holmes wrote:
Hi Dmitry,
On 11/12/2018 12:16 am, Dmitry Chuyko wrote:
Hello,
Please review a small fix in java_md_solinux.c: continuation is not truly compatible with pthread_create start_routine's signature but we control what actually happens. So it makes sense to add intermediate void* cast to silence the error.
I'd be tempted to fix the signature and get rid of all the casts.
David, the signature is a signature of
int JNICALL JavaMain(void * _args)
It would be fun to change it. But still on Windows it is correctly passed to _beginthreadex() and then return code is extracted with GetExitCodeThread(). In case we want it to return void* the cast will move there.
I think the current double cast is truly ugly and an ifdef for windows, or a cast for Windows only would be an improvement.
I agree. Maybe making a wrapper function is not so ugly. If there are no objections to changing beginning of the call stack it is quite easy to implement. For consistency it may be done for all 3 points (posix unix, posix mac, windows) or just for posix ones. It looks like ifdef should be better as long as there are already OS-specific parts in libjli. Again, if there are no objections to have different JavaMain signatures on different platforms. In this case there won't be a signature cast for Windows. -Dmitry
But I won't impose that on you just to silence gcc 8.
Cheers, David
-Dmitry
Cheers, David
bug: https://bugs.openjdk.java.net/browse/JDK-8215009 webrev: http://cr.openjdk.java.net/~dchuyko/8215009/webrev.00/ testing: submit repo (mach5-one-dchuyko-JDK-8215009-20181207-1625-13615: PASSED)
-Dmitry
On 19/12/2018 1:56 am, Dmitry Chuyko wrote:
On 12/18/18 3:39 AM, David Holmes wrote:
On 11/12/2018 9:30 pm, Dmitry Chuyko wrote:
On 12/11/18 4:03 AM, David Holmes wrote:
Hi Dmitry,
On 11/12/2018 12:16 am, Dmitry Chuyko wrote:
Hello,
Please review a small fix in java_md_solinux.c: continuation is not truly compatible with pthread_create start_routine's signature but we control what actually happens. So it makes sense to add intermediate void* cast to silence the error.
I'd be tempted to fix the signature and get rid of all the casts.
David, the signature is a signature of
int JNICALL JavaMain(void * _args)
It would be fun to change it. But still on Windows it is correctly passed to _beginthreadex() and then return code is extracted with GetExitCodeThread(). In case we want it to return void* the cast will move there.
I think the current double cast is truly ugly and an ifdef for windows, or a cast for Windows only would be an improvement.
I agree. Maybe making a wrapper function is not so ugly. If there are no objections to changing beginning of the call stack it is quite easy to implement. For consistency it may be done for all 3 points (posix unix, posix mac, windows) or just for posix ones.
It looks like ifdef should be better as long as there are already OS-specific parts in libjli. Again, if there are no objections to have different JavaMain signatures on different platforms. In this case there won't be a signature cast for Windows.
How about setting #define THREAD_FUNC_RETURN int in windows/java_md.h. Then: #ifndef THREAD_FUNC_RETURN #define THREAD_FUNC_RETURN void* #endif in java.h (after the other includes). Then: THREAD_FUNC_RETURN JNICALL JavaMain(void * _args) in java.c. ? Cheers, David
-Dmitry
But I won't impose that on you just to silence gcc 8.
Cheers, David
-Dmitry
Cheers, David
bug: https://bugs.openjdk.java.net/browse/JDK-8215009 webrev: http://cr.openjdk.java.net/~dchuyko/8215009/webrev.00/ testing: submit repo (mach5-one-dchuyko-JDK-8215009-20181207-1625-13615: PASSED)
-Dmitry
To me thread function wrappers look preferable to platform specific JavaMain signature. Consider this webrev with wrappers: http://cr.openjdk.java.net/~dchuyko/8215009/webrev.01/ In some cases JavaMain is called in the same thread and its result is returned from JLI_Launch. ContinueInNewThread is in shared code. And JavaMain uses macro controlled returns. So when JavaMain returns THREAD_FUNC_RETURN changes may contain some quite artificial macro parts in java.c: http://cr.openjdk.java.net/~dchuyko/8215009/webrev.02/ -Dmitry On 12/19/18 9:27 AM, David Holmes wrote:
On 19/12/2018 1:56 am, Dmitry Chuyko wrote:
On 12/18/18 3:39 AM, David Holmes wrote:
On 11/12/2018 9:30 pm, Dmitry Chuyko wrote:
On 12/11/18 4:03 AM, David Holmes wrote:
Hi Dmitry,
On 11/12/2018 12:16 am, Dmitry Chuyko wrote:
Hello,
Please review a small fix in java_md_solinux.c: continuation is not truly compatible with pthread_create start_routine's signature but we control what actually happens. So it makes sense to add intermediate void* cast to silence the error.
I'd be tempted to fix the signature and get rid of all the casts.
David, the signature is a signature of
int JNICALL JavaMain(void * _args)
It would be fun to change it. But still on Windows it is correctly passed to _beginthreadex() and then return code is extracted with GetExitCodeThread(). In case we want it to return void* the cast will move there.
I think the current double cast is truly ugly and an ifdef for windows, or a cast for Windows only would be an improvement.
I agree. Maybe making a wrapper function is not so ugly. If there are no objections to changing beginning of the call stack it is quite easy to implement. For consistency it may be done for all 3 points (posix unix, posix mac, windows) or just for posix ones.
It looks like ifdef should be better as long as there are already OS-specific parts in libjli. Again, if there are no objections to have different JavaMain signatures on different platforms. In this case there won't be a signature cast for Windows.
How about setting
#define THREAD_FUNC_RETURN int
in windows/java_md.h.
Then:
#ifndef THREAD_FUNC_RETURN #define THREAD_FUNC_RETURN void* #endif
in java.h (after the other includes).
Then:
THREAD_FUNC_RETURN JNICALL JavaMain(void * _args)
in java.c.
?
Cheers, David
-Dmitry
But I won't impose that on you just to silence gcc 8.
Cheers, David
-Dmitry
Cheers, David
bug: https://bugs.openjdk.java.net/browse/JDK-8215009 webrev: http://cr.openjdk.java.net/~dchuyko/8215009/webrev.00/ testing: submit repo (mach5-one-dchuyko-JDK-8215009-20181207-1625-13615: PASSED)
-Dmitry
The wrapper based solution looks much cleaner to me as well. webrev.01 looks good. (not a Reviewer) Cheers, Mikael
On Feb 21, 2019, at 9:55 AM, Dmitry Chuyko <dmitry.chuyko@bell-sw.com> wrote:
To me thread function wrappers look preferable to platform specific JavaMain signature. Consider this webrev with wrappers:
http://cr.openjdk.java.net/~dchuyko/8215009/webrev.01/
In some cases JavaMain is called in the same thread and its result is returned from JLI_Launch. ContinueInNewThread is in shared code. And JavaMain uses macro controlled returns. So when JavaMain returns THREAD_FUNC_RETURN changes may contain some quite artificial macro parts in java.c:
http://cr.openjdk.java.net/~dchuyko/8215009/webrev.02/
-Dmitry
On 12/19/18 9:27 AM, David Holmes wrote:
On 19/12/2018 1:56 am, Dmitry Chuyko wrote:
On 12/18/18 3:39 AM, David Holmes wrote:
On 11/12/2018 9:30 pm, Dmitry Chuyko wrote:
On 12/11/18 4:03 AM, David Holmes wrote:
Hi Dmitry,
On 11/12/2018 12:16 am, Dmitry Chuyko wrote: > Hello, > > Please review a small fix in java_md_solinux.c: continuation is not truly compatible with pthread_create start_routine's signature but we control what actually happens. So it makes sense to add intermediate void* cast to silence the error.
I'd be tempted to fix the signature and get rid of all the casts.
David, the signature is a signature of
int JNICALL JavaMain(void * _args)
It would be fun to change it. But still on Windows it is correctly passed to _beginthreadex() and then return code is extracted with GetExitCodeThread(). In case we want it to return void* the cast will move there.
I think the current double cast is truly ugly and an ifdef for windows, or a cast for Windows only would be an improvement.
I agree. Maybe making a wrapper function is not so ugly. If there are no objections to changing beginning of the call stack it is quite easy to implement. For consistency it may be done for all 3 points (posix unix, posix mac, windows) or just for posix ones.
It looks like ifdef should be better as long as there are already OS-specific parts in libjli. Again, if there are no objections to have different JavaMain signatures on different platforms. In this case there won't be a signature cast for Windows.
How about setting
#define THREAD_FUNC_RETURN int
in windows/java_md.h.
Then:
#ifndef THREAD_FUNC_RETURN #define THREAD_FUNC_RETURN void* #endif
in java.h (after the other includes).
Then:
THREAD_FUNC_RETURN JNICALL JavaMain(void * _args)
in java.c.
?
Cheers, David
-Dmitry
But I won't impose that on you just to silence gcc 8.
Cheers, David
-Dmitry
Cheers, David
> bug: https://bugs.openjdk.java.net/browse/JDK-8215009 > webrev: http://cr.openjdk.java.net/~dchuyko/8215009/webrev.00/ > testing: submit repo (mach5-one-dchuyko-JDK-8215009-20181207-1625-13615: PASSED) > > -Dmitry >
On 22/02/2019 4:55 am, Mikael Vidstedt wrote:
The wrapper based solution looks much cleaner to me as well. webrev.01 looks good.
Sorry I really don't like it. The wrappers obfuscate and make complicated something that is not at all complicated. I have to re-read the new code 3 times to figure out what it is even doing! All that complexity to handle the fact one platform wants to return int instead of void* ?? David -----
(not a Reviewer)
Cheers, Mikael
On Feb 21, 2019, at 9:55 AM, Dmitry Chuyko <dmitry.chuyko@bell-sw.com> wrote:
To me thread function wrappers look preferable to platform specific JavaMain signature. Consider this webrev with wrappers:
http://cr.openjdk.java.net/~dchuyko/8215009/webrev.01/
In some cases JavaMain is called in the same thread and its result is returned from JLI_Launch. ContinueInNewThread is in shared code. And JavaMain uses macro controlled returns. So when JavaMain returns THREAD_FUNC_RETURN changes may contain some quite artificial macro parts in java.c:
http://cr.openjdk.java.net/~dchuyko/8215009/webrev.02/
-Dmitry
On 12/19/18 9:27 AM, David Holmes wrote:
On 19/12/2018 1:56 am, Dmitry Chuyko wrote:
On 12/18/18 3:39 AM, David Holmes wrote:
On 11/12/2018 9:30 pm, Dmitry Chuyko wrote:
On 12/11/18 4:03 AM, David Holmes wrote: > Hi Dmitry, > > On 11/12/2018 12:16 am, Dmitry Chuyko wrote: >> Hello, >> >> Please review a small fix in java_md_solinux.c: continuation is not truly compatible with pthread_create start_routine's signature but we control what actually happens. So it makes sense to add intermediate void* cast to silence the error. > > I'd be tempted to fix the signature and get rid of all the casts.
David, the signature is a signature of
int JNICALL JavaMain(void * _args)
It would be fun to change it. But still on Windows it is correctly passed to _beginthreadex() and then return code is extracted with GetExitCodeThread(). In case we want it to return void* the cast will move there.
I think the current double cast is truly ugly and an ifdef for windows, or a cast for Windows only would be an improvement.
I agree. Maybe making a wrapper function is not so ugly. If there are no objections to changing beginning of the call stack it is quite easy to implement. For consistency it may be done for all 3 points (posix unix, posix mac, windows) or just for posix ones.
It looks like ifdef should be better as long as there are already OS-specific parts in libjli. Again, if there are no objections to have different JavaMain signatures on different platforms. In this case there won't be a signature cast for Windows.
How about setting
#define THREAD_FUNC_RETURN int
in windows/java_md.h.
Then:
#ifndef THREAD_FUNC_RETURN #define THREAD_FUNC_RETURN void* #endif
in java.h (after the other includes).
Then:
THREAD_FUNC_RETURN JNICALL JavaMain(void * _args)
in java.c.
?
Cheers, David
-Dmitry
But I won't impose that on you just to silence gcc 8.
Cheers, David
-Dmitry
> > Cheers, > David > >> bug: https://bugs.openjdk.java.net/browse/JDK-8215009 >> webrev: http://cr.openjdk.java.net/~dchuyko/8215009/webrev.00/ >> testing: submit repo (mach5-one-dchuyko-JDK-8215009-20181207-1625-13615: PASSED) >> >> -Dmitry >>
Hi, If the warning can be addressed with an extra in-line cast then that's cleaner and easier to understand than replicating that structure in 3 files. And of course, add a comment. To make the source more readable, the cast could be factored into a macro in the same file with the comment about why it is needed. Roger On 02/21/2019 11:07 PM, David Holmes wrote:
On 22/02/2019 4:55 am, Mikael Vidstedt wrote:
The wrapper based solution looks much cleaner to me as well. webrev.01 looks good.
Sorry I really don't like it. The wrappers obfuscate and make complicated something that is not at all complicated. I have to re-read the new code 3 times to figure out what it is even doing!
All that complexity to handle the fact one platform wants to return int instead of void* ??
David -----
(not a Reviewer)
Cheers, Mikael
On Feb 21, 2019, at 9:55 AM, Dmitry Chuyko <dmitry.chuyko@bell-sw.com> wrote:
To me thread function wrappers look preferable to platform specific JavaMain signature. Consider this webrev with wrappers:
http://cr.openjdk.java.net/~dchuyko/8215009/webrev.01/
In some cases JavaMain is called in the same thread and its result is returned from JLI_Launch. ContinueInNewThread is in shared code. And JavaMain uses macro controlled returns. So when JavaMain returns THREAD_FUNC_RETURN changes may contain some quite artificial macro parts in java.c:
http://cr.openjdk.java.net/~dchuyko/8215009/webrev.02/
-Dmitry
On 12/19/18 9:27 AM, David Holmes wrote:
On 19/12/2018 1:56 am, Dmitry Chuyko wrote:
On 12/18/18 3:39 AM, David Holmes wrote:
On 11/12/2018 9:30 pm, Dmitry Chuyko wrote: > On 12/11/18 4:03 AM, David Holmes wrote: >> Hi Dmitry, >> >> On 11/12/2018 12:16 am, Dmitry Chuyko wrote: >>> Hello, >>> >>> Please review a small fix in java_md_solinux.c: continuation >>> is not truly compatible with pthread_create start_routine's >>> signature but we control what actually happens. So it makes >>> sense to add intermediate void* cast to silence the error. >> >> I'd be tempted to fix the signature and get rid of all the casts. > > David, the signature is a signature of > > int JNICALL JavaMain(void * _args) > > It would be fun to change it. But still on Windows it is > correctly passed to _beginthreadex() and then return code is > extracted with GetExitCodeThread(). In case we want it to return > void* the cast will move there.
I think the current double cast is truly ugly and an ifdef for windows, or a cast for Windows only would be an improvement.
I agree. Maybe making a wrapper function is not so ugly. If there are no objections to changing beginning of the call stack it is quite easy to implement. For consistency it may be done for all 3 points (posix unix, posix mac, windows) or just for posix ones.
It looks like ifdef should be better as long as there are already OS-specific parts in libjli. Again, if there are no objections to have different JavaMain signatures on different platforms. In this case there won't be a signature cast for Windows.
How about setting
#define THREAD_FUNC_RETURN int
in windows/java_md.h.
Then:
#ifndef THREAD_FUNC_RETURN #define THREAD_FUNC_RETURN void* #endif
in java.h (after the other includes).
Then:
THREAD_FUNC_RETURN JNICALL JavaMain(void * _args)
in java.c.
?
Cheers, David
-Dmitry
But I won't impose that on you just to silence gcc 8.
Cheers, David
> -Dmitry > >> >> Cheers, >> David >> >>> bug: https://bugs.openjdk.java.net/browse/JDK-8215009 >>> webrev: http://cr.openjdk.java.net/~dchuyko/8215009/webrev.00/ >>> testing: submit repo >>> (mach5-one-dchuyko-JDK-8215009-20181207-1625-13615: PASSED) >>> >>> -Dmitry >>>
Let me preface by saying that I don’t have a strong opinion about this. Like, at all. The problem with an inline cast is that it’s not necessarily correct, because it all depends on the ABI. For example, consider this case: double JavaMain(void* args) { … return 47.11; } int ContinueInNewThread0(…) { pthread_t tid = pthread_create(…, (void (*)(void*))JavaMain, args); … void* tmp; pthread_join(tid, &tmp); // do something with tmp } The above code will “typically” not work, since a double isn’t returned in the same register as a void*. The added cast silences the warning, but the warning is there for a reason. Now, for most ABIs I can think of, void* and int are returned in the same register, so using a cast to silence the warning probably works for all platforms and ABIs we support today, and may well work for all platforms we ever end up supporting. Chances are also that if it doesn’t work we’ll also find pretty quickly. That said, I personally don’t like code that makes these implicit assumptions. Sooner or later it always has a tendency to bite back. Is there some way to make it more obvious what the wrapper code does perhaps? In the end though, I don’t feel strongly about this, so if the consensus is that the wrappers are that ugly and complex I’d rather add the cast and make progress on the toolchain upgrade. Cheers, Mikael
On Feb 22, 2019, at 7:32 AM, Roger Riggs <Roger.Riggs@Oracle.com> wrote:
Hi,
If the warning can be addressed with an extra in-line cast then that's cleaner and easier to understand than replicating that structure in 3 files. And of course, add a comment. To make the source more readable, the cast could be factored into a macro in the same file with the comment about why it is needed.
Roger
On 02/21/2019 11:07 PM, David Holmes wrote:
On 22/02/2019 4:55 am, Mikael Vidstedt wrote:
The wrapper based solution looks much cleaner to me as well. webrev.01 looks good.
Sorry I really don't like it. The wrappers obfuscate and make complicated something that is not at all complicated. I have to re-read the new code 3 times to figure out what it is even doing!
All that complexity to handle the fact one platform wants to return int instead of void* ??
David -----
(not a Reviewer)
Cheers, Mikael
On Feb 21, 2019, at 9:55 AM, Dmitry Chuyko <dmitry.chuyko@bell-sw.com> wrote:
To me thread function wrappers look preferable to platform specific JavaMain signature. Consider this webrev with wrappers:
http://cr.openjdk.java.net/~dchuyko/8215009/webrev.01/
In some cases JavaMain is called in the same thread and its result is returned from JLI_Launch. ContinueInNewThread is in shared code. And JavaMain uses macro controlled returns. So when JavaMain returns THREAD_FUNC_RETURN changes may contain some quite artificial macro parts in java.c:
http://cr.openjdk.java.net/~dchuyko/8215009/webrev.02/
-Dmitry
On 12/19/18 9:27 AM, David Holmes wrote:
On 19/12/2018 1:56 am, Dmitry Chuyko wrote:
On 12/18/18 3:39 AM, David Holmes wrote: > On 11/12/2018 9:30 pm, Dmitry Chuyko wrote: >> On 12/11/18 4:03 AM, David Holmes wrote: >>> Hi Dmitry, >>> >>> On 11/12/2018 12:16 am, Dmitry Chuyko wrote: >>>> Hello, >>>> >>>> Please review a small fix in java_md_solinux.c: continuation is not truly compatible with pthread_create start_routine's signature but we control what actually happens. So it makes sense to add intermediate void* cast to silence the error. >>> >>> I'd be tempted to fix the signature and get rid of all the casts. >> >> David, the signature is a signature of >> >> int JNICALL JavaMain(void * _args) >> >> It would be fun to change it. But still on Windows it is correctly passed to _beginthreadex() and then return code is extracted with GetExitCodeThread(). In case we want it to return void* the cast will move there. > > I think the current double cast is truly ugly and an ifdef for windows, or a cast for Windows only would be an improvement.
I agree. Maybe making a wrapper function is not so ugly. If there are no objections to changing beginning of the call stack it is quite easy to implement. For consistency it may be done for all 3 points (posix unix, posix mac, windows) or just for posix ones.
It looks like ifdef should be better as long as there are already OS-specific parts in libjli. Again, if there are no objections to have different JavaMain signatures on different platforms. In this case there won't be a signature cast for Windows.
How about setting
#define THREAD_FUNC_RETURN int
in windows/java_md.h.
Then:
#ifndef THREAD_FUNC_RETURN #define THREAD_FUNC_RETURN void* #endif
in java.h (after the other includes).
Then:
THREAD_FUNC_RETURN JNICALL JavaMain(void * _args)
in java.c.
?
Cheers, David
-Dmitry
> > But I won't impose that on you just to silence gcc 8. > > Cheers, > David > >> -Dmitry >> >>> >>> Cheers, >>> David >>> >>>> bug: https://bugs.openjdk.java.net/browse/JDK-8215009 >>>> webrev: http://cr.openjdk.java.net/~dchuyko/8215009/webrev.00/ >>>> testing: submit repo (mach5-one-dchuyko-JDK-8215009-20181207-1625-13615: PASSED) >>>> >>>> -Dmitry >>>>
Hi, After a closer look, I'd take the route of the 01 webrev. It is more localized and does not force the function signature needed by pthread_create to be propagated elsewhere. The code can be a lot more comprehensible by renaming the CallIntFunc function to be specific to ContinueInNewThread0. It looks like a trampoline to me. The data structure being passed is on the stack of the caller of pthread_create. It seems safe to refer to it here because the caller will wait in pthread_join. Also important is to document that the return type is specific to the OS and is needed to cast the return value expected by the start_pthread_create start_routine. That may still be in question because pthread_create expects void*. $.02, Roger On 02/22/2019 10:32 AM, Roger Riggs wrote:
Hi,
If the warning can be addressed with an extra in-line cast then that's cleaner and easier to understand than replicating that structure in 3 files. And of course, add a comment. To make the source more readable, the cast could be factored into a macro in the same file with the comment about why it is needed.
Roger
On 02/21/2019 11:07 PM, David Holmes wrote:
On 22/02/2019 4:55 am, Mikael Vidstedt wrote:
The wrapper based solution looks much cleaner to me as well. webrev.01 looks good.
Sorry I really don't like it. The wrappers obfuscate and make complicated something that is not at all complicated. I have to re-read the new code 3 times to figure out what it is even doing!
All that complexity to handle the fact one platform wants to return int instead of void* ?? The complexity is due to the function being called in some other thread context and there is a necessary cast/type conversion on the return value from the start_routine that has to come back through pthread to pthread_join.
David -----
(not a Reviewer)
Cheers, Mikael
On Feb 21, 2019, at 9:55 AM, Dmitry Chuyko <dmitry.chuyko@bell-sw.com> wrote:
To me thread function wrappers look preferable to platform specific JavaMain signature. Consider this webrev with wrappers:
http://cr.openjdk.java.net/~dchuyko/8215009/webrev.01/
In some cases JavaMain is called in the same thread and its result is returned from JLI_Launch. ContinueInNewThread is in shared code. And JavaMain uses macro controlled returns. So when JavaMain returns THREAD_FUNC_RETURN changes may contain some quite artificial macro parts in java.c:
http://cr.openjdk.java.net/~dchuyko/8215009/webrev.02/
-Dmitry
On 12/19/18 9:27 AM, David Holmes wrote:
On 19/12/2018 1:56 am, Dmitry Chuyko wrote:
On 12/18/18 3:39 AM, David Holmes wrote: > On 11/12/2018 9:30 pm, Dmitry Chuyko wrote: >> On 12/11/18 4:03 AM, David Holmes wrote: >>> Hi Dmitry, >>> >>> On 11/12/2018 12:16 am, Dmitry Chuyko wrote: >>>> Hello, >>>> >>>> Please review a small fix in java_md_solinux.c: continuation >>>> is not truly compatible with pthread_create start_routine's >>>> signature but we control what actually happens. So it makes >>>> sense to add intermediate void* cast to silence the error. >>> >>> I'd be tempted to fix the signature and get rid of all the casts. >> >> David, the signature is a signature of >> >> int JNICALL JavaMain(void * _args) >> >> It would be fun to change it. But still on Windows it is >> correctly passed to _beginthreadex() and then return code is >> extracted with GetExitCodeThread(). In case we want it to >> return void* the cast will move there. > > I think the current double cast is truly ugly and an ifdef for > windows, or a cast for Windows only would be an improvement.
I agree. Maybe making a wrapper function is not so ugly. If there are no objections to changing beginning of the call stack it is quite easy to implement. For consistency it may be done for all 3 points (posix unix, posix mac, windows) or just for posix ones.
It looks like ifdef should be better as long as there are already OS-specific parts in libjli. Again, if there are no objections to have different JavaMain signatures on different platforms. In this case there won't be a signature cast for Windows.
How about setting
#define THREAD_FUNC_RETURN int
in windows/java_md.h.
Then:
#ifndef THREAD_FUNC_RETURN #define THREAD_FUNC_RETURN void* #endif
in java.h (after the other includes).
Then:
THREAD_FUNC_RETURN JNICALL JavaMain(void * _args)
in java.c.
?
Cheers, David
-Dmitry
> > But I won't impose that on you just to silence gcc 8. > > Cheers, > David > >> -Dmitry >> >>> >>> Cheers, >>> David >>> >>>> bug: https://bugs.openjdk.java.net/browse/JDK-8215009 >>>> webrev: http://cr.openjdk.java.net/~dchuyko/8215009/webrev.00/ >>>> testing: submit repo >>>> (mach5-one-dchuyko-JDK-8215009-20181207-1625-13615: PASSED) >>>> >>>> -Dmitry >>>>
On 2/22/19 9:55 PM, Roger Riggs wrote:
Hi,
After a closer look, I'd take the route of the 01 webrev. It is more localized and does not force the function signature needed by pthread_create to be propagated elsewhere.
The code can be a lot more comprehensible by renaming the CallIntFunc function to be specific to ContinueInNewThread0. It looks like a trampoline to me. The data structure being passed is on the stack of the caller of pthread_create. It seems safe to refer to it here because the caller will wait in pthread_join.
After some hesitation it looks like ContinueInNewThread0 may know about JavaMain just like ContinueInNewThread, no need to work with abstract continuation. Even that abstract continuation is limited to int return type. In webrev.02 continuation gets platform specific signature. But then we have to cast the result where the call is direct. Another approach in that direction could be to add result field in JavaMainArgs, but it will again force ContinueInNewThread0 to know about continuation's parameters structure as there may be a direct call of continuation. If we let ContinueInNewThread0 call only JavaMain, it all can work without extra trampoline structures (just need a wrapper): http://cr.openjdk.java.net/~dchuyko/8215009/webrev.03/ -Dmitry
Also important is to document that the return type is specific to the OS and is needed to cast the return value expected by the start_pthread_create start_routine. That may still be in question because pthread_create expects void*.
$.02, Roger
On 02/22/2019 10:32 AM, Roger Riggs wrote:
Hi,
If the warning can be addressed with an extra in-line cast then that's cleaner and easier to understand than replicating that structure in 3 files. And of course, add a comment. To make the source more readable, the cast could be factored into a macro in the same file with the comment about why it is needed.
Roger
On 02/21/2019 11:07 PM, David Holmes wrote:
On 22/02/2019 4:55 am, Mikael Vidstedt wrote:
The wrapper based solution looks much cleaner to me as well. webrev.01 looks good.
Sorry I really don't like it. The wrappers obfuscate and make complicated something that is not at all complicated. I have to re-read the new code 3 times to figure out what it is even doing!
All that complexity to handle the fact one platform wants to return int instead of void* ?? The complexity is due to the function being called in some other thread context and there is a necessary cast/type conversion on the return value from the start_routine that has to come back through pthread to pthread_join.
David -----
(not a Reviewer)
Cheers, Mikael
On Feb 21, 2019, at 9:55 AM, Dmitry Chuyko <dmitry.chuyko@bell-sw.com> wrote:
To me thread function wrappers look preferable to platform specific JavaMain signature. Consider this webrev with wrappers:
http://cr.openjdk.java.net/~dchuyko/8215009/webrev.01/
In some cases JavaMain is called in the same thread and its result is returned from JLI_Launch. ContinueInNewThread is in shared code. And JavaMain uses macro controlled returns. So when JavaMain returns THREAD_FUNC_RETURN changes may contain some quite artificial macro parts in java.c:
http://cr.openjdk.java.net/~dchuyko/8215009/webrev.02/
-Dmitry
On 12/19/18 9:27 AM, David Holmes wrote:
On 19/12/2018 1:56 am, Dmitry Chuyko wrote: > On 12/18/18 3:39 AM, David Holmes wrote: >> On 11/12/2018 9:30 pm, Dmitry Chuyko wrote: >>> On 12/11/18 4:03 AM, David Holmes wrote: >>>> Hi Dmitry, >>>> >>>> On 11/12/2018 12:16 am, Dmitry Chuyko wrote: >>>>> Hello, >>>>> >>>>> Please review a small fix in java_md_solinux.c: continuation >>>>> is not truly compatible with pthread_create start_routine's >>>>> signature but we control what actually happens. So it makes >>>>> sense to add intermediate void* cast to silence the error. >>>> >>>> I'd be tempted to fix the signature and get rid of all the >>>> casts. >>> >>> David, the signature is a signature of >>> >>> int JNICALL JavaMain(void * _args) >>> >>> It would be fun to change it. But still on Windows it is >>> correctly passed to _beginthreadex() and then return code is >>> extracted with GetExitCodeThread(). In case we want it to >>> return void* the cast will move there. >> >> I think the current double cast is truly ugly and an ifdef for >> windows, or a cast for Windows only would be an improvement. > > I agree. Maybe making a wrapper function is not so ugly. If > there are no objections to changing beginning of the call stack > it is quite easy to implement. For consistency it may be done > for all 3 points (posix unix, posix mac, windows) or just for > posix ones. > > It looks like ifdef should be better as long as there are > already OS-specific parts in libjli. Again, if there are no > objections to have different JavaMain signatures on different > platforms. In this case there won't be a signature cast for > Windows.
How about setting
#define THREAD_FUNC_RETURN int
in windows/java_md.h.
Then:
#ifndef THREAD_FUNC_RETURN #define THREAD_FUNC_RETURN void* #endif
in java.h (after the other includes).
Then:
THREAD_FUNC_RETURN JNICALL JavaMain(void * _args)
in java.c.
?
Cheers, David
> > -Dmitry > >> >> But I won't impose that on you just to silence gcc 8. >> >> Cheers, >> David >> >>> -Dmitry >>> >>>> >>>> Cheers, >>>> David >>>> >>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8215009 >>>>> webrev: http://cr.openjdk.java.net/~dchuyko/8215009/webrev.00/ >>>>> testing: submit repo >>>>> (mach5-one-dchuyko-JDK-8215009-20181207-1625-13615: PASSED) >>>>> >>>>> -Dmitry >>>>>
On Feb 25, 2019, at 9:09 AM, Dmitry Chuyko <dmitry.chuyko@bell-sw.com> wrote:
On 2/22/19 9:55 PM, Roger Riggs wrote:
Hi,
After a closer look, I'd take the route of the 01 webrev. It is more localized and does not force the function signature needed by pthread_create to be propagated elsewhere.
The code can be a lot more comprehensible by renaming the CallIntFunc function to be specific to ContinueInNewThread0. It looks like a trampoline to me. The data structure being passed is on the stack of the caller of pthread_create. It seems safe to refer to it here because the caller will wait in pthread_join.
After some hesitation it looks like ContinueInNewThread0 may know about JavaMain just like ContinueInNewThread, no need to work with abstract continuation. Even that abstract continuation is limited to int return type. In webrev.02 continuation gets platform specific signature. But then we have to cast the result where the call is direct. Another approach in that direction could be to add result field in JavaMainArgs, but it will again force ContinueInNewThread0 to know about continuation's parameters structure as there may be a direct call of continuation.
If we let ContinueInNewThread0 call only JavaMain, it all can work without extra trampoline structures (just need a wrapper):
http://cr.openjdk.java.net/~dchuyko/8215009/webrev.03/ <http://cr.openjdk.java.net/~dchuyko/8215009/webrev.03/>
I like it! Since ContinueInNewThread0 is now always calling JavaMain I guess it could be renamed to CallJavaMainInNewThread (or something to that same effect). Minor nit: some consistency in where the ‘*’ is placed in the various “void *” places would be nice. Cheers, Mikael
-Dmitry
Also important is to document that the return type is specific to the OS and is needed to cast the return value expected by the start_pthread_create start_routine. That may still be in question because pthread_create expects void*.
$.02, Roger
On 02/22/2019 10:32 AM, Roger Riggs wrote:
Hi,
If the warning can be addressed with an extra in-line cast then that's cleaner and easier to understand than replicating that structure in 3 files. And of course, add a comment. To make the source more readable, the cast could be factored into a macro in the same file with the comment about why it is needed.
Roger
On 02/21/2019 11:07 PM, David Holmes wrote:
On 22/02/2019 4:55 am, Mikael Vidstedt wrote:
The wrapper based solution looks much cleaner to me as well. webrev.01 looks good.
Sorry I really don't like it. The wrappers obfuscate and make complicated something that is not at all complicated. I have to re-read the new code 3 times to figure out what it is even doing!
All that complexity to handle the fact one platform wants to return int instead of void* ?? The complexity is due to the function being called in some other thread context and there is a necessary cast/type conversion on the return value from the start_routine that has to come back through pthread to pthread_join.
David -----
(not a Reviewer)
Cheers, Mikael
On Feb 21, 2019, at 9:55 AM, Dmitry Chuyko <dmitry.chuyko@bell-sw.com> wrote:
To me thread function wrappers look preferable to platform specific JavaMain signature. Consider this webrev with wrappers:
http://cr.openjdk.java.net/~dchuyko/8215009/webrev.01/
In some cases JavaMain is called in the same thread and its result is returned from JLI_Launch. ContinueInNewThread is in shared code. And JavaMain uses macro controlled returns. So when JavaMain returns THREAD_FUNC_RETURN changes may contain some quite artificial macro parts in java.c:
http://cr.openjdk.java.net/~dchuyko/8215009/webrev.02/
-Dmitry
On 12/19/18 9:27 AM, David Holmes wrote: > On 19/12/2018 1:56 am, Dmitry Chuyko wrote: >> On 12/18/18 3:39 AM, David Holmes wrote: >>> On 11/12/2018 9:30 pm, Dmitry Chuyko wrote: >>>> On 12/11/18 4:03 AM, David Holmes wrote: >>>>> Hi Dmitry, >>>>> >>>>> On 11/12/2018 12:16 am, Dmitry Chuyko wrote: >>>>>> Hello, >>>>>> >>>>>> Please review a small fix in java_md_solinux.c: continuation is not truly compatible with pthread_create start_routine's signature but we control what actually happens. So it makes sense to add intermediate void* cast to silence the error. >>>>> >>>>> I'd be tempted to fix the signature and get rid of all the casts. >>>> >>>> David, the signature is a signature of >>>> >>>> int JNICALL JavaMain(void * _args) >>>> >>>> It would be fun to change it. But still on Windows it is correctly passed to _beginthreadex() and then return code is extracted with GetExitCodeThread(). In case we want it to return void* the cast will move there. >>> >>> I think the current double cast is truly ugly and an ifdef for windows, or a cast for Windows only would be an improvement. >> >> I agree. Maybe making a wrapper function is not so ugly. If there are no objections to changing beginning of the call stack it is quite easy to implement. For consistency it may be done for all 3 points (posix unix, posix mac, windows) or just for posix ones. >> >> It looks like ifdef should be better as long as there are already OS-specific parts in libjli. Again, if there are no objections to have different JavaMain signatures on different platforms. In this case there won't be a signature cast for Windows. > > How about setting > > #define THREAD_FUNC_RETURN int > > in windows/java_md.h. > > Then: > > #ifndef THREAD_FUNC_RETURN > #define THREAD_FUNC_RETURN void* > #endif > > in java.h (after the other includes). > > Then: > > THREAD_FUNC_RETURN JNICALL > JavaMain(void * _args) > > in java.c. > > ? > > Cheers, > David > > >> >> -Dmitry >> >>> >>> But I won't impose that on you just to silence gcc 8. >>> >>> Cheers, >>> David >>> >>>> -Dmitry >>>> >>>>> >>>>> Cheers, >>>>> David >>>>> >>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8215009 >>>>>> webrev: http://cr.openjdk.java.net/~dchuyko/8215009/webrev.00/ >>>>>> testing: submit repo (mach5-one-dchuyko-JDK-8215009-20181207-1625-13615: PASSED) >>>>>> >>>>>> -Dmitry >>>>>>
+1 Much cleaner, since it does not need to be more general. I'd probably add a comment to the ThreadJavaMain method to say why it is needed. BTW, it looks like the indents have gotten mixed up between 2 spaces and 4. For the libjli it should be 4 spaces. But this change is probably not the place to fix it and it is locally consistent. Thanks, Roger On 02/25/2019 01:16 PM, Mikael Vidstedt wrote:
On Feb 25, 2019, at 9:09 AM, Dmitry Chuyko <dmitry.chuyko@bell-sw.com> wrote:
On 2/22/19 9:55 PM, Roger Riggs wrote:
Hi,
After a closer look, I'd take the route of the 01 webrev. It is more localized and does not force the function signature needed by pthread_create to be propagated elsewhere.
The code can be a lot more comprehensible by renaming the CallIntFunc function to be specific to ContinueInNewThread0. It looks like a trampoline to me. The data structure being passed is on the stack of the caller of pthread_create. It seems safe to refer to it here because the caller will wait in pthread_join. After some hesitation it looks like ContinueInNewThread0 may know about JavaMain just like ContinueInNewThread, no need to work with abstract continuation. Even that abstract continuation is limited to int return type. In webrev.02 continuation gets platform specific signature. But then we have to cast the result where the call is direct. Another approach in that direction could be to add result field in JavaMainArgs, but it will again force ContinueInNewThread0 to know about continuation's parameters structure as there may be a direct call of continuation.
If we let ContinueInNewThread0 call only JavaMain, it all can work without extra trampoline structures (just need a wrapper):
http://cr.openjdk.java.net/~dchuyko/8215009/webrev.03/ <http://cr.openjdk.java.net/~dchuyko/8215009/webrev.03/> I like it! Since ContinueInNewThread0 is now always calling JavaMain I guess it could be renamed to CallJavaMainInNewThread (or something to that same effect).
I'm fine with the rename (no additional review necessary).
Minor nit: some consistency in where the ‘*’ is placed in the various “void *” places would be nice.
Cheers, Mikael
-Dmitry
Also important is to document that the return type is specific to the OS and is needed to cast the return value expected by the start_pthread_create start_routine. That may still be in question because pthread_create expects void*.
$.02, Roger
On 02/22/2019 10:32 AM, Roger Riggs wrote:
Hi,
If the warning can be addressed with an extra in-line cast then that's cleaner and easier to understand than replicating that structure in 3 files. And of course, add a comment. To make the source more readable, the cast could be factored into a macro in the same file with the comment about why it is needed.
Roger
On 02/21/2019 11:07 PM, David Holmes wrote:
On 22/02/2019 4:55 am, Mikael Vidstedt wrote:
The wrapper based solution looks much cleaner to me as well. webrev.01 looks good. Sorry I really don't like it. The wrappers obfuscate and make complicated something that is not at all complicated. I have to re-read the new code 3 times to figure out what it is even doing!
All that complexity to handle the fact one platform wants to return int instead of void* ?? The complexity is due to the function being called in some other thread context and there is a necessary cast/type conversion on the return value from the start_routine that has to come back through pthread to pthread_join.
David -----
(not a Reviewer)
Cheers, Mikael
> On Feb 21, 2019, at 9:55 AM, Dmitry Chuyko <dmitry.chuyko@bell-sw.com> wrote: > > To me thread function wrappers look preferable to platform specific JavaMain signature. Consider this webrev with wrappers: > > http://cr.openjdk.java.net/~dchuyko/8215009/webrev.01/ > > In some cases JavaMain is called in the same thread and its result is returned from JLI_Launch. ContinueInNewThread is in shared code. And JavaMain uses macro controlled returns. > So when JavaMain returns THREAD_FUNC_RETURN changes may contain some quite artificial macro parts in java.c: > > http://cr.openjdk.java.net/~dchuyko/8215009/webrev.02/ > > -Dmitry > > On 12/19/18 9:27 AM, David Holmes wrote: >> On 19/12/2018 1:56 am, Dmitry Chuyko wrote: >>> On 12/18/18 3:39 AM, David Holmes wrote: >>>> On 11/12/2018 9:30 pm, Dmitry Chuyko wrote: >>>>> On 12/11/18 4:03 AM, David Holmes wrote: >>>>>> Hi Dmitry, >>>>>> >>>>>> On 11/12/2018 12:16 am, Dmitry Chuyko wrote: >>>>>>> Hello, >>>>>>> >>>>>>> Please review a small fix in java_md_solinux.c: continuation is not truly compatible with pthread_create start_routine's signature but we control what actually happens. So it makes sense to add intermediate void* cast to silence the error. >>>>>> I'd be tempted to fix the signature and get rid of all the casts. >>>>> David, the signature is a signature of >>>>> >>>>> int JNICALL JavaMain(void * _args) >>>>> >>>>> It would be fun to change it. But still on Windows it is correctly passed to _beginthreadex() and then return code is extracted with GetExitCodeThread(). In case we want it to return void* the cast will move there. >>>> I think the current double cast is truly ugly and an ifdef for windows, or a cast for Windows only would be an improvement. >>> I agree. Maybe making a wrapper function is not so ugly. If there are no objections to changing beginning of the call stack it is quite easy to implement. For consistency it may be done for all 3 points (posix unix, posix mac, windows) or just for posix ones. >>> >>> It looks like ifdef should be better as long as there are already OS-specific parts in libjli. Again, if there are no objections to have different JavaMain signatures on different platforms. In this case there won't be a signature cast for Windows. >> How about setting >> >> #define THREAD_FUNC_RETURN int >> >> in windows/java_md.h. >> >> Then: >> >> #ifndef THREAD_FUNC_RETURN >> #define THREAD_FUNC_RETURN void* >> #endif >> >> in java.h (after the other includes). >> >> Then: >> >> THREAD_FUNC_RETURN JNICALL >> JavaMain(void * _args) >> >> in java.c. >> >> ? >> >> Cheers, >> David >> >> >>> -Dmitry >>> >>>> But I won't impose that on you just to silence gcc 8. >>>> >>>> Cheers, >>>> David >>>> >>>>> -Dmitry >>>>> >>>>>> Cheers, >>>>>> David >>>>>> >>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8215009 >>>>>>> webrev: http://cr.openjdk.java.net/~dchuyko/8215009/webrev.00/ >>>>>>> testing: submit repo (mach5-one-dchuyko-JDK-8215009-20181207-1625-13615: PASSED) >>>>>>> >>>>>>> -Dmitry >>>>>>>
I made mentioned cleanups in changed code, just in case here is a webrev without functional changes: function renaming, comments, indents (just a couple), void*. http://cr.openjdk.java.net/~dchuyko/8215009/webrev.04/ Started dev-submit for that patch. -Dmitry On 2/25/19 9:37 PM, Roger Riggs wrote:
+1
Much cleaner, since it does not need to be more general.
I'd probably add a comment to the ThreadJavaMain method to say why it is needed.
BTW, it looks like the indents have gotten mixed up between 2 spaces and 4. For the libjli it should be 4 spaces. But this change is probably not the place to fix it and it is locally consistent.
Thanks, Roger
On 02/25/2019 01:16 PM, Mikael Vidstedt wrote:
On Feb 25, 2019, at 9:09 AM, Dmitry Chuyko <dmitry.chuyko@bell-sw.com> wrote:
On 2/22/19 9:55 PM, Roger Riggs wrote:
Hi,
After a closer look, I'd take the route of the 01 webrev. It is more localized and does not force the function signature needed by pthread_create to be propagated elsewhere.
The code can be a lot more comprehensible by renaming the CallIntFunc function to be specific to ContinueInNewThread0. It looks like a trampoline to me. The data structure being passed is on the stack of the caller of pthread_create. It seems safe to refer to it here because the caller will wait in pthread_join. After some hesitation it looks like ContinueInNewThread0 may know about JavaMain just like ContinueInNewThread, no need to work with abstract continuation. Even that abstract continuation is limited to int return type. In webrev.02 continuation gets platform specific signature. But then we have to cast the result where the call is direct. Another approach in that direction could be to add result field in JavaMainArgs, but it will again force ContinueInNewThread0 to know about continuation's parameters structure as there may be a direct call of continuation.
If we let ContinueInNewThread0 call only JavaMain, it all can work without extra trampoline structures (just need a wrapper):
http://cr.openjdk.java.net/~dchuyko/8215009/webrev.03/ <http://cr.openjdk.java.net/~dchuyko/8215009/webrev.03/> I like it! Since ContinueInNewThread0 is now always calling JavaMain I guess it could be renamed to CallJavaMainInNewThread (or something to that same effect).
I'm fine with the rename (no additional review necessary).
Minor nit: some consistency in where the ‘*’ is placed in the various “void *” places would be nice.
Cheers, Mikael
-Dmitry
Also important is to document that the return type is specific to the OS and is needed to cast the return value expected by the start_pthread_create start_routine. That may still be in question because pthread_create expects void*.
$.02, Roger
On 02/22/2019 10:32 AM, Roger Riggs wrote:
Hi,
If the warning can be addressed with an extra in-line cast then that's cleaner and easier to understand than replicating that structure in 3 files. And of course, add a comment. To make the source more readable, the cast could be factored into a macro in the same file with the comment about why it is needed.
Roger
On 02/21/2019 11:07 PM, David Holmes wrote:
On 22/02/2019 4:55 am, Mikael Vidstedt wrote: > The wrapper based solution looks much cleaner to me as well. > webrev.01 looks good. Sorry I really don't like it. The wrappers obfuscate and make complicated something that is not at all complicated. I have to re-read the new code 3 times to figure out what it is even doing!
All that complexity to handle the fact one platform wants to return int instead of void* ?? The complexity is due to the function being called in some other thread context and there is a necessary cast/type conversion on the return value from the start_routine that has to come back through pthread to pthread_join.
David -----
> (not a Reviewer) > > Cheers, > Mikael > >> On Feb 21, 2019, at 9:55 AM, Dmitry Chuyko >> <dmitry.chuyko@bell-sw.com> wrote: >> >> To me thread function wrappers look preferable to platform >> specific JavaMain signature. Consider this webrev with wrappers: >> >> http://cr.openjdk.java.net/~dchuyko/8215009/webrev.01/ >> >> In some cases JavaMain is called in the same thread and its >> result is returned from JLI_Launch. ContinueInNewThread is in >> shared code. And JavaMain uses macro controlled returns. >> So when JavaMain returns THREAD_FUNC_RETURN changes may contain >> some quite artificial macro parts in java.c: >> >> http://cr.openjdk.java.net/~dchuyko/8215009/webrev.02/ >> >> -Dmitry >> >> On 12/19/18 9:27 AM, David Holmes wrote: >>> On 19/12/2018 1:56 am, Dmitry Chuyko wrote: >>>> On 12/18/18 3:39 AM, David Holmes wrote: >>>>> On 11/12/2018 9:30 pm, Dmitry Chuyko wrote: >>>>>> On 12/11/18 4:03 AM, David Holmes wrote: >>>>>>> Hi Dmitry, >>>>>>> >>>>>>> On 11/12/2018 12:16 am, Dmitry Chuyko wrote: >>>>>>>> Hello, >>>>>>>> >>>>>>>> Please review a small fix in java_md_solinux.c: >>>>>>>> continuation is not truly compatible with pthread_create >>>>>>>> start_routine's signature but we control what actually >>>>>>>> happens. So it makes sense to add intermediate void* cast >>>>>>>> to silence the error. >>>>>>> I'd be tempted to fix the signature and get rid of all the >>>>>>> casts. >>>>>> David, the signature is a signature of >>>>>> >>>>>> int JNICALL JavaMain(void * _args) >>>>>> >>>>>> It would be fun to change it. But still on Windows it is >>>>>> correctly passed to _beginthreadex() and then return code >>>>>> is extracted with GetExitCodeThread(). In case we want it >>>>>> to return void* the cast will move there. >>>>> I think the current double cast is truly ugly and an ifdef >>>>> for windows, or a cast for Windows only would be an >>>>> improvement. >>>> I agree. Maybe making a wrapper function is not so ugly. If >>>> there are no objections to changing beginning of the call >>>> stack it is quite easy to implement. For consistency it may >>>> be done for all 3 points (posix unix, posix mac, windows) or >>>> just for posix ones. >>>> >>>> It looks like ifdef should be better as long as there are >>>> already OS-specific parts in libjli. Again, if there are no >>>> objections to have different JavaMain signatures on different >>>> platforms. In this case there won't be a signature cast for >>>> Windows. >>> How about setting >>> >>> #define THREAD_FUNC_RETURN int >>> >>> in windows/java_md.h. >>> >>> Then: >>> >>> #ifndef THREAD_FUNC_RETURN >>> #define THREAD_FUNC_RETURN void* >>> #endif >>> >>> in java.h (after the other includes). >>> >>> Then: >>> >>> THREAD_FUNC_RETURN JNICALL >>> JavaMain(void * _args) >>> >>> in java.c. >>> >>> ? >>> >>> Cheers, >>> David >>> >>> >>>> -Dmitry >>>> >>>>> But I won't impose that on you just to silence gcc 8. >>>>> >>>>> Cheers, >>>>> David >>>>> >>>>>> -Dmitry >>>>>> >>>>>>> Cheers, >>>>>>> David >>>>>>> >>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8215009 >>>>>>>> webrev: >>>>>>>> http://cr.openjdk.java.net/~dchuyko/8215009/webrev.00/ >>>>>>>> testing: submit repo >>>>>>>> (mach5-one-dchuyko-JDK-8215009-20181207-1625-13615: PASSED) >>>>>>>> >>>>>>>> -Dmitry >>>>>>>>
Hi Dmitry, This seems fine to me too - much simpler. Thanks, David On 27/02/2019 6:23 am, Dmitry Chuyko wrote:
I made mentioned cleanups in changed code, just in case here is a webrev without functional changes: function renaming, comments, indents (just a couple), void*.
http://cr.openjdk.java.net/~dchuyko/8215009/webrev.04/
Started dev-submit for that patch.
-Dmitry
On 2/25/19 9:37 PM, Roger Riggs wrote:
+1
Much cleaner, since it does not need to be more general.
I'd probably add a comment to the ThreadJavaMain method to say why it is needed.
BTW, it looks like the indents have gotten mixed up between 2 spaces and 4. For the libjli it should be 4 spaces. But this change is probably not the place to fix it and it is locally consistent.
Thanks, Roger
On 02/25/2019 01:16 PM, Mikael Vidstedt wrote:
On Feb 25, 2019, at 9:09 AM, Dmitry Chuyko <dmitry.chuyko@bell-sw.com> wrote:
On 2/22/19 9:55 PM, Roger Riggs wrote:
Hi,
After a closer look, I'd take the route of the 01 webrev. It is more localized and does not force the function signature needed by pthread_create to be propagated elsewhere.
The code can be a lot more comprehensible by renaming the CallIntFunc function to be specific to ContinueInNewThread0. It looks like a trampoline to me. The data structure being passed is on the stack of the caller of pthread_create. It seems safe to refer to it here because the caller will wait in pthread_join. After some hesitation it looks like ContinueInNewThread0 may know about JavaMain just like ContinueInNewThread, no need to work with abstract continuation. Even that abstract continuation is limited to int return type. In webrev.02 continuation gets platform specific signature. But then we have to cast the result where the call is direct. Another approach in that direction could be to add result field in JavaMainArgs, but it will again force ContinueInNewThread0 to know about continuation's parameters structure as there may be a direct call of continuation.
If we let ContinueInNewThread0 call only JavaMain, it all can work without extra trampoline structures (just need a wrapper):
http://cr.openjdk.java.net/~dchuyko/8215009/webrev.03/ <http://cr.openjdk.java.net/~dchuyko/8215009/webrev.03/> I like it! Since ContinueInNewThread0 is now always calling JavaMain I guess it could be renamed to CallJavaMainInNewThread (or something to that same effect).
I'm fine with the rename (no additional review necessary).
Minor nit: some consistency in where the ‘*’ is placed in the various “void *” places would be nice.
Cheers, Mikael
-Dmitry
Also important is to document that the return type is specific to the OS and is needed to cast the return value expected by the start_pthread_create start_routine. That may still be in question because pthread_create expects void*.
$.02, Roger
On 02/22/2019 10:32 AM, Roger Riggs wrote:
Hi,
If the warning can be addressed with an extra in-line cast then that's cleaner and easier to understand than replicating that structure in 3 files. And of course, add a comment. To make the source more readable, the cast could be factored into a macro in the same file with the comment about why it is needed.
Roger
On 02/21/2019 11:07 PM, David Holmes wrote: > On 22/02/2019 4:55 am, Mikael Vidstedt wrote: >> The wrapper based solution looks much cleaner to me as well. >> webrev.01 looks good. > Sorry I really don't like it. The wrappers obfuscate and make > complicated something that is not at all complicated. I have to > re-read the new code 3 times to figure out what it is even doing! > > All that complexity to handle the fact one platform wants to > return int instead of void* ?? The complexity is due to the function being called in some other thread context and there is a necessary cast/type conversion on the return value from the start_routine that has to come back through pthread to pthread_join.
> David > ----- > >> (not a Reviewer) >> >> Cheers, >> Mikael >> >>> On Feb 21, 2019, at 9:55 AM, Dmitry Chuyko >>> <dmitry.chuyko@bell-sw.com> wrote: >>> >>> To me thread function wrappers look preferable to platform >>> specific JavaMain signature. Consider this webrev with wrappers: >>> >>> http://cr.openjdk.java.net/~dchuyko/8215009/webrev.01/ >>> >>> In some cases JavaMain is called in the same thread and its >>> result is returned from JLI_Launch. ContinueInNewThread is in >>> shared code. And JavaMain uses macro controlled returns. >>> So when JavaMain returns THREAD_FUNC_RETURN changes may contain >>> some quite artificial macro parts in java.c: >>> >>> http://cr.openjdk.java.net/~dchuyko/8215009/webrev.02/ >>> >>> -Dmitry >>> >>> On 12/19/18 9:27 AM, David Holmes wrote: >>>> On 19/12/2018 1:56 am, Dmitry Chuyko wrote: >>>>> On 12/18/18 3:39 AM, David Holmes wrote: >>>>>> On 11/12/2018 9:30 pm, Dmitry Chuyko wrote: >>>>>>> On 12/11/18 4:03 AM, David Holmes wrote: >>>>>>>> Hi Dmitry, >>>>>>>> >>>>>>>> On 11/12/2018 12:16 am, Dmitry Chuyko wrote: >>>>>>>>> Hello, >>>>>>>>> >>>>>>>>> Please review a small fix in java_md_solinux.c: >>>>>>>>> continuation is not truly compatible with pthread_create >>>>>>>>> start_routine's signature but we control what actually >>>>>>>>> happens. So it makes sense to add intermediate void* cast >>>>>>>>> to silence the error. >>>>>>>> I'd be tempted to fix the signature and get rid of all the >>>>>>>> casts. >>>>>>> David, the signature is a signature of >>>>>>> >>>>>>> int JNICALL JavaMain(void * _args) >>>>>>> >>>>>>> It would be fun to change it. But still on Windows it is >>>>>>> correctly passed to _beginthreadex() and then return code >>>>>>> is extracted with GetExitCodeThread(). In case we want it >>>>>>> to return void* the cast will move there. >>>>>> I think the current double cast is truly ugly and an ifdef >>>>>> for windows, or a cast for Windows only would be an >>>>>> improvement. >>>>> I agree. Maybe making a wrapper function is not so ugly. If >>>>> there are no objections to changing beginning of the call >>>>> stack it is quite easy to implement. For consistency it may >>>>> be done for all 3 points (posix unix, posix mac, windows) or >>>>> just for posix ones. >>>>> >>>>> It looks like ifdef should be better as long as there are >>>>> already OS-specific parts in libjli. Again, if there are no >>>>> objections to have different JavaMain signatures on different >>>>> platforms. In this case there won't be a signature cast for >>>>> Windows. >>>> How about setting >>>> >>>> #define THREAD_FUNC_RETURN int >>>> >>>> in windows/java_md.h. >>>> >>>> Then: >>>> >>>> #ifndef THREAD_FUNC_RETURN >>>> #define THREAD_FUNC_RETURN void* >>>> #endif >>>> >>>> in java.h (after the other includes). >>>> >>>> Then: >>>> >>>> THREAD_FUNC_RETURN JNICALL >>>> JavaMain(void * _args) >>>> >>>> in java.c. >>>> >>>> ? >>>> >>>> Cheers, >>>> David >>>> >>>> >>>>> -Dmitry >>>>> >>>>>> But I won't impose that on you just to silence gcc 8. >>>>>> >>>>>> Cheers, >>>>>> David >>>>>> >>>>>>> -Dmitry >>>>>>> >>>>>>>> Cheers, >>>>>>>> David >>>>>>>> >>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8215009 >>>>>>>>> webrev: >>>>>>>>> http://cr.openjdk.java.net/~dchuyko/8215009/webrev.00/ >>>>>>>>> testing: submit repo >>>>>>>>> (mach5-one-dchuyko-JDK-8215009-20181207-1625-13615: PASSED) >>>>>>>>> >>>>>>>>> -Dmitry >>>>>>>>>
Thanks for all the reviews. mach5-one-dchuyko-JDK-8215009-4-20190226-2029-850647: PASSED. Pushed. -Dmitry On 2/27/19 12:07 AM, David Holmes wrote:
Hi Dmitry,
This seems fine to me too - much simpler.
Thanks, David
On 27/02/2019 6:23 am, Dmitry Chuyko wrote:
I made mentioned cleanups in changed code, just in case here is a webrev without functional changes: function renaming, comments, indents (just a couple), void*.
http://cr.openjdk.java.net/~dchuyko/8215009/webrev.04/
Started dev-submit for that patch.
-Dmitry
On 2/25/19 9:37 PM, Roger Riggs wrote:
+1
Much cleaner, since it does not need to be more general.
I'd probably add a comment to the ThreadJavaMain method to say why it is needed.
BTW, it looks like the indents have gotten mixed up between 2 spaces and 4. For the libjli it should be 4 spaces. But this change is probably not the place to fix it and it is locally consistent.
Thanks, Roger
On 02/25/2019 01:16 PM, Mikael Vidstedt wrote:
On Feb 25, 2019, at 9:09 AM, Dmitry Chuyko <dmitry.chuyko@bell-sw.com> wrote:
On 2/22/19 9:55 PM, Roger Riggs wrote:
Hi,
After a closer look, I'd take the route of the 01 webrev. It is more localized and does not force the function signature needed by pthread_create to be propagated elsewhere.
The code can be a lot more comprehensible by renaming the CallIntFunc function to be specific to ContinueInNewThread0. It looks like a trampoline to me. The data structure being passed is on the stack of the caller of pthread_create. It seems safe to refer to it here because the caller will wait in pthread_join. After some hesitation it looks like ContinueInNewThread0 may know about JavaMain just like ContinueInNewThread, no need to work with abstract continuation. Even that abstract continuation is limited to int return type. In webrev.02 continuation gets platform specific signature. But then we have to cast the result where the call is direct. Another approach in that direction could be to add result field in JavaMainArgs, but it will again force ContinueInNewThread0 to know about continuation's parameters structure as there may be a direct call of continuation.
If we let ContinueInNewThread0 call only JavaMain, it all can work without extra trampoline structures (just need a wrapper):
http://cr.openjdk.java.net/~dchuyko/8215009/webrev.03/ <http://cr.openjdk.java.net/~dchuyko/8215009/webrev.03/> I like it! Since ContinueInNewThread0 is now always calling JavaMain I guess it could be renamed to CallJavaMainInNewThread (or something to that same effect).
I'm fine with the rename (no additional review necessary).
Minor nit: some consistency in where the ‘*’ is placed in the various “void *” places would be nice.
Cheers, Mikael
-Dmitry
Also important is to document that the return type is specific to the OS and is needed to cast the return value expected by the start_pthread_create start_routine. That may still be in question because pthread_create expects void*.
$.02, Roger
On 02/22/2019 10:32 AM, Roger Riggs wrote: > Hi, > > If the warning can be addressed with an extra in-line cast then > that's > cleaner and easier to understand than replicating that structure > in 3 files. > And of course, add a comment. > To make the source more readable, the cast could be factored > into a macro in the same file with the comment about why it is > needed. > > Roger > > > On 02/21/2019 11:07 PM, David Holmes wrote: >> On 22/02/2019 4:55 am, Mikael Vidstedt wrote: >>> The wrapper based solution looks much cleaner to me as well. >>> webrev.01 looks good. >> Sorry I really don't like it. The wrappers obfuscate and make >> complicated something that is not at all complicated. I have to >> re-read the new code 3 times to figure out what it is even doing! >> >> All that complexity to handle the fact one platform wants to >> return int instead of void* ?? The complexity is due to the function being called in some other thread context and there is a necessary cast/type conversion on the return value from the start_routine that has to come back through pthread to pthread_join.
>> David >> ----- >> >>> (not a Reviewer) >>> >>> Cheers, >>> Mikael >>> >>>> On Feb 21, 2019, at 9:55 AM, Dmitry Chuyko >>>> <dmitry.chuyko@bell-sw.com> wrote: >>>> >>>> To me thread function wrappers look preferable to platform >>>> specific JavaMain signature. Consider this webrev with wrappers: >>>> >>>> http://cr.openjdk.java.net/~dchuyko/8215009/webrev.01/ >>>> >>>> In some cases JavaMain is called in the same thread and its >>>> result is returned from JLI_Launch. ContinueInNewThread is in >>>> shared code. And JavaMain uses macro controlled returns. >>>> So when JavaMain returns THREAD_FUNC_RETURN changes may >>>> contain some quite artificial macro parts in java.c: >>>> >>>> http://cr.openjdk.java.net/~dchuyko/8215009/webrev.02/ >>>> >>>> -Dmitry >>>> >>>> On 12/19/18 9:27 AM, David Holmes wrote: >>>>> On 19/12/2018 1:56 am, Dmitry Chuyko wrote: >>>>>> On 12/18/18 3:39 AM, David Holmes wrote: >>>>>>> On 11/12/2018 9:30 pm, Dmitry Chuyko wrote: >>>>>>>> On 12/11/18 4:03 AM, David Holmes wrote: >>>>>>>>> Hi Dmitry, >>>>>>>>> >>>>>>>>> On 11/12/2018 12:16 am, Dmitry Chuyko wrote: >>>>>>>>>> Hello, >>>>>>>>>> >>>>>>>>>> Please review a small fix in java_md_solinux.c: >>>>>>>>>> continuation is not truly compatible with >>>>>>>>>> pthread_create start_routine's signature but we control >>>>>>>>>> what actually happens. So it makes sense to add >>>>>>>>>> intermediate void* cast to silence the error. >>>>>>>>> I'd be tempted to fix the signature and get rid of all >>>>>>>>> the casts. >>>>>>>> David, the signature is a signature of >>>>>>>> >>>>>>>> int JNICALL JavaMain(void * _args) >>>>>>>> >>>>>>>> It would be fun to change it. But still on Windows it is >>>>>>>> correctly passed to _beginthreadex() and then return code >>>>>>>> is extracted with GetExitCodeThread(). In case we want it >>>>>>>> to return void* the cast will move there. >>>>>>> I think the current double cast is truly ugly and an ifdef >>>>>>> for windows, or a cast for Windows only would be an >>>>>>> improvement. >>>>>> I agree. Maybe making a wrapper function is not so ugly. If >>>>>> there are no objections to changing beginning of the call >>>>>> stack it is quite easy to implement. For consistency it may >>>>>> be done for all 3 points (posix unix, posix mac, windows) >>>>>> or just for posix ones. >>>>>> >>>>>> It looks like ifdef should be better as long as there are >>>>>> already OS-specific parts in libjli. Again, if there are no >>>>>> objections to have different JavaMain signatures on >>>>>> different platforms. In this case there won't be a >>>>>> signature cast for Windows. >>>>> How about setting >>>>> >>>>> #define THREAD_FUNC_RETURN int >>>>> >>>>> in windows/java_md.h. >>>>> >>>>> Then: >>>>> >>>>> #ifndef THREAD_FUNC_RETURN >>>>> #define THREAD_FUNC_RETURN void* >>>>> #endif >>>>> >>>>> in java.h (after the other includes). >>>>> >>>>> Then: >>>>> >>>>> THREAD_FUNC_RETURN JNICALL >>>>> JavaMain(void * _args) >>>>> >>>>> in java.c. >>>>> >>>>> ? >>>>> >>>>> Cheers, >>>>> David >>>>> >>>>> >>>>>> -Dmitry >>>>>> >>>>>>> But I won't impose that on you just to silence gcc 8. >>>>>>> >>>>>>> Cheers, >>>>>>> David >>>>>>> >>>>>>>> -Dmitry >>>>>>>> >>>>>>>>> Cheers, >>>>>>>>> David >>>>>>>>> >>>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8215009 >>>>>>>>>> webrev: >>>>>>>>>> http://cr.openjdk.java.net/~dchuyko/8215009/webrev.00/ >>>>>>>>>> testing: submit repo >>>>>>>>>> (mach5-one-dchuyko-JDK-8215009-20181207-1625-13615: >>>>>>>>>> PASSED) >>>>>>>>>> >>>>>>>>>> -Dmitry >>>>>>>>>>
participants (4)
-
David Holmes
-
Dmitry Chuyko
-
Mikael Vidstedt
-
Roger Riggs