> On Feb 25, 2019, at 9:09 AM, Dmitry Chuyko <dmitry.chu...@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).
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.chu...@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
>>>>>>>>>>>>
>>>>>
>>>
>>
