On 12.01.2013 20:42, Andres Freund wrote:
On 2013-01-12 13:16:56 -0500, Tom Lane wrote:
Heikki Linnakangas<hlinnakan...@vmware.com> writes:
Here's one more construct to consider:
#define ereport_domain(elevel, domain, rest) \
do { \
const int elevel_ = elevel; \
if (errstart(elevel_,__FILE__, __LINE__, PG_FUNCNAME_MACRO,
domain) ||
elevel_>= ERROR) \
{ \
(void) rest; \
if (elevel_>= ERROR) \
errfinish_noreturn(1); \
else \
errfinish(1); \
} \
} while(0)
I don't care for that too much in detail -- if errstart were to return
false (it shouldn't, but if it did) this would be utterly broken,
because we'd be making error reporting calls that elog.c wouldn't be
prepared to accept. We should stick to the technique of doing the
ereport as today and then adding something that tells the compiler we
don't expect to get here; preferably something that emits no added code.
With the current ereport(), we'll call abort() if errstart returns false
and elevel >= ERROR. That's even worse than making an error reporting
calls that elog.c isn't prepared for. If we take that risk seriously,
with bogus error reporting calls we at least have chance of catching it
and reporting an error.
Yea, I didn't really like it all that much either - but anyway, I have
yet to find *any* version with a local variable that doesn't lead to
200kb size increase :(.
Hmm, that's strange. Assuming the optimizer optimizes away the paths in
the macro that are never taken when elevel is a constant, I would expect
the resulting binary to be smaller than what we have now. All those
useless abort() calls must take up some space.
Here's what I got with and without my patch on my laptop:
-rwxr-xr-x 1 heikki heikki 24767237 tammi 12 21:27 postgres-do-while-mypatch
-rwxr-xr-x 1 heikki heikki 24623081 tammi 12 21:28 postgres-unpatched
But when I build without --enable-debug, the situation reverses:
-rwxr-xr-x 1 heikki heikki 5961309 tammi 12 21:48 postgres-do-while-mypatch
-rwxr-xr-x 1 heikki heikki 5986961 tammi 12 21:49 postgres-unpatched
I suspect you're also just seeing bloat caused by more debugging
symbols, which won't have any effect at runtime. It would be nice to
have smaller debug-enabled builds, too, of course, but it's not that
important.
However, using a do-block with a local variable is definitely something
worth considering. I'm getting less enamored of the __builtin_constant_p
idea after finding out that the gcc boys seem to have curious ideas
about what its semantics ought to be:
https://bugzilla.redhat.com/show_bug.cgi?id=894515
I wonder whether __builtin_choose_expr is any better?
I forgot to mention (and it wasn't visible in the snippet I posted) the
reason I used __attribute__ ((noreturn)). We already use that attribute
elsewhere, so this optimization wouldn't require anything more from the
compiler than what already take advantage of. I think that noreturn is
more widely supported than these other constructs. Also, if I'm reading
the MSDN docs correctly, while MSVC doesn't understand __attribute__
((noreturn)) directly, it has an equivalent syntax _declspec(noreturn).
So we could easily support MSVC with this approach.
Attached is the complete patch I used for the above tests.
- Heikki
diff --git a/src/backend/bootstrap/bootscanner.l b/src/backend/bootstrap/bootscanner.l
index bdd7dcb..b97bee4 100644
--- a/src/backend/bootstrap/bootscanner.l
+++ b/src/backend/bootstrap/bootscanner.l
@@ -42,8 +42,13 @@
/* Avoid exit() on fatal scanner errors (a bit ugly -- see yy_fatal_error) */
#undef fprintf
-#define fprintf(file, fmt, msg) ereport(ERROR, (errmsg_internal("%s", msg)))
+#define fprintf(file, fmt, msg) fprintf_to_ereport(fmt, msg)
+static void
+fprintf_to_ereport(char *fmt, const char *msg)
+{
+ ereport(ERROR, (errmsg_internal("%s", msg)));
+}
static int yyline = 1; /* line number for error reporting */
diff --git a/src/backend/parser/scan.l b/src/backend/parser/scan.l
index 622cb1f..877345b 100644
--- a/src/backend/parser/scan.l
+++ b/src/backend/parser/scan.l
@@ -42,7 +42,14 @@
/* Avoid exit() on fatal scanner errors (a bit ugly -- see yy_fatal_error) */
#undef fprintf
-#define fprintf(file, fmt, msg) ereport(ERROR, (errmsg_internal("%s", msg)))
+#define fprintf(file, fmt, msg) fprintf_to_ereport(fmt, msg)
+
+static void
+fprintf_to_ereport(char *fmt, const char *msg)
+{
+ ereport(ERROR, (errmsg_internal("%s", msg)));
+}
+
/*
* GUC variables. This is a DIRECT violation of the warning given at the
diff --git a/src/backend/replication/repl_scanner.l b/src/backend/replication/repl_scanner.l
index 9ccf02a..b62625b 100644
--- a/src/backend/replication/repl_scanner.l
+++ b/src/backend/replication/repl_scanner.l
@@ -19,7 +19,13 @@
/* Avoid exit() on fatal scanner errors (a bit ugly -- see yy_fatal_error) */
#undef fprintf
-#define fprintf(file, fmt, msg) ereport(ERROR, (errmsg_internal("%s", msg)))
+#define fprintf(file, fmt, msg) fprintf_to_ereport(fmt, msg)
+
+static void
+fprintf_to_ereport(char *fmt, const char *msg)
+{
+ ereport(ERROR, (errmsg_internal("%s", msg)));
+}
/* Handle to the buffer that the lexer uses internally */
static YY_BUFFER_STATE scanbufhandle;
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index e710f22..92940e9 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -380,6 +380,18 @@ errstart(int elevel, const char *filename, int lineno,
return true;
}
+void
+errfinish_noreturn(int dummy,...)
+{
+#ifdef USE_ASSERT_CHECKING
+ ErrorData *edata = &errordata[errordata_stack_depth];
+ int elevel = edata->elevel;
+ Assert(elevel >= ERROR);
+#endif
+ errfinish(dummy);
+ abort();
+}
+
/*
* errfinish --- end an error-reporting cycle
*
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index cbbda04..523ce5c 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -101,15 +101,23 @@
* ereport_domain() directly, or preferably they can override the TEXTDOMAIN
* macro.
*
- * When elevel >= ERROR, we add an abort() call to give the compiler a hint
- * that the ereport() expansion will not return, but the abort() isn't actually
- * reached because the longjmp happens in errfinish().
+ * When elevel >= ERROR, we use errfinish_noreturn, which is identical to
+ * errfinish, but is annotated with the noreturn compiler attribute to tell
+ * the compiler that it doesn't return.
*----------
*/
-#define ereport_domain(elevel, domain, rest) \
- (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain) ? \
- (errfinish rest) : (void) 0), \
- ((elevel) >= ERROR ? abort() : (void) 0)
+#define ereport_domain(elevel, domain, rest) \
+ do { \
+ const int elevel_ = elevel; \
+ if (errstart(elevel_,__FILE__, __LINE__, PG_FUNCNAME_MACRO, domain) || elevel_>= ERROR) \
+ { \
+ (void) rest; \
+ if (elevel_>= ERROR) \
+ errfinish_noreturn(1); \
+ else \
+ errfinish(1); \
+ } \
+ } while(0)
#define ereport(elevel, rest) \
ereport_domain(elevel, TEXTDOMAIN, rest)
@@ -119,6 +127,7 @@
extern bool errstart(int elevel, const char *filename, int lineno,
const char *funcname, const char *domain);
extern void errfinish(int dummy,...);
+extern void errfinish_noreturn(int dummy,...) __attribute__((noreturn));
extern int errcode(int sqlerrcode);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
