| Message ID | 20141216134432.GY30928@spoyarek.pnq.redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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;