Обсуждение: [HACKERS] safer node casting

There is a common coding pattern that goes like this:

    RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
    Assert(IsA(rinfo, RestrictInfo));

(Arguably, the Assert should come before the cast, but I guess it's done
this way out of convenience.)

(Not to mention the other common coding pattern of just doing the cast
and hoping for the best.)

I propose a macro castNode() that combines the assertion and the cast,
so this would become

    RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));

This is inspired by the dynamic_cast operator in C++, but follows the
syntax of the well-known makeNode() macro.

Besides saving a bunch of code and making things safer, the function
syntax also makes some code easier to read by saving levels of
parentheses, for example:

-           Assert(IsA(sstate->testexpr, BoolExprState));
-           oplist = ((BoolExprState *) sstate->testexpr)->args;
+           oplist = castNode(BoolExprState, sstate->testexpr)->args;

Attached is a patch that shows how this would work.  There is a lot more
that can be done, but I just stopped after a while for now.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> I propose a macro castNode() that combines the assertion and the cast,
> so this would become
>     RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));

Seems like an OK idea, but I'm concerned by the implied multiple
evaluations, particularly if you're going to apply this to function
results --- as the above example does.  I think you need to go the
extra mile to make it single-evaluation.  See newNode() for ideas.

Just to bikeshed a bit ... would "castNode" be a better name?
Seems like a closer analogy to makeNode(), for instance.
        regards, tom lane

I wrote:
> Just to bikeshed a bit ... would "castNode" be a better name?

Um ... -ENOCAFFEINE.  Never mind that bit.
        regards, tom lane

On Sat, Dec 31, 2016 at 11:30 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>> I propose a macro castNode() that combines the assertion and the cast,
>> so this would become
>>     RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));
>

+1. That would be wonderful.

> Seems like an OK idea, but I'm concerned by the implied multiple
> evaluations, particularly if you're going to apply this to function
> results --- as the above example does.  I think you need to go the
> extra mile to make it single-evaluation.  See newNode() for ideas.
>

+1.

In case the Assert fails, the debugger would halt at castNode macro
and a first time reader would be puzzled to see no assert there.
Obviously looking at the #define should clarify the confusion. But I
don't see how that can be avoided. I was thinking of using a function
castNodeFunc(), but it can't accomodate Assert with _type_. Will
calling this function as checkAndCastNode() help?

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Hi,


On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:
> There is a common coding pattern that goes like this:
> 
>     RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
>     Assert(IsA(rinfo, RestrictInfo));


> I propose a macro castNode() that combines the assertion and the cast,
> so this would become
> 
>     RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));

I'm quite a bit in favor of something like this, having proposed it
before ;)

> +#define castNode(_type_,nodeptr)    (AssertMacro(!nodeptr || IsA(nodeptr,_type_)), (_type_ *)(nodeptr))

ISTM that we need to do the core part of this in an inline function, to
avoid multiple evaluation hazards - which seem quite likely to occur
here - it's pretty common to cast the result of a function after all.

Something like

static inline Node*
castNodeImpl(void *c, enum NodeTag t)
{   Assert(c == NULL || IsA(c, t));   return c;
}

#define castNode(_type_, nodeptr) ((_type_ *) castNodeImpl(nodeptr, _type_))

should work without too much trouble afaics?

Greetings,

Andres Freund

On Mon, Jan 2, 2017 at 2:10 PM, Andres Freund <andres@anarazel.de> wrote:
> Hi,
>
>
> On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:
>> There is a common coding pattern that goes like this:
>>
>>     RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
>>     Assert(IsA(rinfo, RestrictInfo));
>
>
>> I propose a macro castNode() that combines the assertion and the cast,
>> so this would become
>>
>>     RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));
>
> I'm quite a bit in favor of something like this, having proposed it
> before ;)
>
>> +#define castNode(_type_,nodeptr)     (AssertMacro(!nodeptr || IsA(nodeptr,_type_)), (_type_ *)(nodeptr))
>
> ISTM that we need to do the core part of this in an inline function, to
> avoid multiple evaluation hazards - which seem quite likely to occur
> here - it's pretty common to cast the result of a function after all.
>
> Something like
>
> static inline Node*
> castNodeImpl(void *c, enum NodeTag t)
> {
>     Assert(c == NULL || IsA(c, t));
>     return c;
> }
>
> #define castNode(_type_, nodeptr) ((_type_ *) castNodeImpl(nodeptr, _type_))
>
> should work without too much trouble afaics?
>
I tried this quickly as per attached patch. It gave a compiler error
createplan.c: In function ‘castNodeImpl’:
createplan.c:340:2: error: ‘T_t’ undeclared (first use in this function)
createplan.c:340:2: note: each undeclared identifier is reported only
once for each function it appears in
createplan.c: In function ‘create_plan_recurse’:
createplan.c:445:13: error: expected expression before ‘AggPath’

