RFR: 8145096: Undefined behaviour in HotSpot
aph at redhat.com
Thu Dec 17 10:31:21 UTC 2015
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:
>>> 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.
>>> 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
>>> 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.
> 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
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.
More information about the hotspot-dev