RFR: 8280476: [macOS] : hotspot arm64 bug exposed by latest clang

[Andrew Dinn]

On Thu, 3 Feb 2022 04:32:48 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> @adinn - Thanks for chiming in on this review thread.
>> 
>> I'm reading and re-reading your comments and it will take me a while
>> to put it all in proper context.
>> 
>> I do agree with one of your points:
>> 
>>> `replicate` is never actually called with `count == 0`
>> 
>> so that means that this:
>> 
>>     `assert(count <= 1, "must be");`
>> 
>> is a little too loose and should change to:
>> 
>>     `assert(count == 1, "must be");`
>> 
>> if we're going to keep it at all. I think we should keep it, but
>> I can be convinced otherwise since the original code had no
>> similar `assert()`.
>> 
>> @kimbarrett - I've re-read your comments and I'm not clear
>> on whether you think that `result <<= nbits` is UB when
>> `nbits == 64`. Can you please clarify?
>> 
>> Of course, it's not helping matters that I can't reproduce the
>> failure mode reported by Apple and we're getting no response
>> about our request for version info.
>
> It's definitely UB according to the language.  Then there's the question of what the hardware might actually implement in the cases where the compiler doesn't recognize it and the relevant instruction(s) are executed with that value.  I know I've seen hardware that used the shift quantity mod word-size.  I think I've seen hardware that would just shift out all the bits, leaving a value of 0.  One reason for the language to make this UB is because of hardware variance.  Oh, and I *think* I've seen hardware where it was UB and would produce "strange" results.

> I do agree with one of your points:
>
> > replicate is never actually called with count == 0
>
> so that means that this:
>
> >     assert(count <= 1, "must be");
>
> is a little too loose and should change to:
>
> >     assert(count == 1, "must be");
>
> if we're going to keep it at all. I think we should keep it, but
> I can be convinced otherwise since the original code had no similar assert().

That is entirely reasonable given that replicate is only meant to be used locally and none of these local use cases will ever try to replicate 0 copies of a bit field.

We could actually add one or both of these more general asserts at entry to `replicate` since they apply to all cases, not just when `nbits == 64`:

    assert(count > 0, "must be")
    assert(count * nbits <= 64, "must be")

In other words it is an error to try to replicate zero copies of a bit field and it is an error to try to replicate more copies than can fit into 64 bits.

Note that the second assert removes the need for the current assert and combining it with the first assert also enforces your recommended alternative.

We could also add another general assert

    assert(nbits > 0, "must be")

i.e. it is an error to try to replicate an empty bit field.

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

PR: https://git.openjdk.java.net/jdk/pull/7270