Is the attached patch as per your suggestion?

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

On 2017-01-03 11:00:47 +0530, Ashutosh Bapat wrote:
> On Mon, Jan 2, 2017 at 2:10 PM, Andres Freund <andres@anarazel.de> wrote:
> > Hi,
> >
> >
> > On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:
> >> There is a common coding pattern that goes like this:
> >>
> >>     RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
> >>     Assert(IsA(rinfo, RestrictInfo));
> >
> >
> >> I propose a macro castNode() that combines the assertion and the cast,
> >> so this would become
> >>
> >>     RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));
> >
> > I'm quite a bit in favor of something like this, having proposed it
> > before ;)
> >
> >> +#define castNode(_type_,nodeptr)     (AssertMacro(!nodeptr || IsA(nodeptr,_type_)), (_type_ *)(nodeptr))
> >
> > ISTM that we need to do the core part of this in an inline function, to
> > avoid multiple evaluation hazards - which seem quite likely to occur
> > here - it's pretty common to cast the result of a function after all.
> >
> > Something like
> >
> > static inline Node*
> > castNodeImpl(void *c, enum NodeTag t)
> > {
> >     Assert(c == NULL || IsA(c, t));
> >     return c;
> > }
> >
> > #define castNode(_type_, nodeptr) ((_type_ *) castNodeImpl(nodeptr, _type_))
> >
> > should work without too much trouble afaics?
> >
> I tried this quickly as per attached patch. It gave a compiler error
> createplan.c: In function ‘castNodeImpl’:
> createplan.c:340:2: error: ‘T_t’ undeclared (first use in this function)
> createplan.c:340:2: note: each undeclared identifier is reported only
> once for each function it appears in
> createplan.c: In function ‘create_plan_recurse’:
> createplan.c:445:13: error: expected expression before ‘AggPath’

Well, I wrote that just to outline my suggestion, not as a patch ;).
It's just that we have to replace IsA() with nodeTag(nodeptr) == t
(because IsA does string concat magic).

Greetings,

Andres Freund

Hi Peter,


On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:
>     RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));

Are you planning to add this / update this patch? Because I really would
have liked this a number of times already...  I can update it according
to my suggestions (to avoid multiple eval scenarios) if helpful

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:
>> RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));

> Are you planning to add this / update this patch? Because I really would
> have liked this a number of times already...  I can update it according
> to my suggestions (to avoid multiple eval scenarios) if helpful

Yeah, I'd like that in sooner rather than later, too.  But we do need
it to be foolproof - no multiple evals.  The first draft had
inadequate-parenthesization hazards, too.
        regards, tom lane

On 2017-01-25 19:21:40 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:
> >> RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));
>
> > Are you planning to add this / update this patch? Because I really would
> > have liked this a number of times already...  I can update it according
> > to my suggestions (to avoid multiple eval scenarios) if helpful
>
> Yeah, I'd like that in sooner rather than later, too.  But we do need
> it to be foolproof - no multiple evals.  The first draft had
> inadequate-parenthesization hazards,

How about something like the attached? The first patch just contains
castNode(), the second one a rebased version of Peter's changes (with
some long lines broken up).

Andres

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andres Freund <andres@anarazel.de> writes:
> How about something like the attached? The first patch just contains
> castNode(), the second one a rebased version of Peter's changes (with
> some long lines broken up).

Looks generally good.  A couple quibbles from a quick read-through:

* All but the first change in ProcessCopyOptions seem rather pointless:
            else if (defel->arg && IsA(defel->arg, List))
-                cstate->force_quote = (List *) defel->arg;
+                cstate->force_quote = castNode(List, defel->arg);

In these places, castNode() isn't checking anything the if-condition
didn't.  Maybe it's good style anyway, but I'm not really convinced.

* In ExecInitAgg:
            aggnode = list_nth(node->chain, phase - 1);
-            sortnode = (Sort *) aggnode->plan.lefttree;
-            Assert(IsA(sortnode, Sort));
+            sortnode = castNode(Sort, aggnode->plan.lefttree);

it seems like the assignment to aggnode ought to have a castNode on it too
(the fact that it lacks any cast now is sloppy and not per project style,
IMO).

There were a bunch of places in ab1f0c822 where I wished I had this,
but I can go back and back-fill that later; doesn't need to be in the
first commit.

