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