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

[coleen.phillimore at oracle.com]

Thanks Harold!
Coleen

On 1/11/18 3:07 PM, harold seigel wrote:
> 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
>>>>>
>>>>
>>
>