RFR: 8145096: Undefined behaviour in HotSpot

[Andrew Haley]

On 12/10/2015 09:44 PM, Kim Barrett wrote:
> On Dec 10, 2015, at 8:30 AM, Andrew Haley <aph at redhat.com> wrote:
>
> 
>> http://cr.openjdk.java.net/~aph/8145096-1/
>>
> 
> ------------------------------------------------------------------------------ 
> 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.

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

> Note that I would not like to see these java_xxx functions used as a
> general "solution" to overflow problems.  Most cases of signed
> integer arithmetic overflow are actual bugs, and papering over them
> with these functions would be a mistake.  I haven't reviewed them
> carefully, but the way these are being used in the various changes
> in the opto directory look appropriate, in order to match Java's
> defined behavior.  But I wouldn't be surprised if there were other
> places (like the problem found in advancedThresholdPolicy.cpp) where
> I would not want these operations used.  In fact, I'd like the
> comment describing these operations to say something along that
> line.

OK.

Andrew.