[gfortran,patch] Fix undue out-of-bounds warning (PR30655)

[Brooks Moses]

FX Coudert wrote:
> This fixes an out-of-bounds warning that was wrongly issued at  
> compile-time. The code that checked the dimensions, expanded and  
> rewritten by yours truly last year, made the stupid mistake of  
> assuming that if a comparison comp was such that (comp != CMP_LT),  
> then comp was either CMP_EQ or CMP_GT. It so happens that there is a  
> CMP_UNKNOWN for cases where one of the operands is not constant. The  
> patch is simply:
>      1. changing this back to (comp == CPM_EQ || comp == CMP_GT),
>      2. storing the often used comparison result in a variable,
>      3. reindent the whole thing correctly, which results  
> (unfortunately) in an almost unreadable diff.
> 
> This patch is in fact very simple, so don't let put back by the diff!

I have some concerns about the following piece of the patch, which 
appears to be unchanged except for the indentation:

--------------------------------------
  	if (compare_bound (AR_START, AR_END) == CMP_EQ
  	    && (compare_bound (AR_START, as->lower[i]) == CMP_LT
  	        || compare_bound (AR_START, as->upper[i]) == CMP_GT))
  	  goto bound;
--------------------------------------

First, the compare_bound call on the first line can be changed to 
"comp_start_end == CMP_EQ", yes?

Second, this seems to be partly redundant with the clause that you 
changed, which is (in its changed form):
--------------------------------------
+	if (((compare_bound_int (ar->stride[i], 0) == CMP_GT
+	      || ar->stride[i] == NULL)
+	     && (comp_start_end == CMP_LT || comp_start_end == CMP_EQ))
+	    || (compare_bound_int (ar->stride[i], 0) == CMP_LT
+	        && (comp_start_end == CMP_GT || comp_start_end == CMP_EQ)))
	  {
	    if (compare_bound (AR_START, as->lower[i]) == CMP_LT)
	      goto bound;
	    if (compare_bound (AR_START, as->upper[i]) == CMP_GT)
	      goto bound;
	  }
--------------------------------------
It appears to me that this will fire if comp_start_end == CMP_EQ and 
either compare_bound_int (ar->stride[i], 0) == CMP_GT or CMP_LT, or 
ar->stride[i] == NULL.  I don't see any other possibilities other than 
the comparison equaling CMP_EQ, in which case we'll have already thrown 
an error.

Is it likewise possible for comp_start_end to get CMP_UNKNOWN?  I 
presume that it can, but if it can't do that, then the first clause 
appears to me to be entirely redundant, and can be removed.

On the other hand, if comp_start_end can get CMP_UNKNOWN, then both 
clauses are necessary.  However, in that case, the "comp_start_end == 
CMP_EQ" parts of the second clause are redundant, as that situation has 
already been tested.  I think it would be considerably clearer to 
rearrange and combine both of the clauses thusly:

    if (comp_start_end == CMP_EQ
        || (comp_start_end == CMP_LT
            && (compare_bound_int (ar->stride[i], 0) == CMP_GT
	       || ar->stride[i] == NULL))
        || (comp_start_end == CMP_GT
            && compare_bound_int (ar->stride[i], 0) == CMP_LT))
     {
       if (compare_bound (AR_START, as->lower[i]) == CMP_LT)
         goto bound;
       if (compare_bound (AR_START, as->upper[i]) == CMP_GT)
         goto bound;
     }

(Note that I haven't gotten the indentation right there.)

Also, on the body of these clauses -- this one has the two compare_bound 
lines in separate if-statements, but the subsequent check for the last 
value of the triplet has them in the same if-statement connected by an 
"||".  It seems to me that this should be done consistently one way or 
the other.

Finally, I think that this bit of code could really stand to have some 
brief comments, along the lines of "Check that the starting index of the 
section is within range, but only if the array section is non-empty" for 
the above-quoted block, and something similar for the check for the last 
value of the triplet.

> Bootstrapped and regtested some time ago on i686-linux, currently  
> rebootstrapping and reregtesting on x86_64-linux. OK for mainline and  
> 4.2?

OK for mainline, with the above changes (assuming that you agree with my 
logic!).

Also ok for 4.2 if this is fixing a regression.  If it isn't, I think 
I'd like to ask for a second opinion on that (or feel free to email Mark 
and lobby for including it).

- Brooks