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 
>>>> <dmitry.chuyko at 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
>>>>>>>>>>
>>>
>