Fix 'array subscript is above array bounds' warning in res_send.c

  On Tue, Dec 16, 2014 at 11:12:24AM -0200, Adhemerval Zanella wrote:
> In this loop 'ns' is initialized to '0' and updated on a simple while with
> 2 constraints.  Someone with more compiler background could correct me, but
> I don't think this is really hard to compile evaluate that will fall
> in 0 <= ns < MAXNS in all cases.

Oh I see it now - N is initialized to EXT(statp).nscount, not NS :/

I agree that this is a compiler issue.  Here's a patch that undoes the
change I made and adds DIAG_IGNORE_NEEDS_COMMENT instead.  Does this
look OK?

Siddhesh

	* resolv/res_send.c (__libc_res_nsend): Disable warning 'array
	subscript above bounds'.
  

On 16-12-2014 11:44, Siddhesh Poyarekar wrote:
> On Tue, Dec 16, 2014 at 11:12:24AM -0200, Adhemerval Zanella wrote:
>> In this loop 'ns' is initialized to '0' and updated on a simple while with
>> 2 constraints.  Someone with more compiler background could correct me, but
>> I don't think this is really hard to compile evaluate that will fall
>> in 0 <= ns < MAXNS in all cases.
> Oh I see it now - N is initialized to EXT(statp).nscount, not NS :/
>
> I agree that this is a compiler issue.  Here's a patch that undoes the
> change I made and adds DIAG_IGNORE_NEEDS_COMMENT instead.  Does this
> look OK?
Looks ok, thanks!
>
> Siddhesh
>
> 	* resolv/res_send.c (__libc_res_nsend): Disable warning 'array
> 	subscript above bounds'.
>
> diff --git a/resolv/res_send.c b/resolv/res_send.c
> index 5a9882c..c35fb66 100644
> --- a/resolv/res_send.c
> +++ b/resolv/res_send.c
> @@ -429,9 +429,15 @@ __libc_res_nsend(res_state statp, const u_char *buf, int buflen,
>  				while (ns < MAXNS
>  				       && EXT(statp).nsmap[ns] != MAXNS)
>  					ns++;
> -				if (ns >= MAXNS)
> +				if (ns == MAXNS)
>  					break;
> +				/* NS never exceeds MAXNS, but gcc 4.9 somehow
> +				   does not see this.  */
> +				DIAG_PUSH_NEEDS_COMMENT;
> +				DIAG_IGNORE_NEEDS_COMMENT (4.9,
> +							   "-Warray-bounds");
>  				EXT(statp).nsmap[ns] = n;
> +				DIAG_POP_NEEDS_COMMENT;
>  				map[n] = ns++;
>  			}
>  		EXT(statp).nscount = n;
  

When changing the source code does not actually cause any de-optimization
of the generated code, then it seems cleaner to change the code rather than
to suppress the warning.  In this case, changing the condition will change
what conditional-branch instruction is emitted, but it won't change the
number (or size) of instructions emitted or change anything that would
affect performance.  It doesn't matter a lot either way.  When the compiler
is being really dumb as appears to be the case here, then a substantial
comment about the compiler's stupidity is warranted; that makes it be even
less of a difference in cognitive load and source clutter between the code
change and the warning suppression.  But as to the general rule/policy,
changing the source code just to make the compiler happy is not the thing
we avoid: it's changes that de-optimize the generated code.
  

On 12/16/2014 10:05 AM, Roland McGrath wrote:
> changing the source code just to make the compiler happy is not the thing
> we avoid: it's changes that de-optimize the generated code.

For what it's worth, we use a similar rule in Gnulib and core GNU apps.

That being said, I can't resist mentioning that Dijkstra was a fan using 
the 'i == n' test as opposed to the 'i >= n' test in cases like these, 
so I expect he would not have favored this change. Dijkstra's argument 
was that if 'i == n' is correct, then using 'i >= n' will make the code 
more robust in the presence of programming errors elsewhere, which is 
not what you should want: you should want your program to crash nicely 
(not limp along) in the presence of these other errors.  Those were the 
days, eh?
  

On Tue, Dec 16, 2014 at 10:05:36AM -0800, Roland McGrath wrote:
> When changing the source code does not actually cause any de-optimization
> of the generated code, then it seems cleaner to change the code rather than
> to suppress the warning.  In this case, changing the condition will change
> what conditional-branch instruction is emitted, but it won't change the
> number (or size) of instructions emitted or change anything that would
> affect performance. 

That is false, could change code as compiler thinks that branches with
equality are less probable than with inequality and decide to optimize
branch for size only where there is equality.

A simplest example is look on icc assembly as it also writes guessed
probabilities.

icc -O3 test.c -S

test.c:

int foo(int x)
{
  return (x == 42) ? sin (32.0) : cos (14.3);
}

test.s:

        je        ..B1.4        # Prob 16%                      #4.16

test.c:

int foo(int x) 
{
  return (x >= 42) ? sin (32.0) : cos (14.3);
}

test.s:

       jl        ..B1.3        # Prob 50%                      #4.16