Horton

Thread

Commits

  1. Avoid shadowing a variable in sync.c.

  1. Fix gcc warning in sync.c (usr/src/backend/storage/sync/sync.c)

    Ranier Vilela <ranier.vf@gmail.com> — 2022-07-10T00:53:31Z

    Hi,

In one of my compilations of Postgres, I noted this warning from gcc.

gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Werror=vla -Wendif-labels
-Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type
-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard
-Wno-format-truncation -Wno-stringop-truncation -O2
-I../../../../src/include  -D_GNU_SOURCE   -c -o sync.o sync.c
sync.c: In function ‘RememberSyncRequest’:
sync.c:528:10: warning: assignment to ‘PendingFsyncEntry *’ {aka ‘struct
<anonymous> *’} from incompatible pointer type ‘PendingUnlinkEntry *’ {aka
‘struct <anonymous> *’} [-Wincompatible-pointer-types]
  528 |    entry = (PendingUnlinkEntry *) lfirst(cell);

Although the structures are identical, gcc bothers to assign a pointer from
one to the other.

typedef struct
{
FileTag tag; /* identifies handler and file */
CycleCtr cycle_ctr; /* sync_cycle_ctr of oldest request */
bool canceled; /* canceled is true if we canceled "recently" */
} PendingFsyncEntry;

typedef struct
{
FileTag tag; /* identifies handler and file */
CycleCtr cycle_ctr; /* checkpoint_cycle_ctr when request was made */
bool canceled; /* true if request has been canceled */
} PendingUnlinkEntry;

The patch tries to fix this.

regards,
Ranier Vilela

  2. Re: Fix gcc warning in sync.c (usr/src/backend/storage/sync/sync.c)

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-07-11T05:35:00Z

    At Sat, 9 Jul 2022 21:53:31 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in 
> sync.c: In function ‘RememberSyncRequest’:
> sync.c:528:10: warning: assignment to ‘PendingFsyncEntry *’ {aka ‘struct
> <anonymous> *’} from incompatible pointer type ‘PendingUnlinkEntry *’ {aka
> ‘struct <anonymous> *’} [-Wincompatible-pointer-types]
>   528 |    entry = (PendingUnlinkEntry *) lfirst(cell);
> 
> Although the structures are identical, gcc bothers to assign a pointer from
> one to the other.

If the entry were of really PendingSyncEntry, it would need a fix, but
at the same time everyone should see the same warning at their hand.

Actually, I already see the following line (maybe) at the place instead.

529@master,REL14, 508@REL13
>		PendingUnlinkEntry *entry = (PendingUnlinkEntry *) lfirst(cell);


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  3. Re: Fix gcc warning in sync.c (usr/src/backend/storage/sync/sync.c)

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-11T05:45:16Z

    [ cc'ing Thomas, whose code this seems to be ]

Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> At Sat, 9 Jul 2022 21:53:31 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in 
>> sync.c: In function ¡RememberSyncRequest¢:
>> sync.c:528:10: warning: assignment to ¡PendingFsyncEntry *¢ {aka ¡struct
>> <anonymous> *¢} from incompatible pointer type ¡PendingUnlinkEntry *¢ {aka
>> ¡struct <anonymous> *¢} [-Wincompatible-pointer-types]
>> 528 |    entry = (PendingUnlinkEntry *) lfirst(cell);

> Actually, I already see the following line (maybe) at the place instead.
>> PendingUnlinkEntry *entry = (PendingUnlinkEntry *) lfirst(cell);

Yeah, I see no line matching that in HEAD either.

However, I do not much like the code at line 528, because its
"PendingUnlinkEntry *entry" is masking an outer variable
"PendingFsyncEntry *entry" from line 513.  We should rename
one or both variables to avoid that masking.

			regards, tom lane

  4. Re: Fix gcc warning in sync.c (usr/src/backend/storage/sync/sync.c)

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-07-11T09:45:07Z

    At Mon, 11 Jul 2022 01:45:16 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> [ cc'ing Thomas, whose code this seems to be ]
> 
> Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> > At Sat, 9 Jul 2022 21:53:31 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in 
> >> 528 |    entry = (PendingUnlinkEntry *) lfirst(cell);
> 
> > Actually, I already see the following line (maybe) at the place instead.
> >> PendingUnlinkEntry *entry = (PendingUnlinkEntry *) lfirst(cell);
> 
> Yeah, I see no line matching that in HEAD either.
> 
> However, I do not much like the code at line 528, because its
> "PendingUnlinkEntry *entry" is masking an outer variable
> "PendingFsyncEntry *entry" from line 513.  We should rename
> one or both variables to avoid that masking.

I thought the same at the moment looking this.  In this case, changing
entry->syncent, unl(del)lent works. But at the same time I don't think
that can be strictly applied.

So, for starters, I compiled the whole tree with -Wshadow=local. and I
saw many warnings with it.  At a glance all of them are reasonably
"fixed" but I don't think it is what we want...

Thoughts?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  5. Re: Fix gcc warning in sync.c (usr/src/backend/storage/sync/sync.c)

    Thomas Munro <thomas.munro@gmail.com> — 2022-07-14T12:11:15Z

    On Mon, Jul 11, 2022 at 9:45 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> At Mon, 11 Jul 2022 01:45:16 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in
> > Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> > > At Sat, 9 Jul 2022 21:53:31 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in
> > >> 528 |    entry = (PendingUnlinkEntry *) lfirst(cell);
> >
> > > Actually, I already see the following line (maybe) at the place instead.
> > >> PendingUnlinkEntry *entry = (PendingUnlinkEntry *) lfirst(cell);
> >
> > Yeah, I see no line matching that in HEAD either.

Confusing report :-)

> > However, I do not much like the code at line 528, because its
> > "PendingUnlinkEntry *entry" is masking an outer variable
> > "PendingFsyncEntry *entry" from line 513.  We should rename
> > one or both variables to avoid that masking.

Fair point.

> I thought the same at the moment looking this.  In this case, changing
> entry->syncent, unl(del)lent works. But at the same time I don't think
> that can be strictly applied.

Yeah, let's rename both of them.  Done.

> So, for starters, I compiled the whole tree with -Wshadow=local. and I
> saw many warnings with it.  At a glance all of them are reasonably
> "fixed" but I don't think it is what we want...

Wow, yeah.

  6. Re: Fix gcc warning in sync.c (usr/src/backend/storage/sync/sync.c)

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-07-21T18:01:43Z

    On 2022-Jul-15, Thomas Munro wrote:

> On Mon, Jul 11, 2022 at 9:45 PM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:

> > So, for starters, I compiled the whole tree with -Wshadow=local. and I
> > saw many warnings with it.  At a glance all of them are reasonably
> > "fixed" but I don't think it is what we want...
> 
> Wow, yeah.

Previous threads on this topic:

https://postgr.es/m/MN2PR18MB2927F7B5F690065E1194B258E35D0@MN2PR18MB2927.namprd18.prod.outlook.com
https://postgr.es/m/CAApHDvpqBR7u9yzW4yggjG=QfN=FZsc8Wo2ckokpQtif-+iQ2A@mail.gmail.com
https://postgr.es/m/877k1psmpf.fsf@mailbox.samurai.com

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
Thou shalt study thy libraries and strive not to reinvent them without
cause, that thy code may be short and readable and thy days pleasant
and productive. (7th Commandment for C Programmers)