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.chu...@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
