RFR (XS) : 8141641: Runtime: implement range for ErrorLogTimeout

[Dmitry Dmitriev]

Hi Gerard,

See my response in-line:

On 16.11.2015 18:59, gerard ziemski wrote:
> Thank you David and Dmitry for reviews. Please see my response in-line:
>
>
>>> 3) Also, David suggested to wrap constants in multiplication in
>>> src/share/vm/runtime/thread.cpp into CONST64 to avoid possible
>>> overflows, i.e.:
>>> os::sleep(this, ErrorLogTimeout * CONST64(1000), false);
>
> I must have missed something - where/when did David suggest doing this?
Probably, you missed these e-mail's... I reply on David e-mail at 11 
November an he replied to me with this suggestion.
My e-mail: 
http://mail.openjdk.java.net/pipermail/hotspot-dev/2015-November/020603.html
David answer: 
http://mail.openjdk.java.net/pipermail/hotspot-dev/2015-November/020605.html
>
>
>> I'm making a bit of a hash of this review - sorry.
>>
>> I agree with Dmitry that the os::sleep call should be changed as above:
>>
>> os::sleep(this, ErrorLogTimeout * CONST64(1000), false);
>>
>> but as the sleep arg is a jlong we need to limit ErrorLogTimeout to 
>> (MAX_JLONG/1000) _but_ that's too big for uintx on
>> 32-bit. Which means the true range is different on 32-bit versus 
>> 64-bit. So if I finally get this right we should have
>> the range as:
>>
>> range(0, LP64_ONLY(MAX_JLONG/1000) NOT_LP64(UINTX_MAX))
>>
>> Not sure we have MAX_JLONG defined as a constant. I don't agree with 
>> just selecting the 32-bit range as that again
>> becomes an arbitrary constraint on 64-bit.
>
> Since os:sleep() takes jlong (8 bytes long on both 32 and 64 bit), 
> them it would appear that the most straightforward solution is to 
> change ErrorLogTimeout type from "uintx" to "uint64_t" (also 8 bytes 
> on both 32 and 64 bit) and then we can have the range simply defined as:
>
> range(0, max_jlong/1000)
Yes, this seems as alternative aproach!
>
> And then we can have:
>
> os::sleep(this, ErrorLogTimeout * CONST64(1000), false); // in seconds
>
> Do we agree?
>
>
> cheers

Thanks,
Dmitry