Let me preface by saying that I don’t have a strong opinion about this. Like,
at all.
The problem with an inline cast is that it’s not necessarily correct, because
it all depends on the ABI. For example, consider this case:
double JavaMain(void* args) {
…
return 47.11;
}
int
ContinueInNewThread0(…) {
pthread_t tid = pthread_create(…, (void (*)(void*))JavaMain, args);
…
void* tmp;
pthread_join(tid, &tmp);
// do something with tmp
}
The above code will “typically” not work, since a double isn’t returned in the
same register as a void*. The added cast silences the warning, but the warning
is there for a reason.
Now, for most ABIs I can think of, void* and int are returned in the same
register, so using a cast to silence the warning probably works for all
platforms and ABIs we support today, and may well work for all platforms we
ever end up supporting. Chances are also that if it doesn’t work we’ll also
find pretty quickly.
That said, I personally don’t like code that makes these implicit assumptions.
Sooner or later it always has a tendency to bite back.
Is there some way to make it more obvious what the wrapper code does perhaps?
In the end though, I don’t feel strongly about this, so if the consensus is
that the wrappers are that ugly and complex I’d rather add the cast and make
progress on the toolchain upgrade.
Cheers,
Mikael
> On Feb 22, 2019, at 7:32 AM, Roger Riggs <roger.ri...@oracle.com> 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* ??
>>
>> 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
>>>>>>>>>>
>>>
>
