Thread: More weird compiler warnings
serinus' experimental gcc whines about a few places in network.c:
../../../../../pgsql/src/backend/utils/adt/network.c: In function 'inetnot':
../../../../../pgsql/src/backend/utils/adt/network.c:1893:34: warning: writing 1 byte into a region of size 0
[-Wstringop-overflow=]
1893 | pdst[nb] = ~pip[nb];
| ~~~~~~~~~^~~~~~~~~~
../../../../../pgsql/src/include/utils/inet.h:27:23: note: at offset -1 into destination object 'ipaddr' of size 16
27 | unsigned char ipaddr[16]; /* up to 128 bits of address */
| ^~~~~~
../../../../../pgsql/src/include/utils/inet.h:27:23: note: at offset -1 into destination object 'ipaddr' of size 16
The code in question looks like
{
int nb = ip_addrsize(ip);
unsigned char *pip = ip_addr(ip);
unsigned char *pdst = ip_addr(dst);
while (nb-- > 0)
pdst[nb] = ~pip[nb];
}
There's nothing actually wrong with this, but I'm wondering if
we could silence the warning by changing the loop condition to
while (--nb >= 0)
which seems like it might be marginally more readable anyway.
regards, tom lane
Hi,
On 2022-03-26 16:23:26 -0400, Tom Lane wrote:
> serinus' experimental gcc whines about a few places in network.c:
>
> ../../../../../pgsql/src/backend/utils/adt/network.c: In function 'inetnot':
> ../../../../../pgsql/src/backend/utils/adt/network.c:1893:34: warning: writing 1 byte into a region of size 0
[-Wstringop-overflow=]
> 1893 | pdst[nb] = ~pip[nb];
> | ~~~~~~~~~^~~~~~~~~~
> ../../../../../pgsql/src/include/utils/inet.h:27:23: note: at offset -1 into destination object 'ipaddr' of size 16
> 27 | unsigned char ipaddr[16]; /* up to 128 bits of address */
> | ^~~~~~
> ../../../../../pgsql/src/include/utils/inet.h:27:23: note: at offset -1 into destination object 'ipaddr' of size 16
>
> The code in question looks like
>
> {
> int nb = ip_addrsize(ip);
> unsigned char *pip = ip_addr(ip);
> unsigned char *pdst = ip_addr(dst);
>
> while (nb-- > 0)
> pdst[nb] = ~pip[nb];
> }
>
> There's nothing actually wrong with this
I reported this to the gcc folks, that's clearly a bug. I suspect that it
might not just cause spurious warnings, but also code generation issues - but
I don't know that part for sure.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104986
> but I'm wondering if we could silence the warning by changing the loop condition to
>
> while (--nb >= 0)
>
> which seems like it might be marginally more readable anyway.
Yes, that looks like it silences it. I modified the small reproducer I had in
that bug (https://godbolt.org/z/ejK9h6von) and the warning vanishes.
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes: > On 2022-03-26 16:23:26 -0400, Tom Lane wrote: >> serinus' experimental gcc whines about a few places in network.c: > I reported this to the gcc folks, that's clearly a bug. I suspect that it > might not just cause spurious warnings, but also code generation issues - but > I don't know that part for sure. > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104986 Hmm, looks like the gcc folk aren't too sure either ;-). But yeah, given the discussion so far it's plausible there could be actually bad code emitted. >> but I'm wondering if we could silence the warning by changing the loop condition to >> while (--nb >= 0) >> which seems like it might be marginally more readable anyway. > Yes, that looks like it silences it. I modified the small reproducer I had in > that bug (https://godbolt.org/z/ejK9h6von) and the warning vanishes. Okay, so we can change this code, or just do nothing and wait for a repaired gcc. Since that's an unreleased version there's no concern about any possible bug in-the-wild. I think it probably should come down to whether we think the predecrement form is indeed more readable. I'm about +0.1 towards changing, what do you think? regards, tom lane
Hi, On 2022-03-26 17:04:16 -0400, Tom Lane wrote: > Hmm, looks like the gcc folk aren't too sure either ;-). Heh, yea ;) > Okay, so we can change this code, or just do nothing and wait for > a repaired gcc. Since that's an unreleased version there's no > concern about any possible bug in-the-wild. I think it probably > should come down to whether we think the predecrement form is > indeed more readable. Agreed. > I'm about +0.1 towards changing, what do you think? Similar. Greetings, Andres Freund
Hi,
On 2022-03-26 13:55:49 -0700, Andres Freund wrote:
> On 2022-03-26 16:23:26 -0400, Tom Lane wrote:
> > serinus' experimental gcc whines about a few places in network.c:
> >
> > ../../../../../pgsql/src/backend/utils/adt/network.c: In function 'inetnot':
> > ../../../../../pgsql/src/backend/utils/adt/network.c:1893:34: warning: writing 1 byte into a region of size 0
[-Wstringop-overflow=]
> > 1893 | pdst[nb] = ~pip[nb];
> > | ~~~~~~~~~^~~~~~~~~~
> > ../../../../../pgsql/src/include/utils/inet.h:27:23: note: at offset -1 into destination object 'ipaddr' of size
16
> > 27 | unsigned char ipaddr[16]; /* up to 128 bits of address */
> > | ^~~~~~
> > ../../../../../pgsql/src/include/utils/inet.h:27:23: note: at offset -1 into destination object 'ipaddr' of size
16
> >
> > The code in question looks like
> >
> > {
> > int nb = ip_addrsize(ip);
> > unsigned char *pip = ip_addr(ip);
> > unsigned char *pdst = ip_addr(dst);
> >
> > while (nb-- > 0)
> > pdst[nb] = ~pip[nb];
> > }
> >
> > There's nothing actually wrong with this
>
> I reported this to the gcc folks, that's clearly a bug. I suspect that it
> might not just cause spurious warnings, but also code generation issues - but
> I don't know that part for sure.
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104986
>
>
> > but I'm wondering if we could silence the warning by changing the loop condition to
> >
> > while (--nb >= 0)
> >
> > which seems like it might be marginally more readable anyway.
>
> Yes, that looks like it silences it. I modified the small reproducer I had in
> that bug (https://godbolt.org/z/ejK9h6von) and the warning vanishes.
The recent discussion about warnings reminded me of this. Given the gcc bug
hasn't been fixed, I think we should make that change. I'd vote for
backpatching it as well - what do you think?
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes: > On 2022-03-26 13:55:49 -0700, Andres Freund wrote: >> On 2022-03-26 16:23:26 -0400, Tom Lane wrote: >>> but I'm wondering if we could silence the warning by changing the loop condition to >>> while (--nb >= 0) >>> which seems like it might be marginally more readable anyway. >> Yes, that looks like it silences it. I modified the small reproducer I had in >> that bug (https://godbolt.org/z/ejK9h6von) and the warning vanishes. > The recent discussion about warnings reminded me of this. Given the gcc bug > hasn't been fixed, I think we should make that change. I'd vote for > backpatching it as well - what do you think? +1, can't hurt anything AFAICS. regards, tom lane
On 2023-03-16 14:31:37 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2022-03-26 13:55:49 -0700, Andres Freund wrote: > >> On 2022-03-26 16:23:26 -0400, Tom Lane wrote: > >>> but I'm wondering if we could silence the warning by changing the loop condition to > >>> while (--nb >= 0) > >>> which seems like it might be marginally more readable anyway. > > >> Yes, that looks like it silences it. I modified the small reproducer I had in > >> that bug (https://godbolt.org/z/ejK9h6von) and the warning vanishes. > > > The recent discussion about warnings reminded me of this. Given the gcc bug > > hasn't been fixed, I think we should make that change. I'd vote for > > backpatching it as well - what do you think? > > +1, can't hurt anything AFAICS. Done.