Horton

Thread

Commits

  1. Work around spurious compiler warning in inet operators

  1. More weird compiler warnings

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-26T20:23:26Z

    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

  2. Re: More weird compiler warnings

    Andres Freund <andres@anarazel.de> — 2022-03-26T20:55:49Z

    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

  3. Re: More weird compiler warnings

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-26T21:04:16Z

    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

  4. Re: More weird compiler warnings

    Andres Freund <andres@anarazel.de> — 2022-03-26T21:26:29Z

    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

  5. Re: More weird compiler warnings

    Andres Freund <andres@anarazel.de> — 2023-03-16T17:18:46Z

    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

  6. Re: More weird compiler warnings

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-03-16T18:31:37Z

    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

  7. Re: More weird compiler warnings

    Andres Freund <andres@anarazel.de> — 2023-03-16T21:53:30Z

    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.