8066103: C2's range check smearing allows out of bound array accesses

[John Rose]

That's good; let's do it that way!

Final suggestion (really):  Add some more test cases, containing duplicate array indexes, to exercise the "local" logic. 

– John

> On Dec 8, 2014, at 5:30 AM, Roland Westrelin <roland.westrelin at oracle.com> wrote:
> 
> Hi John,
> 
> Thanks for the suggestions. They sound very reasonable to me. Here is a new webrev:
> 
> http://cr.openjdk.java.net/~roland/8066103/webrev.03/
> 
> In the patch below:
> 
>> Or better yet a purely local cutout.  (This sketch has a bad goto; the value of 'prev_dom' carried by the goto is what I called 'most_recent_prev_check' above.)
>> 
>> diff --git a/src/share/vm/opto/ifnode.cpp b/src/share/vm/opto/ifnode.cpp
>> --- a/src/share/vm/opto/ifnode.cpp
>> +++ b/src/share/vm/opto/ifnode.cpp
>> @@ -876,6 +876,7 @@
>>    // Low and high offsets seen so far
>>    jint off_lo = offset1;
>>    jint off_hi = offset1;
>> +    int nb_checks = 0;
>> 
>>    // Scan for the top 2 checks and collect range of offsets
>>    for( int dist = 0; dist < 999; dist++ ) { // Range-Check scan limit
>> @@ -890,6 +891,15 @@
>>        // the same array bounds.
>>        if( flip2 == flip1 && range2 == range1 && index2 == index1 &&
>>            dom->outcnt() == 2 ) {
>> +          if (nb_checks == 0 && dom->in(1) == in(1)) {
>> +            // Found an immediately dominating test at the same offset.
>> +            // This kind of back-to-back test can be eliminated locally,
>> +            // and there is no need to search further for dominating tests.
>> +            assert(offset2 == offset1, "");
>> +            prev_dom = dom;
>> +            goto got_prev_dom;  // FIXME: unstructured control flow
>> +          }
>> +          nb_checks++;
>>          // Gather expanded bounds
>>          off_lo = MIN2(off_lo,offset2);
>>          off_hi = MAX2(off_hi,offset2);
>> @@ -985,6 +995,7 @@
>> 
>>  } // End of Else scan for an equivalent test
>> 
>> + got_prev_dom:  // FIXME: unstructured control flow
>>  // Hit!  Remove this IF
>> #ifndef PRODUCT
>>  if( TraceIterativeGVN ) {
> 
> I don’t think you need prev_dom. prev_dom is the projection, dom is the If node. IfNode::dominated_by needs the projection as input.
> 
> Roland.
>