BTW, maybe it's just the first flush of enthusiasm, but I can see us
using this so much that the lack of it in back branches will become
a serious PITA for back-patching.  So I'm strongly tempted to propose
that your 0001 should be back-patched.  However, before 9.6 we didn't
have the compiler requirement that "static inline" in headers must be
handled sanely.  Maybe a useful compromise would be to put 0001 in 9.6,
and before that just add

#define castNode(_type_,nodeptr)  ((_type_ *)(nodeptr))

which would allow the notation to be used safely, albeit without
any assertion backup.  Alternatively, we could enable the assertion
code only for gcc, which would probably be plenty good enough for
finding bugs in stable branches.
        regards, tom lane

On 2017-01-26 16:55:33 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > How about something like the attached? The first patch just contains
> > castNode(), the second one a rebased version of Peter's changes (with
> > some long lines broken up).
> 
> Looks generally good.  A couple quibbles from a quick read-through:
> 
> * All but the first change in ProcessCopyOptions seem rather pointless:
> 
>              else if (defel->arg && IsA(defel->arg, List))
> -                cstate->force_quote = (List *) defel->arg;
> +                cstate->force_quote = castNode(List, defel->arg);
> 
> In these places, castNode() isn't checking anything the if-condition
> didn't.  Maybe it's good style anyway, but I'm not really convinced.

Agreed that it's not not necessary - I didn't add this one (or any
castNode actually). But I don't think it matters much.


> * In ExecInitAgg:
> 
>              aggnode = list_nth(node->chain, phase - 1);
> -            sortnode = (Sort *) aggnode->plan.lefttree;
> -            Assert(IsA(sortnode, Sort));
> +            sortnode = castNode(Sort, aggnode->plan.lefttree);
> 
> it seems like the assignment to aggnode ought to have a castNode on it
> too

Yea, looks good.


> (the fact that it lacks any cast now is sloppy and not per project style,
> IMO).

There's a lot of these missing :(.  This is one of these things that'd
be a lot easier to enforce if we'd be able to compile in a c++
compatible mode (-Wc++-compat), because there void * to X * casts
have to be done explicitly.


> BTW, maybe it's just the first flush of enthusiasm, but I can see us
> using this so much that the lack of it in back branches will become
> a serious PITA for back-patching.

Might, yea.


> So I'm strongly tempted to propose
> that your 0001 should be back-patched.  However, before 9.6 we didn't
> have the compiler requirement that "static inline" in headers must be
> handled sanely.  Maybe a useful compromise would be to put 0001 in 9.6,
> and before that just add
> 
> #define castNode(_type_,nodeptr)  ((_type_ *)(nodeptr))
> 
> which would allow the notation to be used safely, albeit without
> any assertion backup.  Alternatively, we could enable the assertion
> code only for gcc, which would probably be plenty good enough for
> finding bugs in stable branches.

#if defined(USE_ASSERT_CHECKING) && defined(PG_USE_INLINE)
is probably a better gatekeeper in the back-branches, than gcc? Then we
can just remove the defined(PG_USE_INLINE) and it's associated comment
in >= 9.6.

Regards,

Andres

Andres Freund <andres@anarazel.de> writes:
> #if defined(USE_ASSERT_CHECKING) && defined(PG_USE_INLINE)
> is probably a better gatekeeper in the back-branches, than gcc?

Ah, yeah, that would work --- I'd already swapped out that business ;-)
        regards, tom lane

On 2017-01-26 17:27:45 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > #if defined(USE_ASSERT_CHECKING) && defined(PG_USE_INLINE)
> > is probably a better gatekeeper in the back-branches, than gcc?
> 
> Ah, yeah, that would work --- I'd already swapped out that business ;-)

Done that way.

- Andres

Hi,

On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:
> This is inspired by the dynamic_cast operator in C++, but follows the
> syntax of the well-known makeNode() macro.

The analogy to dynamic_cast goes only so far, because we don't actually
support inheritance.  I.e. in c++ we could successfully cast SeqScanState to a
PlanState, ScanState and SeqScanState - but with our model only
SeqScanState can be checked.

Greetings,

Andres

Andres Freund <andres@anarazel.de> writes:
> On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:
>> This is inspired by the dynamic_cast operator in C++, but follows the
>> syntax of the well-known makeNode() macro.

> The analogy to dynamic_cast goes only so far, because we don't actually
> support inheritance.  I.e. in c++ we could successfully cast SeqScanState to a
> PlanState, ScanState and SeqScanState - but with our model only
> SeqScanState can be checked.

