Re: RFR(XS): 8215009: GCC 8 compilation eror in libjli

[Dmitry Chuyko]

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 
 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/ 

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 
 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 

Re: RFR(XS): 8215009: GCC 8 compilation eror in libjli

[David Holmes]

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 
 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/ 

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 
 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 

Re: RFR(XS): 8215009: GCC 8 compilation eror in libjli

[Dmitry Chuyko]

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 
 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/ 

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 
 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 signatu

Re: RFR(XS): 8215009: GCC 8 compilation eror in libjli

[Roger Riggs]

+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  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/ 


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

Re: RFR(XS): 8215009: GCC 8 compilation eror in libjli

[Mikael Vidstedt]

> On Feb 25, 2019, at 9:09 AM, Dmitry Chuyko  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/ 
> 

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  
>> 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 st

Re: RFR(XS): 8215009: GCC 8 compilation eror in libjli

[Dmitry Chuyko]

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 
 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

Re: RFR(XS): 8215009: GCC 8 compilation eror in libjli

[Roger Riggs]

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 
 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

Re: RFR(XS): 8215009: GCC 8 compilation eror in libjli

[Mikael Vidstedt]

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  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  
 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 wi

Re: RFR(XS): 8215009: GCC 8 compilation eror in libjli

[Roger Riggs]

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 
 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

Re: RFR(XS): 8215009: GCC 8 compilation eror in libjli

[David Holmes]

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  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

Re: RFR(XS): 8215009: GCC 8 compilation eror in libjli

[Mikael Vidstedt]

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  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
>>> 

Re: RFR(XS): 8215009: GCC 8 compilation eror in libjli

[Dmitry Chuyko]

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

Re: RFR(XS): 8215009: GCC 8 compilation eror in libjli

[David Holmes]

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

Re: RFR(XS): 8215009: GCC 8 compilation eror in libjli

[Dmitry Chuyko]

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

Re: RFR(XS): 8215009: GCC 8 compilation eror in libjli

[David Holmes]

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

Re: RFR(XS): 8215009: GCC 8 compilation eror in libjli

[Dmitry Chuyko]

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

Re: RFR(XS): 8215009: GCC 8 compilation eror in libjli

[David Holmes]

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

RFR(XS): 8215009: GCC 8 compilation eror in libjli

[Dmitry Chuyko]

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