Re: [Spice-devel] [spice-common v2 8/8] log: Let gcc know about the logging macros which abort
On Fri, Mar 29, 2019 at 06:51:54AM -0400, Frediano Ziglio wrote:
> >
> > Without the added abort(), it cannot know g_log(G_LOG_LEVEL_CRITICAL)
> > will never return.
> >
> > Signed-off-by: Christophe Fergeau
>
> I prefer the "for" way. But by the way, this is not telling the compiler
> that spice_log (NOT g_log) is not returning but to call abort() after
> spice_log.
Since abort() does not return, overall result is that the compiler won't
think code after spice_critical() can be reached.
> I don't think the compiler will warn that with that flag the function
> it not returning, is not clear why you need these changes.
> Optimization? I don't think that calling an additional function
> and jumping back to original flow would change much.
I'm fairly sure I got warnings during some experiments which led me to
these changes, but I haven't been able to get them now.
One testcase which warns without these changes:
#include "log.h"
#include "stdbool.h"
int main(int argc, char **argv)
{
switch(argc) {
case 1:
spice_critical("foo");
default:
return 0;
}
}
$ LC_ALL=C gcc -c$(pkg-config --cflags --libs glib-2.0 spice-protocol) -I
common -Wimplicit-fallthrough=5 ./fallthrough.c
In file included from ./fallthrough.c:1:
./fallthrough.c: In function 'main':
common/log.h:73:5: warning: this statement may fall through
[-Wimplicit-fallthrough=]
73 | spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, __FUNCTION__, ""
format, ## __VA_ARGS__); \
|
^~
./fallthrough.c:8:25: note: in expansion of macro 'spice_critical'
8 | spice_critical("foo");
| ^~
./fallthrough.c:9:17: note: here
9 | default:
| ^~~
(I can add this to the log)
Christophe
signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-common v2 8/8] log: Let gcc know about the logging macros which abort
On Fri, Mar 29, 2019 at 07:18:35AM -0400, Frediano Ziglio wrote:
> > On Fri, Mar 29, 2019 at 06:57:59AM -0400, Frediano Ziglio wrote:
> > > > On Fri, Mar 29, 2019 at 11:30:46AM +0100, Christophe Fergeau wrote:
> > > > > Without the added abort(), it cannot know g_log(G_LOG_LEVEL_CRITICAL)
> > > > > will never return.
> > > > >
> > > > > Signed-off-by: Christophe Fergeau
> > > > > ---
> > > > > common/log.h | 5 +
> > > > > 1 file changed, 5 insertions(+)
> > > > >
> > > > > diff --git a/common/log.h b/common/log.h
> > > > > index 7c67e7a..1482358 100644
> > > > > --- a/common/log.h
> > > > > +++ b/common/log.h
> > > > > @@ -20,6 +20,7 @@
> > > > >
> > > > > #include
> > > > > #include
> > > > > +#include
> > > > > #include
> > > > > #include
> > > > >
> > > > > @@ -42,6 +43,7 @@ void spice_log(GLogLevelFlags log_level,
> > > > > #define spice_return_if_fail(x) G_STMT_START {
> > > > > \
> > > > > if G_LIKELY(x) { } else {
> > > > > \
> > > > > spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, G_STRFUNC,
> > > > > "condition `%s' failed", #x); \
> > > > > +abort();
> > > > > \
> > > > > return;
> > > > > \
> > > >
> > > > The 'return' statment is now unreachable code & can be removed -
> > > > surprised
> > > > the compiler didn't complain that its unreachable.
> > > >
> > >
> > > As OT I would also add that a "spice_return_if_fail" which don't return
> > > is confusing, basically it's a hidden assert.
> >
> > Yeah it is a rather misleading name. I see this code is importing glib.h
> > so I wonder why spice_return_if_fail needs to exist at all. Why not just
> > drop it and use g_return_if_fail from glib instead of reinventing the
> > wheel. Or g_warn_if_fail if you want a log message, or g_assert if
> > you want a fatal error.
> >
> > https://developer.gnome.org/glib/stable/glib-Warnings-and-Assertions.html#g-return-if-fail
> >
> > Regards,
> > Daniel
>
> Yes, this was discussed. The behaviour is different so it makes sense to keep
> them. For instance for use critical is fatal while in Glib not.
> Another difference is the informations they report.
Renaming spice_return_if_fail() to spice_assert_if_fail() or
something like this would make things much clearer. In the long run,
we should try to move away from these aborts when possible.
Christophe
signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-common v2 8/8] log: Let gcc know about the logging macros which abort
> On Fri, Mar 29, 2019 at 06:57:59AM -0400, Frediano Ziglio wrote:
> > > On Fri, Mar 29, 2019 at 11:30:46AM +0100, Christophe Fergeau wrote:
> > > > Without the added abort(), it cannot know g_log(G_LOG_LEVEL_CRITICAL)
> > > > will never return.
> > > >
> > > > Signed-off-by: Christophe Fergeau
> > > > ---
> > > > common/log.h | 5 +
> > > > 1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/common/log.h b/common/log.h
> > > > index 7c67e7a..1482358 100644
> > > > --- a/common/log.h
> > > > +++ b/common/log.h
> > > > @@ -20,6 +20,7 @@
> > > >
> > > > #include
> > > > #include
> > > > +#include
> > > > #include
> > > > #include
> > > >
> > > > @@ -42,6 +43,7 @@ void spice_log(GLogLevelFlags log_level,
> > > > #define spice_return_if_fail(x) G_STMT_START {
> > > > \
> > > > if G_LIKELY(x) { } else {
> > > > \
> > > > spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, G_STRFUNC,
> > > > "condition `%s' failed", #x); \
> > > > +abort();
> > > > \
> > > > return;
> > > > \
> > >
> > > The 'return' statment is now unreachable code & can be removed -
> > > surprised
> > > the compiler didn't complain that its unreachable.
> > >
> >
> > As OT I would also add that a "spice_return_if_fail" which don't return
> > is confusing, basically it's a hidden assert.
>
> Yeah it is a rather misleading name. I see this code is importing glib.h
> so I wonder why spice_return_if_fail needs to exist at all. Why not just
> drop it and use g_return_if_fail from glib instead of reinventing the
> wheel. Or g_warn_if_fail if you want a log message, or g_assert if
> you want a fatal error.
>
> https://developer.gnome.org/glib/stable/glib-Warnings-and-Assertions.html#g-return-if-fail
>
> Regards,
> Daniel
Yes, this was discussed. The behaviour is different so it makes sense to keep
them. For instance for use critical is fatal while in Glib not.
Another difference is the informations they report.
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-common v2 8/8] log: Let gcc know about the logging macros which abort
On Fri, Mar 29, 2019 at 06:57:59AM -0400, Frediano Ziglio wrote:
> > On Fri, Mar 29, 2019 at 11:30:46AM +0100, Christophe Fergeau wrote:
> > > Without the added abort(), it cannot know g_log(G_LOG_LEVEL_CRITICAL)
> > > will never return.
> > >
> > > Signed-off-by: Christophe Fergeau
> > > ---
> > > common/log.h | 5 +
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/common/log.h b/common/log.h
> > > index 7c67e7a..1482358 100644
> > > --- a/common/log.h
> > > +++ b/common/log.h
> > > @@ -20,6 +20,7 @@
> > >
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > >
> > > @@ -42,6 +43,7 @@ void spice_log(GLogLevelFlags log_level,
> > > #define spice_return_if_fail(x) G_STMT_START { \
> > > if G_LIKELY(x) { } else { \
> > > spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, G_STRFUNC,
> > > "condition `%s' failed", #x); \
> > > +abort();
> > > \
> > > return; \
> >
> > The 'return' statment is now unreachable code & can be removed - surprised
> > the compiler didn't complain that its unreachable.
> >
>
> As OT I would also add that a "spice_return_if_fail" which don't return
> is confusing, basically it's a hidden assert.
Yeah it is a rather misleading name. I see this code is importing glib.h
so I wonder why spice_return_if_fail needs to exist at all. Why not just
drop it and use g_return_if_fail from glib instead of reinventing the
wheel. Or g_warn_if_fail if you want a log message, or g_assert if
you want a fatal error.
https://developer.gnome.org/glib/stable/glib-Warnings-and-Assertions.html#g-return-if-fail
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-common v2 8/8] log: Let gcc know about the logging macros which abort
> On Fri, Mar 29, 2019 at 11:30:46AM +0100, Christophe Fergeau wrote:
> > Without the added abort(), it cannot know g_log(G_LOG_LEVEL_CRITICAL)
> > will never return.
> >
> > Signed-off-by: Christophe Fergeau
> > ---
> > common/log.h | 5 +
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/common/log.h b/common/log.h
> > index 7c67e7a..1482358 100644
> > --- a/common/log.h
> > +++ b/common/log.h
> > @@ -20,6 +20,7 @@
> >
> > #include
> > #include
> > +#include
> > #include
> > #include
> >
> > @@ -42,6 +43,7 @@ void spice_log(GLogLevelFlags log_level,
> > #define spice_return_if_fail(x) G_STMT_START { \
> > if G_LIKELY(x) { } else { \
> > spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, G_STRFUNC,
> > "condition `%s' failed", #x); \
> > +abort();
> > \
> > return; \
>
> The 'return' statment is now unreachable code & can be removed - surprised
> the compiler didn't complain that its unreachable.
>
As OT I would also add that a "spice_return_if_fail" which don't return
is confusing, basically it's a hidden assert.
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-common v2 8/8] log: Let gcc know about the logging macros which abort
On Fri, Mar 29, 2019 at 11:30:46AM +0100, Christophe Fergeau wrote:
> Without the added abort(), it cannot know g_log(G_LOG_LEVEL_CRITICAL)
> will never return.
>
> Signed-off-by: Christophe Fergeau
> ---
> common/log.h | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/common/log.h b/common/log.h
> index 7c67e7a..1482358 100644
> --- a/common/log.h
> +++ b/common/log.h
> @@ -20,6 +20,7 @@
>
> #include
> #include
> +#include
> #include
> #include
>
> @@ -42,6 +43,7 @@ void spice_log(GLogLevelFlags log_level,
> #define spice_return_if_fail(x) G_STMT_START { \
> if G_LIKELY(x) { } else { \
> spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, G_STRFUNC, "condition
> `%s' failed", #x); \
> +abort();
> \
> return; \
The 'return' statment is now unreachable code & can be removed - surprised
the compiler didn't complain that its unreachable.
> } \
> } G_STMT_END
> @@ -49,6 +51,7 @@ void spice_log(GLogLevelFlags log_level,
> #define spice_return_val_if_fail(x, val) G_STMT_START { \
> if G_LIKELY(x) { } else { \
> spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, __FUNCTION__,
> "condition `%s' failed", #x); \
> +abort();
> \
> return (val); \
Again here.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-common v2 8/8] log: Let gcc know about the logging macros which abort
>
> Without the added abort(), it cannot know g_log(G_LOG_LEVEL_CRITICAL)
> will never return.
>
> Signed-off-by: Christophe Fergeau
I prefer the "for" way. But by the way, this is not telling the compiler
that spice_log (NOT g_log) is not returning but to call abort() after
spice_log.
I don't think the compiler will warn that with that flag the function
it not returning, is not clear why you need these changes.
Optimization? I don't think that calling an additional function
and jumping back to original flow would change much.
> ---
> common/log.h | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/common/log.h b/common/log.h
> index 7c67e7a..1482358 100644
> --- a/common/log.h
> +++ b/common/log.h
> @@ -20,6 +20,7 @@
>
> #include
> #include
> +#include
> #include
> #include
>
> @@ -42,6 +43,7 @@ void spice_log(GLogLevelFlags log_level,
> #define spice_return_if_fail(x) G_STMT_START { \
> if G_LIKELY(x) { } else { \
> spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, G_STRFUNC, "condition
> `%s' failed", #x); \
> +abort();
> \
> return; \
> } \
> } G_STMT_END
> @@ -49,6 +51,7 @@ void spice_log(GLogLevelFlags log_level,
> #define spice_return_val_if_fail(x, val) G_STMT_START { \
> if G_LIKELY(x) { } else { \
> spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, __FUNCTION__,
> "condition `%s' failed", #x); \
> +abort();
> \
> return (val); \
> } \
> } G_STMT_END
> @@ -71,10 +74,12 @@ void spice_log(GLogLevelFlags log_level,
>
> #define spice_critical(format, ...) G_STMT_START {
> \
> spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, __FUNCTION__, "" format,
> ## __VA_ARGS__); \
> +abort();
> \
> } G_STMT_END
>
> #define spice_error(format, ...) G_STMT_START { \
> spice_log(G_LOG_LEVEL_ERROR, SPICE_STRLOC, __FUNCTION__, "" format, ##
> __VA_ARGS__); \
> +abort();
> \
> } G_STMT_END
>
> #define spice_warn_if_fail(x) G_STMT_START {\
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [spice-common v2 8/8] log: Let gcc know about the logging macros which abort
Without the added abort(), it cannot know g_log(G_LOG_LEVEL_CRITICAL)
will never return.
Signed-off-by: Christophe Fergeau
---
common/log.h | 5 +
1 file changed, 5 insertions(+)
diff --git a/common/log.h b/common/log.h
index 7c67e7a..1482358 100644
--- a/common/log.h
+++ b/common/log.h
@@ -20,6 +20,7 @@
#include
#include
+#include
#include
#include
@@ -42,6 +43,7 @@ void spice_log(GLogLevelFlags log_level,
#define spice_return_if_fail(x) G_STMT_START { \
if G_LIKELY(x) { } else { \
spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, G_STRFUNC, "condition
`%s' failed", #x); \
+abort();
\
return; \
} \
} G_STMT_END
@@ -49,6 +51,7 @@ void spice_log(GLogLevelFlags log_level,
#define spice_return_val_if_fail(x, val) G_STMT_START { \
if G_LIKELY(x) { } else { \
spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, __FUNCTION__, "condition
`%s' failed", #x); \
+abort();
\
return (val); \
} \
} G_STMT_END
@@ -71,10 +74,12 @@ void spice_log(GLogLevelFlags log_level,
#define spice_critical(format, ...) G_STMT_START { \
spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, __FUNCTION__, "" format, ##
__VA_ARGS__); \
+abort();
\
} G_STMT_END
#define spice_error(format, ...) G_STMT_START { \
spice_log(G_LOG_LEVEL_ERROR, SPICE_STRLOC, __FUNCTION__, "" format, ##
__VA_ARGS__); \
+abort();
\
} G_STMT_END
#define spice_warn_if_fail(x) G_STMT_START {\
--
2.21.0
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
