RFR (S) 8130039: Move the platform-specific [OS]Semaphore code

[harold seigel]

Hi Coleen,

This change looks good.

Thanks, Harold

On 1/10/2018 7:51 AM, coleen.phillimore at oracle.com wrote:
>
>
> On 1/10/18 12:11 AM, David Holmes wrote:
>> Hi Coleen,
>>
>> On 10/01/2018 3:17 AM, coleen.phillimore at oracle.com wrote:
>>> On 1/9/18 8:14 AM, coleen.phillimore at oracle.com wrote:
>>>>
>>>>
>>>> On 1/8/18 8:56 PM, David Holmes wrote:
>>>>> Hi Coleen,
>>>>>
>>>>> Overall this seems okay but a few comments ...
>>>>>
>>>>> On 9/01/2018 8:34 AM, coleen.phillimore at oracle.com wrote:
>>>>>> Also:
>>>>>> 8130038: Unify the semaphore usage in os_xxx.cpp
>>>>>>
>>>>>> Tested with hs-tier1-5.  And 
>>>>>> open/test/jdk/sun/misc/SunMiscSignalTest.java and closed signal 
>>>>>> tests.
>>>>>>
>>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8130039.01/webrev
>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8130039
>>>>>
>>>>> With the removal of os::signal_lookup() the parameter to 
>>>>> check_pending_signals becomes irrelevant and the "false" path 
>>>>> becomes dead code. Not sure if it is worth deleting that.
>>>>
>>>> Yes, that's true.  I can delete it and the code for the "false" path.
>>>>>
>>>>> ---
>>>>>
>>>>> os_linux.cpp:
>>>>>
>>>>> static struct timespec create_timespec(unsigned int sec, int nsec) {
>>>>>   struct timespec ts;
>>>>>   // Semaphore's are always associated with CLOCK_REALTIME
>>>>>   os::Linux::clock_gettime(CLOCK_REALTIME, &ts);
>>>>>   // see unpackTime for discussion on overflow checking
>>>>>
>>>>> There's no longer a direct connection between create_timespec and 
>>>>> the fact it is only used with semaphores - so the initial comment 
>>>>> is no longer appropriate. But if you rename to 
>>>>> create_semaphore_timespec then that restores the connection.
>>>>>
>>>>
>>>> Ok, I'll rename it.
>>>>
>>>>> The final comment is not correct (my bad!) as unpackTime no longer 
>>>>> exists since I factored out all the PlatformEvent code. It could say:
>>>>>
>>>>> // see os_posix.cpp for discussion on overflow checking
>>>>
>>>> I see, it's now similar to calc_rel_time in os_posix.cpp. I'll 
>>>> change the comment as suggested in case calc_rel_time changes we 
>>>> don't have to change this.
>>>>>
>>>>> but I'm wondering whether the timedwait API should be taking a 
>>>>> timespec in the first place? If we pass secs and nsecs directly 
>>>>> then we only need to convert to a timespec inside the low-level 
>>>>> implementation that needs it ie in semaphore_posix.cpp. I don't 
>>>>> recall if there was a reason we have to do the timespec conversion 
>>>>> in a platform specific manner.
>>>>>
>>>>
>>>> I didn't understand the difference between solaris and linux 
>>>> create_timespec, so I left it in the platform specific code. The 
>>>> solaris implementation *does* have unpackTime and the linux one 
>>>> does not.  Or could I use the linux version in semaphore_posix.cpp 
>>>> ? That would be better, but I wasn't sure if it was correct.   Let 
>>>> me know. I'd like to make that change.
>>>
>>> No, looking again, the linux version of create_semaphore_timespec is 
>>> competely different and I don't think I can unify these at this time.
>>
>> Well this is quite a mess :( The Solaris unpackTime code is 
>> technically incorrect if Solaris is using posix semaphores because it 
>> reads the time from gettimeofday, but the manpage for sem_timedwait 
>> clearly states the timespec should be from CLOCK_REALTIME. So in 
>> actuality what we currently have in the Linux code is what the shared 
>> POSIX code should look like and what Solaris should actually use.
>>
>> Further, the non-OSX BSD code purports to use PosixSemaphore but it 
>> defines:
>>
>> struct timespec PosixSemaphore::create_timespec(unsigned int sec, int 
>> nsec) {
>>   struct timespec ts;
>>   unpackTime(&ts, false, (sec * NANOSECS_PER_SEC) + nsec);
>>
>>   return ts;
>> }
>>
>> but there is no unpackTime method on BSD!
>
> Right, there is no unpackTime on BSD so I removed this for BSD !APPLE 
> semaphores, since nothing that we have compiles this, and anything 
> that needs it has more work to do.
>
> Yes, please file an RFE.  I might get to it but there was a 
> complication for posix because it also needs to get this clock_gettime 
> function:  os::Linux::clock_gettime(CLOCK_REALTIME, &ts);  We're never 
> done, but hopefully my change is a start.
>
> Thanks,
> Coleen
>>
>> I think we need a separate RFE to clean up this mess. I'll file one.
>>
>> Thanks,
>> David
>>
>>> thanks,
>>> Coleen
>>>>> ---
>>>>>
>>>>> semaphore_posix.cpp
>>>>>
>>>>>   26 #ifndef __APPLE__
>>>>>   27 #include "runtime/os.hpp"
>>>>>   28 // POSIX unamed semaphores are not supported on OS X.
>>>>>   29 #include "semaphore_posix.hpp"
>>>>>   30 #include <semaphore.h>
>>>>>   31 #endif
>>>>>   32
>>>>>   33
>>>>>   34 // POSIX unamed semaphores are not supported on OS X.
>>>>>   35 #ifndef __APPLE__
>>>>>
>>>>> The whole file contents should be ifndef __APPLE__ so this just 
>>>>> reduces to:
>>>>>
>>>>>   // POSIX unamed semaphores are not supported on OS X.
>>>>>   #ifndef __APPLE__
>>>>>   #include "runtime/os.hpp"
>>>>>   #include "semaphore_posix.hpp"
>>>>>   #include <semaphore.h>
>>>>>   ...
>>>>>
>>>>
>>>> Ok.  I'll fix that.
>>>>
>>>> Thanks,
>>>> Coleen
>>>>> ---
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>
>>>
>