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

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-core-libs-dev
Subject:    Re: RFR(XS): 8215009: GCC 8 compilation eror in libjli
From:       Dmitry Chuyko <dmitry.chuyko () bell-sw ! com>
Date:       2019-02-27 10:17:08
Message-ID: f5f9e2c0-e064-bb58-e658-d5b502a29030 () bell-sw ! com
[Download RAW message or body]

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 
>>>>> <dmitry.chuyko@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@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
>>>>>>>>>>>>>>>>
>>>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic