RFR: 8145096: Undefined behaviour in HotSpot

[Kim Barrett]

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. ;-)

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.

>> 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!

My preference is to avoid casts (especially C-style casts), preferably
by arranging for types to match up properly.  I suggest that the
unnecessary grouping be eliminated and then either (1) change the
return type for weight to float, to match the computation, (2) cast
the rate to double, or (3) do neither, performing the computation in
float with a final implicit conversion to double.  (2) is probably
*my* least preferred, but any of these would do for me.

Since (2) is what is in the 8145096-2, I’ll leave it to you to decide
whether you want to use either of the other options or leave it as is.

> http://cr.openjdk.java.net/~aph/8145096-2/

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.  I think this
expression needs more revision than just dealing with the possible
underflow in the subtraction, which can't be reached without already
having UB when con == 1.  [con == 0 is handled earlier.]

Similarly here:
1257   const jlong mask = java_subtract(((jlong)CONST64(1) << (jlong)(BitsPerJavaLong - con)), (jlong)1);

------------------------------------------------------------------------------ 
src/share/vm/opto/type.cpp
3217     java_add(java_add(const_oop() ? const_oop()->hash() : 0, _klass_is_exact),
3218               java_add(_instance_id, TypePtr::hash()));

Unaligned arguments for the outer java_add.

------------------------------------------------------------------------------