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

Dmitry Chuyko dmitry.chuyko at bell-sw.com
Tue Feb 26 20:23:25 UTC 2019 

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