RFR: 8145096: Undefined behaviour in HotSpot

[Andrew Haley]

On 17/12/15 01:07, Kim Barrett wrote:
> On Dec 15, 2015, at 10:14 AM, Andrew Haley <aph at redhat.com> wrote:
>>
>> On 12/10/2015 09:44 PM, Kim Barrett wrote:
>>> src/os/posix/vm/os_posix.cpp
>>> 756     unsigned int i;
>>>
>>> I'm not sure what the purpose of changing this to unsigned might be.
>>> The sa_flags member of struct sigaction is of type int.
>>
>> There are assignments into this field that overflow: SA_RESETHAND
>> (0x80000000) is implicitly unsigned, so assigning it to an int field
>> is at best implementation-define.  I suppose that this is really a bug
>> in the Linux headers.  I'll take it out.
> 
> Ick, that's really annoying.  It does seem like a bug in the Linux
> headers, with a poorly chosen value for SA_RESETHAND.  Or in POSIX,
> for using a signed type for a bitmask.  I suppose asking you to fix
> this root cause would be a bit much. ;-)

We have to work with old kernels, so it wouldn't help for a while.

> Now that I understand it, and given its very localized scope, I would
> be fine with the original change to unsigned.  That would prevent
> future verification/analysis from tripping over the same problem.  My
> only concern is that someone reading this who is familiar with POSIX
> might wonder why an unsigned type is being used; maybe a brief comment?
> Up to you.

OK.

>>> 136   return (double)(method->rate() + 1) * ((double)(method->invocation_count() + 1) * (method->backedge_count() + 1));
>>>
>>> I suspect the problem encountered here is that the multiplication of
>>> the two count values can overflow.  If that multiplication was not
>>> grouped, but instead the rate term and invocation count were
>>> multiplied, then multiplied by the backedge count (e.g. use the
>>> natural grouping for operator*), the result should be mathematically
>>> the same, and I think should not run into overflow problems.  Recall
>>> that method->rate() returns float.
>>
>> I guess this would be okay, but it seems prudent to use a double.  Is
>> there some special performance optimization we have to care about?
>> The return type of AdvancedThresholdPolicy::weight() is double, so
>> I really can't think of any good reason not to do the arithmetic as
>> double.
>>>
>>> So I think a simpler change here might be
>>>
>>>  return (method->rate() + 1) * (method->invocation_count() + 1) * (method->backedge_count() + 1);
>>>
>>> i.e. remove the parens that group the count multiplication before
>>> multiplying by the rate.
> 
> The only use of weight() is about 10 lines later in the same file, in
> compare_methods.  It seems unlikely that the extra bits of precision
> from computing in double vs float is going to matter in any
> interesting way there.  Certainly it can't be worse than the
> "substantially noise" version we've been using!

I'm trying not to break anything.  It's not precision I'm worried
about, it's overflow.  I assumed that the double return type was there
for a reason, given that the existing code very carefully casts to a
double.  If you tell me that there is no reason for that cast I'll
happily do the whole function as float.

> I've now looked at the opto changes too.  They mostly look ok to me.
> 
> ------------------------------------------------------------------------------ 
> src/share/vm/opto/mulnode.cpp
>  805   const jlong bits_mask = java_subtract((jlong)CONST64(1) << (jlong)(BitsPerJavaLong - con), CONST64(1));
> 
> Overflow in a left shift of a signed value is UB.

Argh.  I am aware of that, but IMVHO it's a rather pedantic rule: it
disallows simple and obvious expressions such as -16 << 2.  (What for?
Sign-and-absolute-magnitude machines?)  However, them's the rules, I
guess.

I'm sure that it makes no sense ever to construct bitmasks using a
signed type.  I am doing my best to be certain that I don't break
anything.  I'll have a look to see if there is some way that I can
change this to unsigned without changing behaviour.

Andrew.