fix for strict-alias warnings

Patch applied. Thanks.

---------------------------------------------------------------------------

Andrew Dunstan wrote:

This patch will stop gcc from issuing warnings about type-punned objects when -fstrict-aliasing is turned on, as it is in the latest gcc when you use -O2

enjoy

andrew

[ Attachment, skipping... ]

---------------------------(end of broadcast)---------------------------
TIP 4: Don't 'kill -9' the postmaster

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Patch applied. Thanks.

I hope you applied it with the additional changes you asked for ---
at the very least, cast to (void*) and then to the destination type.
As-is, the patch simply suppresses all error detection for these
conversions, which seems a bad move.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Patch applied. Thanks.

I hope you applied it with the additional changes you asked for ---
at the very least, cast to (void*) and then to the destination type.
As-is, the patch simply suppresses all error detection for these
conversions, which seems a bad move.

I don't have a version that does the double-cast, but I still have the
patch to back out and put in a new one. Andrew's point was that we cast
to void * in many places, so this case is not unique. Is that wrong?

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I don't have a version that does the double-cast, but I still have the
patch to back out and put in a new one. Andrew's point was that we cast
to void * in many places, so this case is not unique. Is that wrong?

I do not like code that uses cast to void* as a substitute for casting
to the real destination type. I think it's a lazy substitute for
providing the correct cast, and it renders the code more fragile because
there is *no* possibility of the compiler detecting a problem should you
change the source or destination datatype in a way that renders the cast
wrong.

I have not gone around and tried to fix all the places that are lazy in
this way, but I don't want to introduce more, and for sure I don't want
to set a precedent that we'll weaken our type checking any time gcc
burps for ill-defined reasons.

I agree completely with all of the objections you raised in your
original comment on the patch. In particular, I don't think we
understand why gcc is complaining about these few places and not any of
the thousands of other casts in our code. Until we understand that
difference completely, we are not "fixing a bug" by introducing void*
casts. I'd have to call it cargo-cult programming instead.

I am perfectly content to leave the warnings in place until we have a
satisfactory explanation.

regards, tom lane

Tough words! :-)

ISTM the best thing would be to back out the patch, add -fno-strict-aliasing
for gcc, and add a TODO to fix this thoroughly.

Having -fstrict-aliasing on and ignoring the warnings doesn't seem like a
sound strategy. I think we should fix it or turn it off. The web is littered
with projects that got bizzare happenings when they turned it on without any
accompanying code changes.

I agree with Tom that my patch isn't ideal (I thought I said as much).
Fixing it thoroughly will require some significant code changes, though. We
seem to be far too close to 7.4 release to contemplate that.

cheers

andrew

----- Original Message -----
From: "Tom Lane" <tgl@sss.pgh.pa.us>
To: "Bruce Momjian" <pgman@candle.pha.pa.us>
Cc: "Andrew Dunstan" <andrew@dunslane.net>; "PG Patches"
<pgsql-patches@postgresql.org>
Sent: Saturday, October 11, 2003 1:29 PM
Subject: Re: [PATCHES] fix for strict-alias warnings

Andrew Dunstan wrote:

Tough words! :-)

ISTM the best thing would be to back out the patch, add -fno-strict-aliasing
for gcc, and add a TODO to fix this thoroughly.

Having -fstrict-aliasing on and ignoring the warnings doesn't seem like a
sound strategy. I think we should fix it or turn it off. The web is littered
with projects that got bizzare happenings when they turned it on without any
accompanying code changes.

I agree with Tom that my patch isn't ideal (I thought I said as much).
Fixing it thoroughly will require some significant code changes, though. We
seem to be far too close to 7.4 release to contemplate that.

I have backed out the patch.

Looking at the case in tablecmds.c and proc.c, the first was assigning a
struct with a NodeTag pointer as its first element to another struct
with NodeTag as its first element. In fact, we do this all over the
place, having different structure pointers with a start element of
NodeTag. The proc.c cases were using MemSet, which was checking if the
int* as aligned for int* access. In fact, we could change MemSet to
always take a void *, and do the int* casting when we access it after
testing for alignment.

