Re: [Spice-devel] [PATCH] common: add backtrace via gstack
On 07/18/2011 01:32 PM, Christophe Fergeau wrote: On Mon, Jul 18, 2011 at 01:19:05PM +0300, Uri Lublin wrote: On Mon, Jul 18, 2011 at 11:13:50AM +0200, Christophe Fergeau wrote: Or this can be checked for in configure.ac I'm not sure configure.ac is the place to check it. Why not? If you want to check if a function is available or not on the system you're building on before using it, configure.ac is a good place for that. I guess I misunderstood what is suggested to be checked in configure.ac From the two options below I meant the second. Checking that backtrace or backtrace_symbols are available during compile time should be done in configure.ac. Checking /usr/bin/gstack existence should not be done at compile time and not in configure.ac, but at run-time in the code itself. ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] common: add backtrace via gstack
On Tue, Jul 19, 2011 at 02:22:14PM +0300, Uri Lublin wrote: I guess I misunderstood what is suggested to be checked in configure.ac From the two options below I meant the second. Checking that backtrace or backtrace_symbols are available during compile time should be done in configure.ac. Checking /usr/bin/gstack existence should not be done at compile time and not in configure.ac, but at run-time in the code itself. Ah, I was talking about the former. And I agree with the latter :) Christophe pgpPb7ZWPDMpN.pgp Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] common: add backtrace via gstack
On Thu, Jul 14, 2011 at 10:50:49AM +0300, Alon Levy wrote: Add a backtrace printing function copied from xserver os/backtrace.c that uses gstack which seems to be available enough that xserver uses it :) Used in ASSERT, tested on F15. What use case do you have in mind for this? Is it meant for use during development when something asserts but we forgot to run it in gdb? Or do you want to get more backtraces from crashes in bugzilla? I'm asking because /usr/bin/gstack comes in the gdb package here, which is not likely to be installed on users boxes. glibc also has backtrace(3) and backtrace_symbols(3), but they are explicitly documented as GNU extensions. Apart from this, this is a good idea to have this. Christophe pgpPiYMwtWroT.pgp Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] common: add backtrace via gstack
On Mon, Jul 18, 2011 at 10:13:27AM +0200, Christophe Fergeau wrote: On Thu, Jul 14, 2011 at 10:50:49AM +0300, Alon Levy wrote: Add a backtrace printing function copied from xserver os/backtrace.c that uses gstack which seems to be available enough that xserver uses it :) Used in ASSERT, tested on F15. What use case do you have in mind for this? Is it meant for use during development when something asserts but we forgot to run it in gdb? Or do you want to get more backtraces from crashes in bugzilla? I'm asking because /usr/bin/gstack comes in the gdb package here, which is not likely to be installed on users boxes. glibc also has backtrace(3) and backtrace_symbols(3), but they are explicitly documented as GNU extensions. Both I guess. Ok, so this fails the second use case. I can do both - compile backtrack support in if it is available (there must be some define I can use) and do gstack first if available, otherwise (if compiled in) glibc's backtrace. Apart from this, this is a good idea to have this. Christophe ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] common: add backtrace via gstack
On Mon, Jul 18, 2011 at 11:57:47AM +0300, Alon Levy wrote: On Mon, Jul 18, 2011 at 10:13:27AM +0200, Christophe Fergeau wrote: What use case do you have in mind for this? Is it meant for use during development when something asserts but we forgot to run it in gdb? Or do you want to get more backtraces from crashes in bugzilla? I'm asking because /usr/bin/gstack comes in the gdb package here, which is not likely to be installed on users boxes. glibc also has backtrace(3) and backtrace_symbols(3), but they are explicitly documented as GNU extensions. Both I guess. Ok, so this fails the second use case. I can do both - compile backtrack support in if it is available (there must be some define I can use) Or this can be checked for in configure.ac and do gstack first if available, otherwise (if compiled in) glibc's backtrace. Yep, though for now we can just go with the gstack based trace, and add the backtrace() code later if it's really needed. Christophe pgp0TY9QhRBWY.pgp Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] common: add backtrace via gstack
On Mon, Jul 18, 2011 at 11:13:50AM +0200, Christophe Fergeau wrote: On Mon, Jul 18, 2011 at 11:57:47AM +0300, Alon Levy wrote: On Mon, Jul 18, 2011 at 10:13:27AM +0200, Christophe Fergeau wrote: What use case do you have in mind for this? Is it meant for use during development when something asserts but we forgot to run it in gdb? Or do you want to get more backtraces from crashes in bugzilla? I'm asking because /usr/bin/gstack comes in the gdb package here, which is not likely to be installed on users boxes. glibc also has backtrace(3) and backtrace_symbols(3), but they are explicitly documented as GNU extensions. Both I guess. Ok, so this fails the second use case. I can do both - compile backtrack support in if it is available (there must be some define I can use) Or this can be checked for in configure.ac and do gstack first if available, otherwise (if compiled in) glibc's backtrace. Yep, though for now we can just go with the gstack based trace, and add the backtrace() code later if it's really needed. Christophe I'll take that as an ack. ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] common: add backtrace via gstack
On 07/14/2011 10:50 AM, Alon Levy wrote:
Add a backtrace printing function copied from xserver os/backtrace.c
that uses gstack which seems to be available enough that xserver uses it :)
Used in ASSERT, tested on F15.
---
diff --git a/common/backtrace.c b/common/backtrace.c
+
+while (!done) {
+bytesread = read(pipefd[0], btline, sizeof(btline) - 1);
+
+if (bytesread 0) {
+btline[bytesread] = 0;
+red_printf(%s, btline);
nitpick: why not just printf similarly to ASSERT below ?
+}
+else if ((bytesread 0) ||
+ ((errno != EINTR) (errno != EAGAIN)))
+done = 1;
This seems wrong. If read() fails due to EINTR or EAGAIN this condition breaks
out of the while loop.
I think it should be
((bytesread 0) (errno != EINTR) (errno != EAGAIN))
or (break when read() returns the value 0 -- end of file)
((bytesread == 0) || ((errno != EINTR) (errno != EAGAIN)))
+}
diff --git a/common/spice_common.h b/common/spice_common.h
#define ASSERT(x) if (!(x)) { \
printf(%s: ASSERT %s failed\n, __FUNCTION__, #x); \
+backtrace(); \
abort();\
}
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] common: add backtrace via gstack
On Thu, Jul 14, 2011 at 11:53:30AM +0300, Uri Lublin wrote:
On 07/14/2011 10:50 AM, Alon Levy wrote:
Add a backtrace printing function copied from xserver os/backtrace.c
that uses gstack which seems to be available enough that xserver uses it :)
Used in ASSERT, tested on F15.
---
diff --git a/common/backtrace.c b/common/backtrace.c
+
+while (!done) {
+bytesread = read(pipefd[0], btline, sizeof(btline) - 1);
+
+if (bytesread 0) {
+btline[bytesread] = 0;
+red_printf(%s, btline);
nitpick: why not just printf similarly to ASSERT below ?
right, should be printf. But just because red_printf adds a prefix.
+}
+else if ((bytesread 0) ||
+ ((errno != EINTR) (errno != EAGAIN)))
+done = 1;
This seems wrong. If read() fails due to EINTR or EAGAIN this
condition breaks out of the while loop.
I think it should be
((bytesread 0) (errno != EINTR) (errno != EAGAIN))
or (break when read() returns the value 0 -- end of file)
((bytesread == 0) || ((errno != EINTR) (errno != EAGAIN)))
You're right. I think both would work, the second is nicer.
+}
diff --git a/common/spice_common.h b/common/spice_common.h
#define ASSERT(x) if (!(x)) { \
printf(%s: ASSERT %s failed\n, __FUNCTION__, #x); \
+backtrace(); \
abort();\
}
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
