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

[coleen.phillimore at oracle.com]

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.

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
>