More weird compiler warnings

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.