The big question in my mind is whether there there is other struct *
passing that could be masked right now by void* casting, and if so, do
they have different first elements? This determined whether we do
-fstrict-aliasing for gcc, or fix just these few cases.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

----- Original Message -----
From: "Bruce Momjian" <pgman@candle.pha.pa.us>

I have backed out the patch.

Looking at the case in tablecmds.c and proc.c, the first was assigning a
struct with a NodeTag pointer as its first element to another struct
with NodeTag as its first element. In fact, we do this all over the
place, having different structure pointers with a start element of
NodeTag.

Right - and it is what would have to change if you really want to obey the
ISO C rules, I believe. This is handled easily in other languages using
variant records, but C is kinda primitive here :-)

As I understand it, instead of

struct foo {
int tag
foostuff f;
}

struct bar {
int tag;
barstuff b;
}

you would need to do something like

struct foo {
foostuff f;
};

struct bar {
barstuff b;
};

struct foobar {
int tag;
union {
struct foo foo;
struct bar bar;
} v;
};

The proc.c cases were using MemSet, which was checking if the
int* as aligned for int* access. In fact, we could change MemSet to
always take a void *, and do the int* casting when we access it after
testing for alignment.

Since MemSet is generic, that is probably a good idea.

The big question in my mind is whether there there is other struct *
passing that could be masked right now by void* casting, and if so, do
they have different first elements? This determined whether we do
-fstrict-aliasing for gcc, or fix just these few cases.

Just analysing this is a non-trivial piece of work, I think.

cheers

andrew

Andrew Dunstan wrote:

The proc.c cases were using MemSet, which was checking if the
int* as aligned for int* access. In fact, we could change MemSet to
always take a void *, and do the int* casting when we access it after
testing for alignment.

Since MemSet is generic, that is probably a good idea.

I have applied the following patch to add an additional void* cast to
MemSet, and added a comment that this was safe because alignment is
checked below --- patch attached.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

[ Moved to hackers.]

Andrew Dunstan wrote:

struct foobar {
int tag;
union {
struct foo foo;
struct bar bar;
} v;
};

OK, this is very clear. Thanks.

The proc.c cases were using MemSet, which was checking if the
int* as aligned for int* access. In fact, we could change MemSet to
always take a void *, and do the int* casting when we access it after
testing for alignment.

Since MemSet is generic, that is probably a good idea.

Done.

The big question in my mind is whether there there is other struct *
passing that could be masked right now by void* casting, and if so, do
they have different first elements? This determined whether we do
-fstrict-aliasing for gcc, or fix just these few cases.

Just analysing this is a non-trivial piece of work, I think.

I understand the desire to deal with this later, but we never seem to
focus on compiler issues except during beta.

I think I know why we are hitting the warning in tablecmds.c and not in
trigger.c. Trigger.c has:

