Thread: Macro nesting hell

Last night my ancient HP compiler spit up on HEAD:
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=pademelon&dt=2015-07-01%2001%3A30%3A18
complaining thus:
cpp: "brin_pageops.c", line 626: error 4018: Macro param too large after substitution - use -H option.
I was able to revive pademelon by adding a new compiler flag as suggested,
but after looking at what the preprocessor is emitting, I can't say that
I blame it for being unhappy.  This simple-looking line
               Assert(BRIN_IS_REGULAR_PAGE(BufferGetPage(oldbuf)));

is expanding to this:
   do { if (!(((((BrinSpecialSpace *) ( ((void) ((bool) (! (!(((const void*)(((Page)( ((void) ((bool) (! (!(( ((void)
((bool)(! (!((oldbuf) <= NBuffers && (oldbuf) >= -NLocBuffer)) || (ExceptionalCondition("!((oldbuf) <= NBuffers &&
(oldbuf)>= -NLocBuffer)", ("FailedAssertion"), "brin_pageops.c", 626), 0)))), (oldbuf) != 0 ))) ||
(ExceptionalCondition("!((((void) ((bool) (! (!((oldbuf) <= NBuffers && (oldbuf) >= -NLocBuffer)) ||
(ExceptionalCondition(\"!((oldbuf)<= NBuffers && (oldbuf) >= -NLocBuffer)\", (\"FailedAssertion\"), \"brin_pageops.c\",
626),0)))), (oldbuf) != 0 ))", ("FailedAssertion"), "brin_pageops.c", 626), 0)))), ((oldbuf) < 0) ?
LocalBufferBlockPointers[-(oldbuf)- 1] : (Block) (BufferBlocks + ((Size) ((oldbuf) - 1)) * 8192) ))) != ((void *)0))))
||(ExceptionalCondition("!(((const void*)(((Page)( ((void) ((bool) (! (!(( ((void) ((bool) (! (!((oldbuf) <= NBuffers
&&(oldbuf) >= -NLocBuffer)) || (ExceptionalCondition(\"!((oldbuf) <= NBuffers && (oldbuf) !>= -NLocBuffer)\",
(\"FailedAssertion\"),\"brin_pageops.c\", 626), 0)))), (oldbuf) != 0 ))) || (ExceptionalCondition(\"!(( ((void) ((bool)
(!(!((oldbuf) <= NBuffers && (oldbuf) >= -NLocBuffer)) || (ExceptionalCondition(\\\"!((oldbuf) <= NBuffers && (oldbuf)
>=-NLocBuffer)\\\", (\\\"FailedAssertion\\\"), \\\"brin_pageops.c\\\", 626), 0)))), (oldbuf) != 0 ))\",
(\"FailedAssertion\"),\"brin_pageops.c\", 626), 0)))), ((oldbuf) < 0) ? LocalBufferBlockPointers[-(oldbuf) - 1] :
(Block)(BufferBlocks + ((Size) ((oldbuf) - 1)) * 8192) ))) != ((void *)0)))", ("FailedAssertion"), "brin_pageops.c",
626),0)))), ((void) ((bool) (! (!(((PageHeader) (((Page)( ((void) ((bool) (! (!(( ((void) ((bool) (! (!((oldbuf) <=
NBuffers&& (oldbuf) >= -NLocBuffer)) || (ExceptionalCondition("!((oldbuf) <= NBuffers && (oldbuf) >= -NLocBuffer)",
("FailedAssertion"),"brin_pageops.c", 626), 0)))), (oldbuf) != 0 ))) || (ExceptionalCondition("!(( ((void) ((bool) (!
(!((oldbuf)<= NBuffers && (oldbuf) >!= -NLocBuffer)) || (ExceptionalCondition(\"!((oldbuf) <= NBuffers && (oldbuf) >=
-NLocBuffer)\",(\"FailedAssertion\"), \"brin_pageops.c\", 626), 0)))), (oldbuf) != 0 ))", ("FailedAssertion"),
"brin_pageops.c",626), 0)))), ((oldbuf) < 0) ? LocalBufferBlockPointers[-(oldbuf) - 1] : (Block) (BufferBlocks +
((Size)((oldbuf) - 1)) * 8192) ))))->pd_special <= 8192)) || (ExceptionalCondition("!(((PageHeader) (((Page)( ((void)
((bool)(! (!(( ((void) ((bool) (! (!((oldbuf) <= NBuffers && (oldbuf) >= -NLocBuffer)) ||
(ExceptionalCondition(\"!((oldbuf)<= NBuffers && (oldbuf) >= -NLocBuffer)\", (\"FailedAssertion\"), \"brin_pageops.c\",
626),0)))), (oldbuf) != 0 ))) || (ExceptionalCondition(\"!(( ((void) ((bool) (! (!((oldbuf) <= NBuffers && (oldbuf) >=
-NLocBuffer))|| (ExceptionalCondition(\\\"!((oldbuf) <= NBuffers && (oldbuf) >= -NLocBuffer)\\\",
(\\\"FailedAssertion\\\"),\\\"brin_pageops.c\\\", 626), 0)))), (oldbuf) != 0 ))\", (\"FailedAssertion\"),
\"brin_pageops.c\",626), 0)))), ((oldbuf) < 0) ? LocalBufferBlockPointers[-(oldbuf) - 1] : (Bl!ock) (BufferBlocks +
((Size)((oldbuf) - 1)) * 8192) ))))->pd_special <= 8192)", ("FailedAssertion"), "brin_pageops.c", 626), 0)))), ((void)
((bool)(! (!(((PageHeader) (((Page)( ((void) ((bool) (! (!(( ((void) ((bool) (! (!((oldbuf) <= NBuffers && (oldbuf) >=
-NLocBuffer))|| (ExceptionalCondition("!((oldbuf) <= NBuffers && (oldbuf) >= -NLocBuffer)", ("FailedAssertion"),
"brin_pageops.c",626), 0)))), (oldbuf) != 0 ))) || (ExceptionalCondition("!(( ((void) ((bool) (! (!((oldbuf) <=
NBuffers&& (oldbuf) >= -NLocBuffer)) || (ExceptionalCondition(\"!((oldbuf) <= NBuffers && (oldbuf) >= -NLocBuffer)\",
(\"FailedAssertion\"),\"brin_pageops.c\", 626), 0)))), (oldbuf) != 0 ))", ("FailedAssertion"), "brin_pageops.c", 626),
0)))),((oldbuf) < 0) ? LocalBufferBlockPointers[-(oldbuf) - 1] : (Block) (BufferBlocks + ((Size) ((oldbuf) - 1)) *
8192)))))->pd_special >= (__builtin_offsetof (PageHeaderData, pd_linp)))) || (ExceptionalCondition("!(((PageHeader)
(((Page)(((void) ((bool) (!! (!(( ((void) ((bool) (! (!((oldbuf) <= NBuffers && (oldbuf) >= -NLocBuffer)) ||
(ExceptionalCondition(\"!((oldbuf)<= NBuffers && (oldbuf) >= -NLocBuffer)\", (\"FailedAssertion\"), \"brin_pageops.c\",
626),0)))), (oldbuf) != 0 ))) || (ExceptionalCondition(\"!(( ((void) ((bool) (! (!((oldbuf) <= NBuffers && (oldbuf) >=
-NLocBuffer))|| (ExceptionalCondition(\\\"!((oldbuf) <= NBuffers && (oldbuf) >= -NLocBuffer)\\\",
(\\\"FailedAssertion\\\"),\\\"brin_pageops.c\\\", 626), 0)))), (oldbuf) != 0 ))\", (\"FailedAssertion\"),
\"brin_pageops.c\",626), 0)))), ((oldbuf) < 0) ? LocalBufferBlockPointers[-(oldbuf) - 1] : (Block) (BufferBlocks +
((Size)((oldbuf) - 1)) * 8192) ))))->pd_special >= (__builtin_offsetof (PageHeaderData, pd_linp)))",
("FailedAssertion"),"brin_pageops.c", 626), 0)))), (char *) ((char *) (((Page)( ((void) ((bool) (! (!(( ((void) ((bool)
(!(!((oldbuf) <= NBuffers && (oldbuf) >= -NLocBuffer)) || (ExceptionalCondition("!((oldbuf) <= NBuffers && (oldbuf) >=
-NLocBuffer)",("FailedAssertion"), "brin_pageops.c", 626), 0)))), (oldb!uf) != 0 ))) || (ExceptionalCondition("!((
((void)((bool) (! (!((oldbuf) <= NBuffers && (oldbuf) >= -NLocBuffer)) || (ExceptionalCondition(\"!((oldbuf) <=
NBuffers&& (oldbuf) >= -NLocBuffer)\", (\"FailedAssertion\"), \"brin_pageops.c\", 626), 0)))), (oldbuf) != 0 ))",
("FailedAssertion"),"brin_pageops.c", 626), 0)))), ((oldbuf) < 0) ? LocalBufferBlockPointers[-(oldbuf) - 1] : (Block)
(BufferBlocks+ ((Size) ((oldbuf) - 1)) * 8192) ))) + ((PageHeader) (((Page)( ((void) ((bool) (! (!(( ((void) ((bool) (!
(!((oldbuf)<= NBuffers && (oldbuf) >= -NLocBuffer)) || (ExceptionalCondition("!((oldbuf) <= NBuffers && (oldbuf) >=
-NLocBuffer)",("FailedAssertion"), "brin_pageops.c", 626), 0)))), (oldbuf) != 0 ))) || (ExceptionalCondition("!((
((void)((bool) (! (!((oldbuf) <= NBuffers && (oldbuf) >= -NLocBuffer)) || (ExceptionalCondition(\"!((oldbuf) <=
NBuffers&& (oldbuf) >= -NLocBuffer)\", (\"FailedAssertion\"), \"brin_pageops.c\", 626), 0)))), (oldbuf) != 0 ))",
("FailedAssertion!"),"brin_pageops.c", 626), 0)))), ((oldbuf) < 0) ? LocalBufferBlockPointers[-(oldbuf) - 1] : (Block)
(BufferBlocks+ ((Size) ((oldbuf) - 1)) * 8192) ))))->pd_special) ))->vector[(((uintptr_t) ((1)) + ((8) - 1)) &
~((uintptr_t)((8) - 1))) / sizeof(uint16) - 1]) == 0xF093))) ExceptionalCondition("!(((((BrinSpecialSpace *) ( ((void)
((bool)(! (!(((const void*)(((Page)( ((void) ((bool) (! (!(( ((void) ((bool) (! (!((oldbuf) <= NBuffers && (oldbuf) >=
-NLocBuffer))|| (ExceptionalCondition(\"!((oldbuf) <= NBuffers && (oldbuf) >= -NLocBuffer)\", (\"FailedAssertion\"),
\"brin_pageops.c\",626), 0)))), (oldbuf) != 0 ))) || (ExceptionalCondition(\"!(( ((void) ((bool) (! (!((oldbuf) <=
NBuffers&& (oldbuf) >= -NLocBuffer)) || (ExceptionalCondition(\\\"!((oldbuf) <= NBuffers && (oldbuf) >=
-NLocBuffer)\\\",(\\\"FailedAssertion\\\"), \\\"brin_pageops.c\\\", 626), 0)))), (oldbuf) != 0 ))\",
(\"FailedAssertion\"),\"brin_pageops.c\", 626), 0)))), ((oldbuf) < 0) ? LocalBufferBlockPointers[-(oldbuf) - 1] :
(Block)(BufferBlocks + ((Size) ((oldbuf) - 1)) *! 8192) ))) != ((void *)0)))) || (ExceptionalCondition(\"!(((const
void*)(((Page)(((void) ((bool) (! (!(( ((void) ((bool) (! (!((oldbuf) <= NBuffers && (oldbuf) >= -NLocBuffer)) ||
(ExceptionalCondition(\\\"!((oldbuf)<= NBuffers && (oldbuf) >= -NLocBuffer)\\\", (\\\"FailedAssertion\\\"),
\\\"brin_pageops.c\\\",626), 0)))), (oldbuf) != 0 ))) || (ExceptionalCondition(\\\"!(( ((void) ((bool) (! (!((oldbuf)
<=NBuffers && (oldbuf) >= -NLocBuffer)) || (ExceptionalCondition(\\\\\\\"!((oldbuf) <= NBuffers && (oldbuf) >=
-NLocBuffer)\\\\\\\",(\\\\\\\"FailedAssertion\\\\\\\"), \\\\\\\"brin_pageops.c\\\\\\\", 626), 0)))), (oldbuf) != 0
))\\\",(\\\"FailedAssertion\\\"), \\\"brin_pageops.c\\\", 626), 0)))), ((oldbuf) < 0) ?
LocalBufferBlockPointers[-(oldbuf)- 1] : (Block) (BufferBlocks + ((Size) ((oldbuf) - 1)) * 8192) ))) != ((void
*)0)))\",(\"FailedAssertion\"), \"brin_pageops.c\", 626), 0)))), ((void) ((bool) (! (!(((PageHeader) (((Page)( ((void)
((bool)(! (!(( ((void) ((bool!) (! (!((oldbuf) <= NBuffers && (oldbuf) >= -NLocBuffer)) ||
(ExceptionalCondition(\"!((oldbuf)<= NBuffers && (oldbuf) >= -NLocBuffer)\", (\"FailedAssertion\"), \"brin_pageops.c\",
626),0)))), (oldbuf) != 0 ))) || (ExceptionalCondition(\"!(( ((void) ((bool) (! (!((oldbuf) <= NBuffers && (oldbuf) >=
-NLocBuffer))|| (ExceptionalCondition(\\\"!((oldbuf) <= NBuffers && (oldbuf) >= -NLocBuffer)\\\",
(\\\"FailedAssertion\\\"),\\\"brin_pageops.c\\\", 626), 0)))), (oldbuf) != 0 ))\", (\"FailedAssertion\"),
\"brin_pageops.c\",626), 0)))), ((oldbuf) < 0) ? LocalBufferBlockPointers[-(oldbuf) - 1] : (Block) (BufferBlocks +
((Size)((oldbuf) - 1)) * 8192) ))))->pd_special <= 8192)) || (ExceptionalCondition(\"!(((PageHeader) (((Page)( ((void)
((bool)(! (!(( ((void) ((bool) (! (!((oldbuf) <= NBuffers && (oldbuf) >= -NLocBuffer)) ||
(ExceptionalCondition(\\\"!((oldbuf)<= NBuffers && (oldbuf) >= -NLocBuffer)\\\", (\\\"FailedAssertion\\\"),
\\\"brin_pageops.c\\\",626), 0)))), (oldbuf) != 0 ))) || (ExceptionalCondition(\\\"!(( ((void) ((bool) (! (!((oldb!uf)
<=NBuffers && (oldbuf) >= -NLocBuffer)) || (ExceptionalCondition(\\\\\\\"!((oldbuf) <= NBuffers && (oldbuf) >=
-NLocBuffer)\\\\\\\",(\\\\\\\"FailedAssertion\\\\\\\"), \\\\\\\"brin_pageops.c\\\\\\\", 626), 0)))), (oldbuf) != 0
))\\\",(\\\"FailedAssertion\\\"), \\\"brin_pageops.c\\\", 626), 0)))), ((oldbuf) < 0) ?
LocalBufferBlockPointers[-(oldbuf)- 1] : (Block) (BufferBlocks + ((Size) ((oldbuf) - 1)) * 8192) ))))->pd_special <=
8192)\",(\"FailedAssertion\"), \"brin_pageops.c\", 626), 0)))), ((void) ((bool) (! (!(((PageHeader) (((Page)( ((void)
((bool)(! (!(( ((void) ((bool) (! (!((oldbuf) <= NBuffers && (oldbuf) >= -NLocBuffer)) ||
(ExceptionalCondition(\"!((oldbuf)<= NBuffers && (oldbuf) >= -NLocBuffer)\", (\"FailedAssertion\"), \"brin_pageops.c\",
626),0)))), (oldbuf) != 0 ))) || (ExceptionalCondition(\"!(( ((void) ((bool) (! (!((oldbuf) <= NBuffers && (oldbuf) >=
-NLocBuffer))|| (ExceptionalCondition(\\\"!((oldbuf) <= NBuffers && (oldbuf) >= -NLocBuffer)\\\",!
(\\\"FailedAssertion\\\"),\\\"brin_pageops.c\\\", 626), 0)))), (oldbuf) != 0 ))\", (\"FailedAssertion\"),
\"brin_pageops.c\",626), 0)))), ((oldbuf) < 0) ? LocalBufferBlockPointers[-(oldbuf) - 1] : (Block) (BufferBlocks +
((Size)((oldbuf) - 1)) * 8192) ))))->pd_special >= (__builtin_offsetof (PageHeaderData, pd_linp)))) ||
(ExceptionalCondition(\"!(((PageHeader)(((Page)( ((void) ((bool) (! (!(( ((void) ((bool) (! (!((oldbuf) <= NBuffers &&
(oldbuf)>= -NLocBuffer)) || (ExceptionalCondition(\\\"!((oldbuf) <= NBuffers && (oldbuf) >= -NLocBuffer)\\\",
(\\\"FailedAssertion\\\"),\\\"brin_pageops.c\\\", 626), 0)))), (oldbuf) != 0 ))) || (ExceptionalCondition(\\\"!((
((void)((bool) (! (!((oldbuf) <= NBuffers && (oldbuf) >= -NLocBuffer)) || (ExceptionalCondition(\\\\\\\"!((oldbuf) <=
NBuffers&& (oldbuf) >= -NLocBuffer)\\\\\\\", (\\\\\\\"FailedAssertion\\\\\\\"), \\\\\\\"brin_pageops.c\\\\\\\", 626),
0)))),(oldbuf) != 0 ))\\\", (\\\"FailedAssertion\\\"), \\\"brin_pageops.c\\\", 626), 0)))), ((oldbuf) < 0) ?
LocalBufferBlockPointers[-(oldbuf)- !1] : (Block) (BufferBlocks + ((Size) ((oldbuf) - 1)) * 8192) ))))->pd_special >=
(__builtin_offsetof(PageHeaderData, pd_linp)))\", (\"FailedAssertion\"), \"brin_pageops.c\", 626), 0)))), (char *)
((char*) (((Page)( ((void) ((bool) (! (!(( ((void) ((bool) (! (!((oldbuf) <= NBuffers && (oldbuf) >= -NLocBuffer)) ||
(ExceptionalCondition(\"!((oldbuf)<= NBuffers && (oldbuf) >= -NLocBuffer)\", (\"FailedAssertion\"), \"brin_pageops.c\",
626),0)))), (oldbuf) != 0 ))) || (ExceptionalCondition(\"!(( ((void) ((bool) (! (!((oldbuf) <= NBuffers && (oldbuf) >=
-NLocBuffer))|| (ExceptionalCondition(\\\"!((oldbuf) <= NBuffers && (oldbuf) >= -NLocBuffer)\\\",
(\\\"FailedAssertion\\\"),\\\"brin_pageops.c\\\", 626), 0)))), (oldbuf) != 0 ))\", (\"FailedAssertion\"),
\"brin_pageops.c\",626), 0)))), ((oldbuf) < 0) ? LocalBufferBlockPointers[-(oldbuf) - 1] : (Block) (BufferBlocks +
((Size)((oldbuf) - 1)) * 8192) ))) + ((PageHeader) (((Page)( ((void) ((bool) (! (!(( ((void) ((bool) (! (!((!oldbuf) <=
NBuffers&& (oldbuf) >= -NLocBuffer)) || (ExceptionalCondition(\"!((oldbuf) <= NBuffers && (oldbuf) >= -NLocBuffer)\",
(\"FailedAssertion\"),\"brin_pageops.c\", 626), 0)))), (oldbuf) != 0 ))) || (ExceptionalCondition(\"!(( ((void) ((bool)
(!(!((oldbuf) <= NBuffers && (oldbuf) >= -NLocBuffer)) || (ExceptionalCondition(\\\"!((oldbuf) <= NBuffers && (oldbuf)
>=-NLocBuffer)\\\", (\\\"FailedAssertion\\\"), \\\"brin_pageops.c\\\", 626), 0)))), (oldbuf) != 0 ))\",
(\"FailedAssertion\"),\"brin_pageops.c\", 626), 0)))), ((oldbuf) < 0) ? LocalBufferBlockPointers[-(oldbuf) - 1] :
(Block)(BufferBlocks + ((Size) ((oldbuf) - 1)) * 8192) ))))->pd_special) ))->vector[(((uintptr_t) ((1)) + ((8) - 1)) &
~((uintptr_t)((8) - 1))) / sizeof(uint16) - 1]) == 0xF093))", ("FailedAssertion"), "brin_pageops.c", 626); } while
(0);

The problem here is that we've got several nested levels of macros that
each feel free to evaluate their arguments multiple times.  Quite aside
from the risk of breaking toolchains, this has got to be inefficient.
A quick look at the generated source code shows five separate occurrences
of the sequence
testl    %ebp, %ebpjns    .L80movq    LocalBufferBlockPointers(%rip), %raxmovq    40(%rsp), %rdxmovq    (%rax,%rdx),
%raxjmp   .L81
 
.L80:movq    24(%rsp), %raxaddq    BufferBlocks(%rip), %rax

corresponding to the BufferGetBlock() macro.  At least gcc seems to
have figured out that it only needed to test BufferIsValid(buffer)
once, but still, this is awful.

And then there's the risk of outright bugs from multiple evaluations
of arguments.

I'm thinking we really ought to mount a campaign to replace some of these
macros with inlined-if-possible functions.
        regards, tom lane

Tom Lane wrote:
> Last night my ancient HP compiler spit up on HEAD:
> http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=pademelon&dt=2015-07-01%2001%3A30%3A18
> complaining thus:
> cpp: "brin_pageops.c", line 626: error 4018: Macro param too large after substitution - use -H option.
> I was able to revive pademelon by adding a new compiler flag as suggested,
> but after looking at what the preprocessor is emitting, I can't say that
> I blame it for being unhappy.  This simple-looking line
> 
>                 Assert(BRIN_IS_REGULAR_PAGE(BufferGetPage(oldbuf)));
> 
> is expanding to this:

Wow, that's kind of amazing.  I think this particular case boils down to
just PageGetSpecialPointer (bufpage.h) and BufferGetBlock (bufmgr.h).

> I'm thinking we really ought to mount a campaign to replace some of these
> macros with inlined-if-possible functions.

My guess is that changing a very small amount of them will do a large
enough portion of the job.

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

On 2015-07-01 12:55:48 -0300, Alvaro Herrera wrote:
> Tom Lane wrote:
> > Last night my ancient HP compiler spit up on HEAD:
> > http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=pademelon&dt=2015-07-01%2001%3A30%3A18
> > complaining thus:
> > cpp: "brin_pageops.c", line 626: error 4018: Macro param too large after substitution - use -H option.
> > I was able to revive pademelon by adding a new compiler flag as suggested,
> > but after looking at what the preprocessor is emitting, I can't say that
> > I blame it for being unhappy.  This simple-looking line
> > 
> >                 Assert(BRIN_IS_REGULAR_PAGE(BufferGetPage(oldbuf)));
> > 
> > is expanding to this:

I just hit this in clang which also warns about too long literals unless
you silence it...

> Wow, that's kind of amazing.  I think this particular case boils down to
> just PageGetSpecialPointer (bufpage.h) and BufferGetBlock (bufmgr.h).

Inlining just BufferGetBlock already helps sufficiently to press it
below 4k (the standard's limit IIRC), but that doesn't mean we shouldn't
go a bit further.

> > I'm thinking we really ought to mount a campaign to replace some of these
> > macros with inlined-if-possible functions.
> 
> My guess is that changing a very small amount of them will do a large
> enough portion of the job.

I think it'd be good to convert the macros in bufpage.h and bufmgr.h
that either currently have multiple evaluation hazards, or have a
AssertMacro() in them. The former for obvious reasons, the latter
because that immediately makes them rather large (on and it implies
multiple evaluation hazards anyway).

That'd mean inlining PageSetPageSizeAndVersion(), PageGetSpecialSize(),
PageGetSpecialPointer(), PageGetItem(), PageGetMaxOffsetNumber(),
PageIsPrunable(), PageSetPrunable(), PageClearPrunable(),
BufferIsValid(), BufferGetBlock(), BufferGetPageSize().

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2015-07-01 12:55:48 -0300, Alvaro Herrera wrote:
>> Tom Lane wrote:
>>> I'm thinking we really ought to mount a campaign to replace some of these
>>> macros with inlined-if-possible functions.

>> My guess is that changing a very small amount of them will do a large
>> enough portion of the job.

> I think it'd be good to convert the macros in bufpage.h and bufmgr.h
> that either currently have multiple evaluation hazards, or have a
> AssertMacro() in them. The former for obvious reasons, the latter
> because that immediately makes them rather large (on and it implies
> multiple evaluation hazards anyway).

> That'd mean inlining PageSetPageSizeAndVersion(), PageGetSpecialSize(),
> PageGetSpecialPointer(), PageGetItem(), PageGetMaxOffsetNumber(),
> PageIsPrunable(), PageSetPrunable(), PageClearPrunable(),
> BufferIsValid(), BufferGetBlock(), BufferGetPageSize().

Sounds reasonable to me.  If you do this, I'll see whether pademelon
can be adjusted to build using the minimum macro expansion buffer
size specified by the C standard.
        regards, tom lane

... btw, why don't we convert c.h's Max(), Min(), and Abs() to inlines?
They've all got multi-eval hazards.

It might also be interesting to research whether inline would allow
simplifying the MemSetFoo family.
        regards, tom lane

On 2015-08-12 10:18:12 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2015-07-01 12:55:48 -0300, Alvaro Herrera wrote:
> >> Tom Lane wrote:
> >>> I'm thinking we really ought to mount a campaign to replace some of these
> >>> macros with inlined-if-possible functions.
>
> >> My guess is that changing a very small amount of them will do a large
> >> enough portion of the job.
>
> > I think it'd be good to convert the macros in bufpage.h and bufmgr.h
> > that either currently have multiple evaluation hazards, or have a
> > AssertMacro() in them. The former for obvious reasons, the latter
> > because that immediately makes them rather large (on and it implies
> > multiple evaluation hazards anyway).
>
> > That'd mean inlining PageSetPageSizeAndVersion(), PageGetSpecialSize(),
> > PageGetSpecialPointer(), PageGetItem(), PageGetMaxOffsetNumber(),
> > PageIsPrunable(), PageSetPrunable(), PageClearPrunable(),
> > BufferIsValid(), BufferGetBlock(), BufferGetPageSize().
>
> Sounds reasonable to me.  If you do this, I'll see whether pademelon
> can be adjusted to build using the minimum macro expansion buffer
> size specified by the C standard.

Here's the patch attached.

There's two more macros on the list that I had missed:
PageXLogRecPtrSet(), PageXLogRecPtrGet(). Unfortunately *Set() requires
to pas a pointer to the PageXLogRecPtr - there's only two callers tho.
We could instead just leave these, or add an indirecting macro. Seems
fine to me though.

With it applied pg compiles without the warning I saw before:
/home/andres/src/postgresql/src/backend/access/brin/brin_pageops.c:778:5: warning: string literal of length 7732
exceedsmaximum length 4095 
      that ISO C99 compilers are required to support [-Woverlength-strings]
                                Assert(BRIN_IS_REGULAR_PAGE(BufferGetPage(oldbuf)));
                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

We could obviously be more aggressive and convert all the applicable
defines, but they are already readable and have no multiple eval
hazards, so I'm not inclined to that.

Andres

Andres Freund <andres@anarazel.de> writes:
> On 2015-08-12 10:18:12 -0400, Tom Lane wrote:
>> Sounds reasonable to me.  If you do this, I'll see whether pademelon
>> can be adjusted to build using the minimum macro expansion buffer
>> size specified by the C standard.

> Here's the patch attached.

Looks like you need to pay more attention to the surrounding comments:
some of them still refer to the code as a macro, and I see at least one
place that explicitly mentions double-eval hazards that this presumably
removes.  (I think your previous patch re fastgetattr was also a bit weak
on the comments, btw.)
        regards, tom lane