Обсуждение: ancient sequence point bug

This code in bootstrap.c contains a sequence point violation (or
whatever that is really called):
       while ((tup = heap_getnext(scan, ForwardScanDirection)) != NULL)       {           (*app)->am_oid =
HeapTupleGetOid(tup);          memmove((char *) &(*app++)->am_typ,                   (char *) GETSTRUCT(tup),
       sizeof((*app)->am_typ));       }
 

In commit 1aebc361, another place in the same file was fixed like this:

@@ -445,13 +455,13 @@ struct typmap       while (HeapTupleIsValid(tup = heap_getnext(scan, 0)))       {
(*app)->am_oid= tup->t_data->t_oid;
 
-           memmove((char *) &(*app++)->am_typ,
-                   (char *) GETSTRUCT(tup),
-                   sizeof((*app)->am_typ));
+           memcpy((char *) &(*app)->am_typ,
+                  (char *) GETSTRUCT(tup),
+                  sizeof((*app)->am_typ));
+           app++;       }       heap_endscan(scan);       heap_close(rel, NoLock);

I think the same (move the app++, and change to memcpy (optionally))
should be done in the first case as well.

Peter Eisentraut <peter_e@gmx.net> writes:
> This code in bootstrap.c contains a sequence point violation (or
> whatever that is really called):

>         while ((tup = heap_getnext(scan, ForwardScanDirection)) != NULL)
>         {
>             (*app)->am_oid = HeapTupleGetOid(tup);
>             memmove((char *) &(*app++)->am_typ,
>                     (char *) GETSTRUCT(tup),
>                     sizeof((*app)->am_typ));
>         }

What exactly is the violation?  sizeof() is a side-effect-free compile
time constant, and the first value to be passed to memmove seems
perfectly well defined.

I grant that this is not terribly good coding style, but I don't believe
there's an actual risk here.

> In commit 1aebc361, another place in the same file was fixed like this:

Well, I don't really remember why I did that twelve years ago, but
seeing that there are a number of purely-cosmetic changes in that
commit, I'm thinking it was only meant to be cosmetic.

I certainly have no objection to making this code look more like that
code, I'm just not seeing that it's a bug.
        regards, tom lane

-----Original Message-----
From: pgsql-hackers-owner@postgresql.org [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Tom Lane
Sent: Tuesday, April 16, 2013 7:52 PM
To: Peter Eisentraut
Cc: pgsql-hackers@postgresql.org
Subject: Re: [HACKERS] ancient sequence point bug

>Peter Eisentraut <peter_e@gmx.net> writes:
>> This code in bootstrap.c contains a sequence point violation (or
>> whatever that is really called):
>
>>         while ((tup = heap_getnext(scan, ForwardScanDirection)) != NULL)
>>         {
>>             (*app)->am_oid = HeapTupleGetOid(tup);
>>             memmove((char *) &(*app++)->am_typ,
>>                     (char *) GETSTRUCT(tup),
>>                     sizeof((*app)->am_typ));
>>         }
>
>What exactly is the violation?  sizeof() is a side-effect-free compile time constant, and the first value to be passed
tomemmove seems perfectly well defined. 

Unless it is C99 and the object is a VLA, which must be evaluated at runtime.  I guess that (*app)->am_typ is not a
VLA,especially since PostgreSQL does not require C99 to compile. 

>I grant that this is not terribly good coding style, but I don't believe there's an actual risk here.

I agree that there is no risk in the sizeof operator.
According to my understanding, even this is legal:
unsigned long long *p = 0;size_t stupid = sizeof (*p);              printf("size of a long long is %u\n", (unsigned)
stupid);

But I also guess that most compilers will have a cow when scanning it.

>> In commit 1aebc361, another place in the same file was fixed like this:
>
>Well, I don't really remember why I did that twelve years ago, but seeing that there are a number of purely-cosmetic
changesin that commit, I'm thinking it was only meant to be cosmetic. 
>
>I certainly have no objection to making this code look more like that code, I'm just not seeing that it's a bug.

You are right. It's not a bug.  However, I would not be surprised if GCC or CLANG would shriek at the higher warning
levels,usually not being able to apply artificial intelligence to solve problems that seem simple to humans. 
In the interest of "quiet compiles" equivalent code that does not produce a warning seems like a good idea.  IMO-YMMV.

>            regards, tom lane
>
>
>