Thread: Fix gcc warning in sync.c (usr/src/backend/storage/sync/sync.c)
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);
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;
{
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
Attachment
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
[ 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
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
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.
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)