Macro nesting hell - Mailing list pgsql-hackers

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