After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 514920 - G_GNUC_NORETURN for g_error();
G_GNUC_NORETURN for g_error();
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: general
unspecified
Other All
: Normal enhancement
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2008-02-07 05:38 UTC by Allison Karlitskaya (desrt)
Modified: 2008-02-09 02:59 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Allison Karlitskaya (desrt) 2008-02-07 05:38:09 UTC
g_error, by the documentation, never returns.

it is, unfortunately, sometimes implemented as a macro:

#define g_error(...)    g_log (G_LOG_DOMAIN,         \
                               G_LOG_LEVEL_ERROR,    \
                               __VA_ARGS__)

and g_log can't be marked as G_GNUC_NORETURN since sometimes it does return.

this results in gcc giving "control reaches end of non-void function" warnings in invalid places.

in case your C compiler doesn't do the whole __VA_ARGS__ thing there is a definition below:

static void
g_error (const gchar *format,
         ...)
{
  va_list args;
  va_start (args, format);
  g_logv (G_LOG_DOMAIN, G_LOG_LEVEL_ERROR, format, args);
  va_end (args);
}

i checked and GCC doesn't inline this.  even if you explicitly declare it "inline" and use -O3 you get no love.  i guess it doesn't know how to deal with va_list while inlining...

it's not like it matters, though.  i doubt anybody cares about g_error() performance....

the only problem is that you'll get a warning about "unused static function".

declaring this "static inline void" makes that warning go away, but in theory could act up on non-GCC compilers (although i doubt this to be true).


so we can do this:

on GNU C (which is the only place G_GNUC_NORETURN does anything anyway) we always define g_error as a static inline function that is marked G_GNUC_NORETURN.  it won't actually get inlined, but it will stop both warnings ("unused function" and "control reaches end of non-void function") and nobody cares about performance anyway.


there's a much cleaner (and simpler) solution too, and i _think_ it preserves API/ABI compatibility:

declare a normal g_error() function and implement it in a .c file.  mark it G_GNUC_NORETURN.  done.

we gain a new symbol g_error() but applications that were compiled with the old headers won't refer to this symbol and applications compiled with new headers will have the new library...

the only problem is if you're compiling with new headers and try to run your program against an old glib version.  is this supported?  what if you make the argument "but i only used g_error() and that's a feature that's in the old version too"?


which version do you prefer?
Comment 1 Allison Karlitskaya (desrt) 2008-02-07 05:45:51 UTC
still another option:

#include <stdlib.h>

#define g_error(...) \
  G_STMT_START {             \
   g_log (G_LOG_DOMAIN,      \
     G_LOG_LEVEL_ERROR,      \
     __VA_ARGS__)            \
   abort ();                 \
  } G_STMT_END


or if you don't like stdlib.h, use some existing glib NORETURN function (there is only one) or write a new one.
Comment 2 Allison Karlitskaya (desrt) 2008-02-07 07:14:30 UTC
or the infinitely easier approach:

2008-02-07  Ryan Lortie  <desrt@desrt.ca>

        * glib/gmessages.h (g_error): add for(;;); after the g_log call so
        that GCC stops issuing false warnings about reachability  Bug #514920

Committed revision 6476.
Comment 3 Behdad Esfahbod 2008-02-07 13:16:44 UTC
/me is not a huge fan
Comment 4 Owen Taylor 2008-02-07 18:06:58 UTC
And the chances that some other compiler or gcc in the future will warn about (;;); ? Seems pretty high.
Comment 5 Owen Taylor 2008-02-07 18:07:29 UTC
(Also adds extra generated code, of course)
Comment 6 Allison Karlitskaya (desrt) 2008-02-09 02:59:00 UTC
my other idea was to write a function


void g_error_with_domain (const char *domain, const char *fmt, ...) G_GNUC_NORETURN;


and #define g_error(...)   g_error_with_domain (G_LOG_DOMAIN, __VA_ARGS__);

but this adds a non-user-used function to our ABI promise.