ExecCallTriggerFunc(TriggerData *trigdata,
FmgrInfo *finfo,
MemoryContext per_tuple_context)
{

...
fcinfo.flinfo = finfo;
fcinfo.context = (Node *) trigdata;

This does not generate a warning, while this does in tablecmds.c:

while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
{
FunctionCallInfoData fcinfo;
TriggerData trigdata;

/*
* Make a call to the trigger function
*
* No parameters are passed, but we do set a context
*/
MemSet(&fcinfo, 0, sizeof(fcinfo));

/*
* We assume RI_FKey_check_ins won't look at flinfo...
*/
trigdata.type = T_TriggerData;
trigdata.tg_event = TRIGGER_EVENT_INSERT | TRIGGER_EVENT_ROW;
trigdata.tg_relation = rel;
trigdata.tg_trigtuple = tuple;
trigdata.tg_newtuple = NULL;
trigdata.tg_trigger = &trig;

fcinfo.context = (Node *) &trigdata;

RI_FKey_check_ins(&fcinfo);
}

trigger.c doesn't generate the warning because it is a TriggerData*,
while tablecmds.c has an acual structure. trigger.c doesn't know if the
passed pointer was allocated via malloc(), and therefore properly
aligned for any data type, or if it was allocated on the stack, so it
does not complain.

In tablecmds.c, they create it as local loop structure that is passed to
RI_FKey_check_ins(). What we should be doing in this code is allocating
a proper Node structure using makeNode(TriggerData), and using that in
the loop. (We should allocate it outside the loop.)

I see the same problem in execQual.c. sysv_shmem probably needs the
void*, and psql/command.c should have parse_char defines as unsigned
char **, rather than char **.

Comments?

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian wrote:

I understand the desire to deal with this later, but we never seem to
focus on compiler issues except during beta.

I think I know why we are hitting the warning in tablecmds.c and not in
trigger.c. Trigger.c has:

ExecCallTriggerFunc(TriggerData *trigdata,
FmgrInfo *finfo,
MemoryContext per_tuple_context)
{

...
fcinfo.flinfo = finfo;
fcinfo.context = (Node *) trigdata;

This does not generate a warning, while this does in tablecmds.c:

while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
{
FunctionCallInfoData fcinfo;
TriggerData trigdata;

/*
* Make a call to the trigger function
*
* No parameters are passed, but we do set a context
*/
MemSet(&fcinfo, 0, sizeof(fcinfo));

/*
* We assume RI_FKey_check_ins won't look at flinfo...
*/
trigdata.type = T_TriggerData;
trigdata.tg_event = TRIGGER_EVENT_INSERT | TRIGGER_EVENT_ROW;
trigdata.tg_relation = rel;
trigdata.tg_trigtuple = tuple;
trigdata.tg_newtuple = NULL;
trigdata.tg_trigger = &trig;

fcinfo.context = (Node *) &trigdata;

RI_FKey_check_ins(&fcinfo);
}

trigger.c doesn't generate the warning because it is a TriggerData*,
while tablecmds.c has an acual structure. trigger.c doesn't know if the
passed pointer was allocated via malloc(), and therefore properly
aligned for any data type, or if it was allocated on the stack, so it
does not complain.

I could be wrong, but I don't think it has to do with malloc or whether
or not it is on the stack or in dynamic memory, but rather to do with
the fact that you have manipulated it using one personality and then
cast it to another.

In tablecmds.c, they create it as local loop structure that is passed to
RI_FKey_check_ins(). What we should be doing in this code is allocating
a proper Node structure using makeNode(TriggerData), and using that in
the loop. (We should allocate it outside the loop.)

I see the same problem in execQual.c. sysv_shmem probably needs the
void*, and psql/command.c should have parse_char defines as unsigned
char **, rather than char **.

signedness is not supposed to affect strict aliasing, but the compiler
could be confused.

(disclaimer) I am not a language lawyer, and this is one of those rare
times we need one :-) Don't take my word for it on this stuff.

cheers

andrew

Andrew Dunstan wrote:

----- Original Message -----
From: "Bruce Momjian" <pgman@candle.pha.pa.us>

I have backed out the patch.

Looking at the case in tablecmds.c and proc.c, the first was assigning a
struct with a NodeTag pointer as its first element to another struct
with NodeTag as its first element. In fact, we do this all over the
place, having different structure pointers with a start element of
NodeTag.

I have attached and applied the following patch to use makeNode for
structures that will later be cast to Node*, rather than having them be
allocated as stack variables.

This leaves the only remaning compiler warning coming from common.c listed
below. What is the exact warning generated --- this seems like a
different issue.

---------------------------------------------------------------------------

Index: src/bin/psql/command.c
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/bin/psql/command.c,v
retrieving revision 1.103
diff -c -w -r1.103 command.c
*** src/bin/psql/command.c	29 Sep 2003 16:39:18 -0000	1.103
--- src/bin/psql/command.c	11 Oct 2003 13:50:15 -0000
***************
*** 1280,1286 ****
  				case '7':
  				case '8':
  				case '9':
! 					c = parse_char((char **) &p);
  					break;

  				default:
--- 1280,1286 ----
  				case '7':
  				case '8':
  				case '9':
! 					c = parse_char((void *) &p);
  					break;

default:

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian wrote:

Andrew Dunstan wrote:

----- Original Message -----
From: "Bruce Momjian" <pgman@candle.pha.pa.us>

I have backed out the patch.

Looking at the case in tablecmds.c and proc.c, the first was assigning a
struct with a NodeTag pointer as its first element to another struct
with NodeTag as its first element. In fact, we do this all over the
place, having different structure pointers with a start element of
NodeTag.

I have attached and applied the following patch to use makeNode for
structures that will later be cast to Node*, rather than having them be
allocated as stack variables.

Oh, and thanks to everyone for doing the research on this compiler issue.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

The warning is this:

command.c: In function `unescape':
command.c:1283: warning: dereferencing type-punned pointer will break
strict-aliasing rules

p is declared thus:

const unsigned char *p;

If I change common.c/h so that parse_char() takes an (unsigned char **)
argument, cast its 2nd argument to the call to strtol to (char **), and
change the cast in the call to (unsigned char **) those warnings go away,
but I get one from the other place parse_char() is called, namely prompt.c,
which has similar code but this time p is not declared as unsigned. - and so
it goes.

Grrr. What a pain.

andrew

----- Original Message -----
From: "Bruce Momjian" <pgman@candle.pha.pa.us>
To: "Andrew Dunstan" <andrew@dunslane.net>
Cc: "PG Patches" <pgsql-patches@postgresql.org>
Sent: Sunday, October 12, 2003 7:18 PM
Subject: Re: [PATCHES] fix for strict-alias warnings

Andrew Dunstan wrote:

----- Original Message -----
From: "Bruce Momjian" <pgman@candle.pha.pa.us>

I have backed out the patch.

Looking at the case in tablecmds.c and proc.c, the first was assigning

a

struct with a NodeTag pointer as its first element to another struct
with NodeTag as its first element. In fact, we do this all over the
place, having different structure pointers with a start element of
NodeTag.

I have attached and applied the following patch to use makeNode for
structures that will later be cast to Node*, rather than having them be
allocated as stack variables.

This leaves the only remaning compiler warning coming from common.c listed
below. What is the exact warning generated --- this seems like a
different issue.

--------------------------------------------------------------------------

-

Index: src/bin/psql/command.c
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/bin/psql/command.c,v
retrieving revision 1.103
diff -c -w -r1.103 command.c
*** src/bin/psql/command.c 29 Sep 2003 16:39:18 -0000 1.103
--- src/bin/psql/command.c 11 Oct 2003 13:50:15 -0000
***************
*** 1280,1286 ****
case '7':
case '8':
case '9':
! c = parse_char((char **) &p);
break;

default:
--- 1280,1286 ----
case '7':
case '8':
case '9':
! c = parse_char((void *) &p);
break;

default:

-- 
Bruce Momjian                        |  http://candle.pha.pa.us
pgman@candle.pha.pa.us               |  (610) 359-1001
+  If your life is a hard drive,     |  13 Roberts Road
+  Christ can be your backup.        |  Newtown Square, Pennsylvania

19073

----------------------------------------------------------------------------
----

Index: src/backend/commands/tablecmds.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/backend/commands/tablecmds.c,v
retrieving revision 1.88
diff -c -c -r1.88 tablecmds.c
*** src/backend/commands/tablecmds.c 11 Oct 2003 18:04:25 -0000 1.88
--- src/backend/commands/tablecmds.c 12 Oct 2003 23:10:21 -0000
***************
*** 3449,3454 ****
--- 3449,3455 ----
Relation pkrel)
{
HeapScanDesc scan;
+ TriggerData *trigdata = makeNode(TriggerData); /* must be Node aligned

*/

HeapTuple tuple;
Trigger trig;
List *list;
***************
*** 3506,3512 ****
while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
{
FunctionCallInfoData fcinfo;
- TriggerData trigdata;

/*
* Make a call to the trigger function
--- 3507,3512 ----
***************
*** 3518,3537 ****
/*
* We assume RI_FKey_check_ins won't look at flinfo...
*/
! trigdata.type = T_TriggerData;
! trigdata.tg_event = TRIGGER_EVENT_INSERT | TRIGGER_EVENT_ROW;
! trigdata.tg_relation = rel;
! trigdata.tg_trigtuple = tuple;
! trigdata.tg_newtuple = NULL;
! trigdata.tg_trigger = &trig;

! fcinfo.context = (Node *) &trigdata;

RI_FKey_check_ins(&fcinfo);
}

heap_endscan(scan);

pfree(trig.tgargs);
}

--- 3518,3538 ----
/*
* We assume RI_FKey_check_ins won't look at flinfo...
*/
! trigdata->type = T_TriggerData;
! trigdata->tg_event = TRIGGER_EVENT_INSERT | TRIGGER_EVENT_ROW;
! trigdata->tg_relation = rel;
! trigdata->tg_trigtuple = tuple;
! trigdata->tg_newtuple = NULL;
! trigdata->tg_trigger = &trig;

! fcinfo.context = (Node *) trigdata;

RI_FKey_check_ins(&fcinfo);
}

heap_endscan(scan);

+ pfree(trigdata);
pfree(trig.tgargs);
}

Index: src/backend/executor/execQual.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/backend/executor/execQual.c,v
retrieving revision 1.148
diff -c -c -r1.148 execQual.c
*** src/backend/executor/execQual.c 11 Oct 2003 18:04:25 -0000 1.148
--- src/backend/executor/execQual.c 12 Oct 2003 23:10:23 -0000
***************
*** 699,705 ****
List    *arguments = fcache->args;
Datum result;
FunctionCallInfoData fcinfo;
! ReturnSetInfo rsinfo; /* for functions returning sets */
ExprDoneCond argDone;
bool hasSetArg;
int i;
--- 699,706 ----
List    *arguments = fcache->args;
Datum result;
FunctionCallInfoData fcinfo;
! /* for functions returning sets, must be aligned as Node, so use

makeNode */

! ReturnSetInfo *rsinfo = makeNode(ReturnSetInfo);
ExprDoneCond argDone;
bool hasSetArg;
int i;
***************
*** 746,760 ****
*/
if (fcache->func.fn_retset)
{
! fcinfo.resultinfo = (Node *) &rsinfo;
! rsinfo.type = T_ReturnSetInfo;
! rsinfo.econtext = econtext;
! rsinfo.expectedDesc = NULL;
! rsinfo.allowedModes = (int) SFRM_ValuePerCall;
! rsinfo.returnMode = SFRM_ValuePerCall;
/* isDone is filled below */
! rsinfo.setResult = NULL;
! rsinfo.setDesc = NULL;
}

/*
--- 747,761 ----
*/
if (fcache->func.fn_retset)
{
! fcinfo.resultinfo = (Node *) rsinfo;
! rsinfo->type = T_ReturnSetInfo;
! rsinfo->econtext = econtext;
! rsinfo->expectedDesc = NULL;
! rsinfo->allowedModes = (int) SFRM_ValuePerCall;
! rsinfo->returnMode = SFRM_ValuePerCall;
/* isDone is filled below */
! rsinfo->setResult = NULL;
! rsinfo->setDesc = NULL;
}

/*
***************
*** 803,812 ****
if (callit)
{
fcinfo.isnull = false;
! rsinfo.isDone = ExprSingleResult;
result = FunctionCallInvoke(&fcinfo);
*isNull = fcinfo.isnull;
! *isDone = rsinfo.isDone;
}
else
{
--- 804,813 ----
if (callit)
{
fcinfo.isnull = false;
! rsinfo->isDone = ExprSingleResult;
result = FunctionCallInvoke(&fcinfo);
*isNull = fcinfo.isnull;
! *isDone = rsinfo->isDone;
}
else
{
***************
*** 903,909 ****
TupleDesc tupdesc = NULL;
Oid funcrettype;
FunctionCallInfoData fcinfo;
! ReturnSetInfo rsinfo;
MemoryContext callerContext;
MemoryContext oldcontext;
TupleTableSlot *slot;
--- 904,910 ----
TupleDesc tupdesc = NULL;
Oid funcrettype;
FunctionCallInfoData fcinfo;
! ReturnSetInfo *rsinfo = makeNode(ReturnSetInfo); /* must be Node aligned

*/

MemoryContext callerContext;
MemoryContext oldcontext;
TupleTableSlot *slot;
***************
*** 992,1006 ****
* doesn't actually get to see the resultinfo, but set it up anyway
* because we use some of the fields as our own state variables.
*/
! fcinfo.resultinfo = (Node *) &rsinfo;
! rsinfo.type = T_ReturnSetInfo;
! rsinfo.econtext = econtext;
! rsinfo.expectedDesc = expectedDesc;
! rsinfo.allowedModes = (int) (SFRM_ValuePerCall | SFRM_Materialize);
! rsinfo.returnMode = SFRM_ValuePerCall;
/* isDone is filled below */
! rsinfo.setResult = NULL;
! rsinfo.setDesc = NULL;

/*
* Switch to short-lived context for calling the function or
--- 993,1007 ----
* doesn't actually get to see the resultinfo, but set it up anyway
* because we use some of the fields as our own state variables.
*/
! fcinfo.resultinfo = (Node *) rsinfo;
! rsinfo->type = T_ReturnSetInfo;
! rsinfo->econtext = econtext;
! rsinfo->expectedDesc = expectedDesc;
! rsinfo->allowedModes = (int) (SFRM_ValuePerCall | SFRM_Materialize);
! rsinfo->returnMode = SFRM_ValuePerCall;
/* isDone is filled below */
! rsinfo->setResult = NULL;
! rsinfo->setDesc = NULL;

/*
* Switch to short-lived context for calling the function or
***************
*** 1028,1044 ****
if (direct_function_call)
{
fcinfo.isnull = false;
! rsinfo.isDone = ExprSingleResult;
result = FunctionCallInvoke(&fcinfo);
}
else
{
result = ExecEvalExpr(funcexpr, econtext,
! &fcinfo.isnull, &rsinfo.isDone);
}

/* Which protocol does function want to use? */
! if (rsinfo.returnMode == SFRM_ValuePerCall)
{
/*
* Check for end of result set.
--- 1029,1045 ----
if (direct_function_call)
{
fcinfo.isnull = false;
! rsinfo->isDone = ExprSingleResult;
result = FunctionCallInvoke(&fcinfo);
}
else
{
result = ExecEvalExpr(funcexpr, econtext,
!   &fcinfo.isnull, &rsinfo->isDone);
}

/* Which protocol does function want to use? */
! if (rsinfo->returnMode == SFRM_ValuePerCall)
{
/*
* Check for end of result set.
***************
*** 1047,1053 ****
* tupdesc or tuplestore (since we can't get a tupdesc in the
* function-returning-tuple case)
*/
! if (rsinfo.isDone == ExprEndResult)
break;

/*
--- 1048,1054 ----
* tupdesc or tuplestore (since we can't get a tupdesc in the
* function-returning-tuple case)
*/
! if (rsinfo->isDone == ExprEndResult)
break;

/*
***************
*** 1093,1100 ****
}
tupstore = tuplestore_begin_heap(true, false, SortMem);
MemoryContextSwitchTo(oldcontext);
! rsinfo.setResult = tupstore;
! rsinfo.setDesc = tupdesc;
}

/*
--- 1094,1101 ----
}
tupstore = tuplestore_begin_heap(true, false, SortMem);
MemoryContextSwitchTo(oldcontext);
! rsinfo->setResult = tupstore;
! rsinfo->setDesc = tupdesc;
}

/*
***************
*** 1127,1139 ****
/*
* Are we done?
*/
! if (rsinfo.isDone != ExprMultipleResult)
break;
}
! else if (rsinfo.returnMode == SFRM_Materialize)
{
/* check we're on the same page as the function author */
! if (!first_time || rsinfo.isDone != ExprSingleResult)
ereport(ERROR,
(errcode(ERRCODE_E_R_I_E_SRF_PROTOCOL_VIOLATED),
errmsg("table-function protocol for materialize mode was not

followed")));

--- 1128,1140 ----
/*
* Are we done?
*/
! if (rsinfo->isDone != ExprMultipleResult)
break;
}
! else if (rsinfo->returnMode == SFRM_Materialize)
{
/* check we're on the same page as the function author */
! if (!first_time || rsinfo->isDone != ExprSingleResult)
ereport(ERROR,
(errcode(ERRCODE_E_R_I_E_SRF_PROTOCOL_VIOLATED),
errmsg("table-function protocol for materialize mode was not

followed")));

***************
*** 1144,1150 ****
ereport(ERROR,
(errcode(ERRCODE_E_R_I_E_SRF_PROTOCOL_VIOLATED),
errmsg("unrecognized table-function returnMode: %d",
! (int) rsinfo.returnMode)));

first_time = false;
}
--- 1145,1151 ----
ereport(ERROR,
(errcode(ERRCODE_E_R_I_E_SRF_PROTOCOL_VIOLATED),
errmsg("unrecognized table-function returnMode: %d",
! (int) rsinfo->returnMode)));

first_time = false;
}
***************
*** 1152,1159 ****
MemoryContextSwitchTo(callerContext);

/* The returned pointers are those in rsinfo */
! *returnDesc = rsinfo.setDesc;
! return rsinfo.setResult;
}

--- 1153,1160 ----
MemoryContextSwitchTo(callerContext);

/* The returned pointers are those in rsinfo */
! *returnDesc = rsinfo->setDesc;
! return rsinfo->setResult;
}

Index: src/backend/port/sysv_shmem.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/backend/port/sysv_shmem.c,v
retrieving revision 1.19
diff -c -c -r1.19 sysv_shmem.c
*** src/backend/port/sysv_shmem.c 11 Oct 2003 18:04:25 -0000 1.19
--- src/backend/port/sysv_shmem.c 12 Oct 2003 23:10:24 -0000
***************
*** 365,371 ****

if (hdr->magic != PGShmemMagic)
{
! shmdt(hdr);
return NULL; /* segment belongs to a non-Postgres app */
}

--- 365,371 ----

if (hdr->magic != PGShmemMagic)
{
! shmdt((void *)hdr);
return NULL; /* segment belongs to a non-Postgres app */
}

----------------------------------------------------------------------------
----

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I have attached and applied the following patch to use makeNode for
structures that will later be cast to Node*, rather than having them be
allocated as stack variables.

AFAICT, this adds unnecessary palloc overhead without actually reducing
the risk of optimization problems.

regards, tom lane

----- Original Message -----
From: "Tom Lane" <tgl@sss.pgh.pa.us>

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I have attached and applied the following patch to use makeNode for
structures that will later be cast to Node*, rather than having them be
allocated as stack variables.

AFAICT, this adds unnecessary palloc overhead without actually reducing
the risk of optimization problems.

Even without the extra overhead, the danger of strict-aliasing is not just
related to alignment. As I understand it, given strict-aliasing assumptions
the compiler is free to reorder some operations on things it thinks can't be
the same thing, or even optimise them away because they can have no effect.
I'm not 100% sure we have avoided that danger.

Quote from nice page on this subject: "ISO C is not your grandfather's C".
:-)

cheers

andrew

"Andrew Dunstan" <andrew@dunslane.net> writes:

Even without the extra overhead, the danger of strict-aliasing is not just
related to alignment.

If I understand the issue at all, it has *nothing* to do with alignment.

As I understand it, given strict-aliasing assumptions
the compiler is free to reorder some operations on things it thinks can't be
the same thing, or even optimise them away because they can have no effect.

Yah...

I'm not 100% sure we have avoided that danger.

I don't think we understand the dangers quite yet, and I think the
patches applied to date constitute useless thrashing rather than fixes.
I'd like to see less quick-hack patching and more discussion.

In particular, given that there is as yet no demonstrated effect other
than mere warnings issued by a possibly-buggy gcc release, I think it's
premature to be hacking our sources at all.

regards, tom lane

[moved to hackers]

----- Original Message -----
From: "Tom Lane" <tgl@sss.pgh.pa.us>

I don't think we understand the dangers quite yet, and I think the
patches applied to date constitute useless thrashing rather than fixes.
I'd like to see less quick-hack patching and more discussion.

In particular, given that there is as yet no demonstrated effect other
than mere warnings issued by a possibly-buggy gcc release, I think it's
premature to be hacking our sources at all.

The warning certainly appears to be bogus in one case (the one from
psql/commands.c).

I was inder the impression, perhaps incorrectly, the casting the pointers to
(void *) would inhibit the compiler from making any assumptions about what
it pointed to, and hence inhibit bad effects from those assumptions. The
only way to know would be to examine the assembler output, I suppose. The
alternative is that it would merely inhibit the compiler from issuing a
warning and mask a bad effect. That would be nasty (and a much nastier bug
in gcc, IMNSHO).

Given your last paragraph and the above, I again suggest that the simplest
and safest course right now is to add -fno-strict-aliasing to the gcc flags.
I understand Bruce's concern that compiler issues only tend to get attention
late in the cycle, but ISTM there has been enough discussion on this for it
not to be dropped.

cheers

andrew

"Andrew Dunstan" <andrew@dunslane.net> writes:

I was inder the impression, perhaps incorrectly, the casting the pointers to
(void *) would inhibit the compiler from making any assumptions about what
it pointed to, and hence inhibit bad effects from those assumptions. The
only way to know would be to examine the assembler output, I suppose. The
alternative is that it would merely inhibit the compiler from issuing a
warning and mask a bad effect. That would be nasty

IIUC the issue is whether the compiler might incorrectly rearrange the
order of operations based on the assumption that two pointers point
to different storage (when in fact they point to the same storage).
I don't see what about introducing "(void *)" would be likely to keep
the compiler from making such assumptions --- you'll still have the same
two pointers and the exact same sequence of operations. Accordingly,
I think it's very likely that the so-far-proposed patches are indeed
masking the symptom and not solving the real problem.

regards, tom lane

Tom Lane wrote:

"Andrew Dunstan" <andrew@dunslane.net> writes:

I was inder the impression, perhaps incorrectly, the casting the pointers to
(void *) would inhibit the compiler from making any assumptions about what
it pointed to, and hence inhibit bad effects from those assumptions. The
only way to know would be to examine the assembler output, I suppose. The
alternative is that it would merely inhibit the compiler from issuing a
warning and mask a bad effect. That would be nasty

IIUC the issue is whether the compiler might incorrectly rearrange the
order of operations based on the assumption that two pointers point
to different storage (when in fact they point to the same storage).

*nod*

I don't see what about introducing "(void *)" would be likely to keep
the compiler from making such assumptions --- you'll still have the same
two pointers and the exact same sequence of operations. Accordingly,
I think it's very likely that the so-far-proposed patches are indeed
masking the symptom and not solving the real problem.

My thinking was that it makes those assumptions based on the type of the
pointer, and it doesn't know the type in the case of (void *).
Otherwise it really ought to omit a warning even when you cast to (void
*), IMNSHO.

You could well be right (as I implied above :-) ), and there are at
least some clues that gcc isn't very smart about this.

So I still think adding -fno-strict-aliasing is the way to go for now.

(back to my day job)

cheers

andrew