[HACKERS] 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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ancient sequence point bug
Peter Eisentraut pete...@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ancient sequence point bug
-Original Message-
From: pgsql-hackers-ow...@postgresql.org
[mailto:pgsql-hackers-ow...@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 pete...@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.
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 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.
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