Yeah, I was thinking about that earlier --- this can only be used to cast
to a concrete node type, not one of the "abstract" types like Plan * or
Expr *.  Not sure if that's worth worrying about though; I don't think
I've ever seen actual bugs in PG code from casting the wrong thing in that
direction.  For the most part, passing the wrong thing would end up firing
a default: case in a switch, or some such, so we already do have some
defenses for that direction.
        regards, tom lane

On 2017-01-26 20:29:06 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:
> >> This is inspired by the dynamic_cast operator in C++, but follows the
> >> syntax of the well-known makeNode() macro.
> 
> > The analogy to dynamic_cast goes only so far, because we don't actually
> > support inheritance.  I.e. in c++ we could successfully cast SeqScanState to a
> > PlanState, ScanState and SeqScanState - but with our model only
> > SeqScanState can be checked.
> 
> Yeah, I was thinking about that earlier --- this can only be used to cast
> to a concrete node type, not one of the "abstract" types like Plan * or
> Expr *.  Not sure if that's worth worrying about though; I don't think
> I've ever seen actual bugs in PG code from casting the wrong thing in that
> direction.  For the most part, passing the wrong thing would end up firing
> a default: case in a switch, or some such, so we already do have some
> defenses for that direction.

Yea, I'm not actually worried about it - I was more generally remarking
on the analogy made by Peter. For a second I was considering bringing up
the analogy in a comment or such, and this was one of a number of
arguments that made me disregard that idea.

Andres

On 1/26/17 16:15, Andres Freund wrote:
> On 2017-01-25 19:21:40 -0500, Tom Lane wrote:
>> Andres Freund <andres@anarazel.de> writes:
>>> On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:
>>>> RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));
>>
>>> Are you planning to add this / update this patch? Because I really would
>>> have liked this a number of times already...  I can update it according
>>> to my suggestions (to avoid multiple eval scenarios) if helpful
>>
>> Yeah, I'd like that in sooner rather than later, too.  But we do need
>> it to be foolproof - no multiple evals.  The first draft had
>> inadequate-parenthesization hazards,
> 
> How about something like the attached? The first patch just contains
> castNode(), the second one a rebased version of Peter's changes (with
> some long lines broken up).

Thanks for finishing that up.  I have committed more uses that I had
lying around partially done.  Looks nice now.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Jeff Janes <jeff.janes@gmail.com> writes:
> With commit 38d103763d14baddf3cbfe4b00b501059fc9447f, I'm now getting a
> compiler warning:
> indxpath.c: In function 'generate_bitmap_or_paths':
> indxpath.c:1312: warning: unused variable 'rinfo'

Me too, without asserts.  Fixed.
        regards, tom lane

Hi,

I was about to add a few more uses of castNode, which made me think.

You proposed replacing:

On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:
> There is a common coding pattern that goes like this:
> 
>     RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
>     Assert(IsA(rinfo, RestrictInfo));

with

> +#define castNode(_type_,nodeptr)    (AssertMacro(!nodeptr || IsA(nodeptr,_type_)), (_type_ *)(nodeptr))
(now an inline function, but that's besides my point)

Those aren't actually equivalent, because of the !nodeptr. IsA() crashes
for NULL pointers, but the new code won't. Which means 9ba8a9ce4548b et
al actually weakened some asserts.

Should we perhaps have one NULL accepting version (castNodeNull?) and
one that separately asserts that ptr != NULL?

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> Those aren't actually equivalent, because of the !nodeptr. IsA() crashes
> for NULL pointers, but the new code won't. Which means 9ba8a9ce4548b et
> al actually weakened some asserts.

> Should we perhaps have one NULL accepting version (castNodeNull?) and
> one that separately asserts that ptr != NULL?

-1 ... if you're going to use something in a way that requires it not tobe null, your code will crash quite efficiently
ona null, with orwithout an assert.  I don't think we need the extra cogitive burden oftwo distinct macros for this.
 
        regards, tom lane

On 2/24/17 10:54, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
>> Those aren't actually equivalent, because of the !nodeptr. IsA() crashes
>> for NULL pointers, but the new code won't. Which means 9ba8a9ce4548b et
>> al actually weakened some asserts.
> 
>> Should we perhaps have one NULL accepting version (castNodeNull?) and
>> one that separately asserts that ptr != NULL?
> 
> -1 ... if you're going to use something in a way that requires it not to
>  be null, your code will crash quite efficiently on a null, with or
>  without an assert.  I don't think we need the extra cogitive burden of
>  two distinct macros for this.

I think we should just add some Assert(thepointer) where necessary.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services