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

David Holmes david.holmes at oracle.com
Tue Feb 26 21:07:34 UTC 2019 

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

More information about the core-libs-dev mailing list