* Re: --with-babeltrace generates many FAILs [not found] <20140816204614.GA7000@host2.jankratochvil.net> @ 2014-08-19 12:43 ` Yao Qi 2014-08-19 14:08 ` Jan Kratochvil 0 siblings, 1 reply; 11+ messages in thread From: Yao Qi @ 2014-08-19 12:43 UTC (permalink / raw) To: Jan Kratochvil, gdb-patches On 08/17/2014 04:46 AM, Jan Kratochvil wrote: > since 65c749e7c049f9bf944c5fbe9e727b7a8b4ccc7c (Fix build/17104) babeltrace > got enabled for me but now I get many new FAILs in testsuites, such as for > gdb.trace/actions.exp: > (gdb) target ctf .../gdb/testsuite/gdb.trace/actions.ctf > [error] Invalid CTF stream: content size is smaller than packet headers. > [error] Stream index creation error. > [error] Open file stream error. > [warning] [Context] Cannot open_tra I can reproduce it on my Fedora 20 x86 machine. Let me know it works for you or not. I build GDB with different babeltrace (1.1.0, 1.1.1, 1.1.2, 1.2.0, and 1.2.1), and run test in gdb.trace. No test result change. -- Yao (é½å°§) Subject: [PATCH] Check babeltrace 1.1.0 When GDB uses recent version of babeltrace, such as 1.2.x, we'll see such error emitted from babeltrace library, (gdb) target ctf .../gdb/testsuite/gdb.trace/actions.ctf [error] Invalid CTF stream: content size is smaller than packet headers. [error] Stream index creation error. [error] Open file stream error. The problem can be reproduce out of GDB too, using babeltrace, $ babeltrace ./fake-packet.ctf/ [error] Invalid CTF stream: content size is smaller than packet headers. [error] Stream index creation error. [error] Open file stream error. Recent babeltrace library becomes more strict on CTF, and complains about one "faked packet" GDB adds, when saving trace data in ctf format from GDB. babeltrace 1.1.0 has a bug that it can't read trace data smaller than a certain size (see https://bugs.lttng.org/issues/450). We workaround it in GDB to append some meaningless data in a faked packet to make sure trace file is large enough (see ctf.c:ctf_end). The babeltrace issue was fixed in 1.1.1 release. However, babeltrace recent release (since 1.1.2) starts to complain about such faked packet. Here is a table shows that whether faked packet or no faked packet is "supported" by various babeltrace releases, faked packet no faked packet 1.1.0 Yes No 1.1.1 Yes Yes 1.1.2 No Yes 1.2.0 No Yes We decide to include the code to workaround 1.1.0 issue only if 1.1.0 is used. We choose pkg-config to check babeltrace's version in configure. gdb: 2014-08-19 Yao Qi <yao@codesourcery.com> * configure.ac: Disable babeltrace support if pkg-config is missing. Use pkg-config to check whether libbabeltrace is 1.1.0. * config.in: Regenerate. * configure: Regenerate. * ctf.c (CTF_FILE_MIN_SIZE): Remove. (ctf_end): Wrap the code with #if HAVE_LIBBABELTRACE1_1_0 #endif. [HAVE_LIBBABELTRACE1_1_0] (CTF_FILE_MIN_SIZE): New macro. --- gdb/config.in | 3 +++ gdb/configure | 25 +++++++++++++++++++++++++ gdb/configure.ac | 22 ++++++++++++++++++++++ gdb/ctf.c | 22 +++++++++++++--------- 4 files changed, 63 insertions(+), 9 deletions(-) diff --git a/gdb/config.in b/gdb/config.in index b853412..54152cd 100644 --- a/gdb/config.in +++ b/gdb/config.in @@ -183,6 +183,9 @@ /* Define if you have the babeltrace library. */ #undef HAVE_LIBBABELTRACE +/* Define to 1 if you have libbabeltrace 1.1.0 */ +#undef HAVE_LIBBABELTRACE1_1_0 + /* Define to 1 if you have the `dl' library (-ldl). */ #undef HAVE_LIBDL diff --git a/gdb/configure b/gdb/configure index 0b992ed..4656b49 100755 --- a/gdb/configure +++ b/gdb/configure @@ -14936,6 +14936,11 @@ $as_echo "$with_babeltrace" >&6; } if test "x$with_babeltrace" = "xno"; then { $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: babletrace support disabled; GDB is unable to read CTF data." >&5 $as_echo "$as_me: WARNING: babletrace support disabled; GDB is unable to read CTF data." >&2;} +elif test "${pkg_config_prog_path}" = "missing"; then + # pkg-config is used to check the version of libbabeltrace. If pkg-config + # is missing, we have to disable babeltrace support. + { $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: pkg-config not found, babletrace support disabled" >&5 +$as_echo "$as_me: WARNING: pkg-config not found, babletrace support disabled" >&2;} else # Append -Werror to CFLAGS so that configure can catch the warning # "assignment from incompatible pointer type", which is related to @@ -15426,6 +15431,26 @@ $as_echo "$LIBBABELTRACE" >&6; } { $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: babeltrace is missing or unusable; GDB is unable to read CTF data." >&5 $as_echo "$as_me: WARNING: babeltrace is missing or unusable; GDB is unable to read CTF data." >&2;} fi + else + # Need to know whether libbabeltrace is 1.1.0. + pkg_config_path= + for x in $LTLIBBABELTRACE; do + case "$x" in + -L*) + dir=`echo "X$x" | sed -e 's/^X-L//'` + if test -d "$dir/pkgconfig"; then + pkg_config_path="${pkg_config_path}${pkg_config_path:+:}$dir/pkgconfig" + fi + ;; + esac + done + + `PKG_CONFIG_PATH=$PKG_CONFIG_PATH:$pkg_config_path ${pkg_config_prog_path} babeltrace = 1.1.0` + if test "$?" -eq 0 ; then + +$as_echo "#define HAVE_LIBBABELTRACE1_1_0 1" >>confdefs.h + + fi fi fi diff --git a/gdb/configure.ac b/gdb/configure.ac index 61919b4..1d8d400 100644 --- a/gdb/configure.ac +++ b/gdb/configure.ac @@ -2420,6 +2420,10 @@ AC_MSG_RESULT([$with_babeltrace]) if test "x$with_babeltrace" = "xno"; then AC_MSG_WARN([babletrace support disabled; GDB is unable to read CTF data.]) +elif test "${pkg_config_prog_path}" = "missing"; then + # pkg-config is used to check the version of libbabeltrace. If pkg-config + # is missing, we have to disable babeltrace support. + AC_MSG_WARN([pkg-config not found, babletrace support disabled]) else # Append -Werror to CFLAGS so that configure can catch the warning # "assignment from incompatible pointer type", which is related to @@ -2450,6 +2454,24 @@ else else AC_MSG_WARN([babeltrace is missing or unusable; GDB is unable to read CTF data.]) fi + else + # Need to know whether libbabeltrace is 1.1.0. + pkg_config_path= + for x in $LTLIBBABELTRACE; do + case "$x" in + -L*) + dir=`echo "X$x" | sed -e 's/^X-L//'` + if test -d "$dir/pkgconfig"; then + pkg_config_path="${pkg_config_path}${pkg_config_path:+:}$dir/pkgconfig" + fi + ;; + esac + done + + `PKG_CONFIG_PATH=$PKG_CONFIG_PATH:$pkg_config_path ${pkg_config_prog_path} babeltrace = 1.1.0` + if test "$?" -eq 0 ; then + AC_DEFINE([HAVE_LIBBABELTRACE1_1_0], [1], [Define to 1 if you have libbabeltrace 1.1.0]) + fi fi fi diff --git a/gdb/ctf.c b/gdb/ctf.c index df645c0..dd115f4 100644 --- a/gdb/ctf.c +++ b/gdb/ctf.c @@ -623,11 +623,6 @@ ctf_write_definition_end (struct trace_file_writer *self) self->ops->frame_ops->end (self); } -/* The minimal file size of data stream. It is required by - babeltrace. */ - -#define CTF_FILE_MIN_SIZE 4096 - /* This is the implementation of trace_file_write_ops method end. */ @@ -637,10 +632,18 @@ ctf_end (struct trace_file_writer *self) struct ctf_trace_file_writer *writer = (struct ctf_trace_file_writer *) self; gdb_assert (writer->tcs.content_size == 0); - /* The babeltrace requires or assumes that the size of datastream - file is greater than 4096 bytes. If we don't generate enough - packets and events, create a fake packet which has zero event, - to use up the space. */ + +#if HAVE_LIBBABELTRACE1_1_0 + /* The minimal file size of data stream. It is required by + babeltrace. */ + +#define CTF_FILE_MIN_SIZE 4096 + + /* The babeltrace-1.1.0 requires or assumes that the size of datastream + file is greater than 4096 bytes. This was fixed after 1.1.0 release. + See https://bugs.lttng.org/issues/450 + If we don't generate enough packets and events, create a fake packet + which has zero event, to use up the space. */ if (writer->tcs.packet_start < CTF_FILE_MIN_SIZE) { uint32_t u32; @@ -681,6 +684,7 @@ ctf_end (struct trace_file_writer *self) ctf_save_write (&writer->tcs, &b, 1); } } +#endif /* HAVE_LIBBABELTRACE1_1_0 */ } /* This is the implementation of trace_frame_write_ops method -- 1.9.3 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: --with-babeltrace generates many FAILs 2014-08-19 12:43 ` --with-babeltrace generates many FAILs Yao Qi @ 2014-08-19 14:08 ` Jan Kratochvil 2014-08-20 4:06 ` Yao Qi 0 siblings, 1 reply; 11+ messages in thread From: Jan Kratochvil @ 2014-08-19 14:08 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On Tue, 19 Aug 2014 14:39:26 +0200, Yao Qi wrote: > I can reproduce it on my Fedora 20 x86 machine. Let me know it > works for you or not. Yes, it works for me (Fedora 21pre x86_64). Just suggesting: * '#if HAVE_LIBBABELTRACE1_1_0' could have a comment that >=1.1.1 rejects the faked packet (what you described in the mail but not in the patch). * It is always better to check for feature/defect than to check for version. For example because various distros backport various fixes (unfortunately including their possible regressions/defects) and so version checks may be misleading then. At least in this case it seems to me as possible to check how libbacktrace behaves from configure; although maybe it is not easy enough, not sure. Thanks, Jan ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: --with-babeltrace generates many FAILs 2014-08-19 14:08 ` Jan Kratochvil @ 2014-08-20 4:06 ` Yao Qi 2014-08-20 9:41 ` Pedro Alves 0 siblings, 1 reply; 11+ messages in thread From: Yao Qi @ 2014-08-20 4:06 UTC (permalink / raw) To: Jan Kratochvil; +Cc: gdb-patches On 08/19/2014 10:07 PM, Jan Kratochvil wrote: > * '#if HAVE_LIBBABELTRACE1_1_0' could have a comment that >=1.1.1 rejects the > faked packet (what you described in the mail but not in the patch). Fixed. To be precise, >= 1.1.2 rejects the faked packet, 1.1.1 doesn't. See the table I posted. > * It is always better to check for feature/defect than to check for version. > For example because various distros backport various fixes (unfortunately > including their possible regressions/defects) and so version checks may be > misleading then. At least in this case it seems to me as possible to check > how libbacktrace behaves from configure; although maybe it is not easy > enough, not sure. In order to check libbabeltrace's behaviour in configure, we have to write a c program to generate CTF data and read the trace data via babeltrace or any program (using libbabeltrace) written by ourselves. It is not easy to do so. The patch is updated. OK to apply? -- Yao (é½å°§) Subject: [PATCH] Check babeltrace 1.1.0 When GDB uses recent version of babeltrace, such as 1.2.x, we'll see such error emitted from babeltrace library, (gdb) target ctf .../gdb/testsuite/gdb.trace/actions.ctf [error] Invalid CTF stream: content size is smaller than packet headers. [error] Stream index creation error. [error] Open file stream error. The problem can be reproduce out of GDB too, using babeltrace, $ babeltrace ./fake-packet.ctf/ [error] Invalid CTF stream: content size is smaller than packet headers. [error] Stream index creation error. [error] Open file stream error. Recent babeltrace library becomes more strict on CTF, and complains about one "faked packet" GDB adds, when saving trace data in ctf format from GDB. babeltrace 1.1.0 has a bug that it can't read trace data smaller than a certain size (see https://bugs.lttng.org/issues/450). We workaround it in GDB to append some meaningless data in a faked packet to make sure trace file is large enough (see ctf.c:ctf_end). The babeltrace issue was fixed in 1.1.1 release. However, babeltrace recent release (since 1.1.2) starts to complain about such faked packet. Here is a table shows that whether faked packet or no faked packet is supported by various babeltrace releases, faked packet no faked packet 1.1.0 Yes No 1.1.1 Yes Yes 1.1.2 No Yes 1.2.0 No Yes We decide to include the code to workaround 1.1.0 issue only if 1.1.0 is used. We choose pkg-config to check babeltrace's version in configure. gdb: 2014-08-20 Yao Qi <yao@codesourcery.com> * configure.ac: Disable babeltrace support if pkg-config is missing. Use pkg-config to check whether libbabeltrace is 1.1.0. * config.in: Regenerate. * configure: Regenerate. * ctf.c (CTF_FILE_MIN_SIZE): Remove. (ctf_end): Wrap the code with #if HAVE_LIBBABELTRACE1_1_0 #endif. [HAVE_LIBBABELTRACE1_1_0] (CTF_FILE_MIN_SIZE): New macro. --- gdb/config.in | 3 +++ gdb/configure | 25 +++++++++++++++++++++++++ gdb/configure.ac | 22 ++++++++++++++++++++++ gdb/ctf.c | 25 ++++++++++++++++--------- 4 files changed, 66 insertions(+), 9 deletions(-) diff --git a/gdb/config.in b/gdb/config.in index b853412..54152cd 100644 --- a/gdb/config.in +++ b/gdb/config.in @@ -183,6 +183,9 @@ /* Define if you have the babeltrace library. */ #undef HAVE_LIBBABELTRACE +/* Define to 1 if you have libbabeltrace 1.1.0 */ +#undef HAVE_LIBBABELTRACE1_1_0 + /* Define to 1 if you have the `dl' library (-ldl). */ #undef HAVE_LIBDL diff --git a/gdb/configure b/gdb/configure index 9253e28..d4e2c6e 100755 --- a/gdb/configure +++ b/gdb/configure @@ -14817,6 +14817,11 @@ $as_echo "$with_babeltrace" >&6; } if test "x$with_babeltrace" = "xno"; then { $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: babletrace support disabled; GDB is unable to read CTF data." >&5 $as_echo "$as_me: WARNING: babletrace support disabled; GDB is unable to read CTF data." >&2;} +elif test "${pkg_config_prog_path}" = "missing"; then + # pkg-config is used to check the version of libbabeltrace. If pkg-config + # is missing, we have to disable babeltrace support. + { $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: pkg-config not found, babletrace support disabled" >&5 +$as_echo "$as_me: WARNING: pkg-config not found, babletrace support disabled" >&2;} else # Append -Werror to CFLAGS so that configure can catch the warning # "assignment from incompatible pointer type", which is related to @@ -15307,6 +15312,26 @@ $as_echo "$LIBBABELTRACE" >&6; } { $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: babeltrace is missing or unusable; GDB is unable to read CTF data." >&5 $as_echo "$as_me: WARNING: babeltrace is missing or unusable; GDB is unable to read CTF data." >&2;} fi + else + # Need to know whether libbabeltrace is 1.1.0. + pkg_config_path= + for x in $LTLIBBABELTRACE; do + case "$x" in + -L*) + dir=`echo "X$x" | sed -e 's/^X-L//'` + if test -d "$dir/pkgconfig"; then + pkg_config_path="${pkg_config_path}${pkg_config_path:+:}$dir/pkgconfig" + fi + ;; + esac + done + + `PKG_CONFIG_PATH=$PKG_CONFIG_PATH:$pkg_config_path ${pkg_config_prog_path} babeltrace = 1.1.0` + if test "$?" -eq 0 ; then + +$as_echo "#define HAVE_LIBBABELTRACE1_1_0 1" >>confdefs.h + + fi fi fi diff --git a/gdb/configure.ac b/gdb/configure.ac index 61919b4..1d8d400 100644 --- a/gdb/configure.ac +++ b/gdb/configure.ac @@ -2420,6 +2420,10 @@ AC_MSG_RESULT([$with_babeltrace]) if test "x$with_babeltrace" = "xno"; then AC_MSG_WARN([babletrace support disabled; GDB is unable to read CTF data.]) +elif test "${pkg_config_prog_path}" = "missing"; then + # pkg-config is used to check the version of libbabeltrace. If pkg-config + # is missing, we have to disable babeltrace support. + AC_MSG_WARN([pkg-config not found, babletrace support disabled]) else # Append -Werror to CFLAGS so that configure can catch the warning # "assignment from incompatible pointer type", which is related to @@ -2450,6 +2454,24 @@ else else AC_MSG_WARN([babeltrace is missing or unusable; GDB is unable to read CTF data.]) fi + else + # Need to know whether libbabeltrace is 1.1.0. + pkg_config_path= + for x in $LTLIBBABELTRACE; do + case "$x" in + -L*) + dir=`echo "X$x" | sed -e 's/^X-L//'` + if test -d "$dir/pkgconfig"; then + pkg_config_path="${pkg_config_path}${pkg_config_path:+:}$dir/pkgconfig" + fi + ;; + esac + done + + `PKG_CONFIG_PATH=$PKG_CONFIG_PATH:$pkg_config_path ${pkg_config_prog_path} babeltrace = 1.1.0` + if test "$?" -eq 0 ; then + AC_DEFINE([HAVE_LIBBABELTRACE1_1_0], [1], [Define to 1 if you have libbabeltrace 1.1.0]) + fi fi fi diff --git a/gdb/ctf.c b/gdb/ctf.c index df645c0..684da50 100644 --- a/gdb/ctf.c +++ b/gdb/ctf.c @@ -623,11 +623,6 @@ ctf_write_definition_end (struct trace_file_writer *self) self->ops->frame_ops->end (self); } -/* The minimal file size of data stream. It is required by - babeltrace. */ - -#define CTF_FILE_MIN_SIZE 4096 - /* This is the implementation of trace_file_write_ops method end. */ @@ -637,10 +632,21 @@ ctf_end (struct trace_file_writer *self) struct ctf_trace_file_writer *writer = (struct ctf_trace_file_writer *) self; gdb_assert (writer->tcs.content_size == 0); - /* The babeltrace requires or assumes that the size of datastream - file is greater than 4096 bytes. If we don't generate enough - packets and events, create a fake packet which has zero event, - to use up the space. */ + +#if HAVE_LIBBABELTRACE1_1_0 + /* The babeltrace-1.1.0 requires or assumes that the size of datastream + file is greater than 4096 bytes. This was fixed after 1.1.0 release. + See https://bugs.lttng.org/issues/450 + If we don't generate enough packets and events, create a fake packet + which has zero event, to use up the space. However, babeltrace + release (since 1.1.2) starts to complain about such faked packet, + we include this workaround only for babeltrace 1.1.0. */ + + /* The minimal file size of data stream. It is required by + babeltrace. */ + +#define CTF_FILE_MIN_SIZE 4096 + if (writer->tcs.packet_start < CTF_FILE_MIN_SIZE) { uint32_t u32; @@ -681,6 +687,7 @@ ctf_end (struct trace_file_writer *self) ctf_save_write (&writer->tcs, &b, 1); } } +#endif /* HAVE_LIBBABELTRACE1_1_0 */ } /* This is the implementation of trace_frame_write_ops method -- 1.9.3 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: --with-babeltrace generates many FAILs 2014-08-20 4:06 ` Yao Qi @ 2014-08-20 9:41 ` Pedro Alves 2014-08-20 11:38 ` Yao Qi 0 siblings, 1 reply; 11+ messages in thread From: Pedro Alves @ 2014-08-20 9:41 UTC (permalink / raw) To: Yao Qi, Jan Kratochvil; +Cc: gdb-patches On 08/20/2014 05:02 AM, Yao Qi wrote: > The patch is updated. OK to apply? I'm wondering whether we really need all this complication for an old version of a library that is quite new-ish, and not relied on for anything really core? I'm wondering whether this check works on Windows, for example. As there's been fixed babeltrace versions for a while, I'd go with simply dropping the workaround, and have integrators build newer GDB with newer babeltrace. I suppose we have a testcase in our testsuite that fails if we remove the workaround and GDB is built with broken babeltrace? That should let the integrator know that it's building again a broken lib. IOW, why do we still need to support 1.1.0? Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: --with-babeltrace generates many FAILs 2014-08-20 9:41 ` Pedro Alves @ 2014-08-20 11:38 ` Yao Qi 2014-08-20 13:56 ` Pedro Alves 0 siblings, 1 reply; 11+ messages in thread From: Yao Qi @ 2014-08-20 11:38 UTC (permalink / raw) To: Pedro Alves, Jan Kratochvil; +Cc: gdb-patches On 08/20/2014 05:41 PM, Pedro Alves wrote: > As there's been fixed babeltrace versions for a while, I'd go with > simply dropping the workaround, and have integrators build newer > GDB with newer babeltrace. I suppose we have a testcase in our > testsuite that fails if we remove the workaround and GDB is built with > broken babeltrace? That should let the integrator know that it's > building again a broken lib. Yes, we have such test case, such as actions.exp, at least. Without the workaround, GDB with libbabeltrace 1.1.0 will fail in actions.exp. However, is it a good idea that let test failure signal a wrong version lib is used? I am not sure. It is the configure's job to check whether the library is wrong or broken. > > IOW, why do we still need to support 1.1.0? No special reason, 1.1.0 was just used when I did the CTF work in GDB, and was used on my laptop since then. IIRC, 1.1.0 was released in 2013 March, so it isn't very old and it might be used somewhere. Shouldn't we be conservative in this case? In general, GDB and GDBserver uses a set of libraries, what are the criteria of 1. stop supporting a version of a library, such as libbabeltrace 1.1.0 2. stop supporting or using a library, such as the UST stuff in GDBserver, -- Yao (é½å°§) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: --with-babeltrace generates many FAILs 2014-08-20 11:38 ` Yao Qi @ 2014-08-20 13:56 ` Pedro Alves 2014-08-21 4:57 ` Yao Qi 0 siblings, 1 reply; 11+ messages in thread From: Pedro Alves @ 2014-08-20 13:56 UTC (permalink / raw) To: Yao Qi, Jan Kratochvil; +Cc: gdb-patches On 08/20/2014 12:34 PM, Yao Qi wrote: > On 08/20/2014 05:41 PM, Pedro Alves wrote: >> As there's been fixed babeltrace versions for a while, I'd go with >> simply dropping the workaround, and have integrators build newer >> GDB with newer babeltrace. I suppose we have a testcase in our >> testsuite that fails if we remove the workaround and GDB is built with >> broken babeltrace? That should let the integrator know that it's >> building again a broken lib. > > Yes, we have such test case, such as actions.exp, at least. Without > the workaround, GDB with libbabeltrace 1.1.0 will fail in actions.exp. > However, is it a good idea that let test failure signal a wrong version > lib is used? IMO, in this case, it's sufficient. There are point releases that fix the issue, which are supposedly compatible with 1.1.0 -- updating those is just a normal maintenance issue, doesn't involve any sort of porting in gdb, etc. > I am not sure. It is the configure's job to check whether > the library is wrong or broken. Note that there's a fundamental issue with the workaround -- it assumes that the gdb that generates CTF is the same that consumes it. It's certainly easy to imagine a CTF file generated by a gdb not affected by the bug be consumed by a gdb with the bug. Or the other way around. >> IOW, why do we still need to support 1.1.0? > > No special reason, 1.1.0 was just used when I did the CTF work in GDB, > and was used on my laptop since then. IIRC, 1.1.0 was released in 2013 > March, so it isn't very old and it might be used somewhere. > Shouldn't we be conservative in this case? My point is exactly that this is new-ish code, and a moving target. If bugs are fixed promptly, why go through trouble working around them in gdb instead of just updating the version in the distro? It'd be different if we were talking about a one liner instead of a good chunk of autoconf/pkg-config glue. I'm not strictly objecting your patch, but it does look like the kind of code that: #1 will need further tweaking in the future; #2 will become obsolete anyway as time passes anyway. Couple that with the generator != consumer issue, and it raises eyebrows. > In general, GDB and GDBserver uses a set of libraries, what are the > criteria of > > 1. stop supporting a version of a library, such as libbabeltrace 1.1.0 When the burden of supporting it outweighs the benefits? > 2. stop supporting or using a library, such as the UST stuff in GDBserver, When nobody wants to maintain it anymore (I personally don't)? Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: --with-babeltrace generates many FAILs 2014-08-20 13:56 ` Pedro Alves @ 2014-08-21 4:57 ` Yao Qi 2014-08-21 12:48 ` Pedro Alves 0 siblings, 1 reply; 11+ messages in thread From: Yao Qi @ 2014-08-21 4:57 UTC (permalink / raw) To: Pedro Alves, Jan Kratochvil; +Cc: gdb-patches On 08/20/2014 09:56 PM, Pedro Alves wrote: > Note that there's a fundamental issue with the workaround -- it > assumes that the gdb that generates CTF is the same that consumes it. > It's certainly easy to imagine a CTF file generated by a gdb not > affected by the bug be consumed by a gdb with the bug. Or the other way > around. No, the workaround doesn't have such assumption. The trace file generated by GDB with 1.1.0 has a faked packet, and the trace file can be read by GDB with 1.1.1. Although the faked packet is not necessary, it doesn't make any error. Since 1.1.2, babeltrace becomes more restrict on CTF, and starts to complain the faked packet GDB adds. The code in GDB was written when 1.1.0 was released, at that moment, nobody knows how 1.1.2 does. Likewise, we don't know how does babeltrace behave in 2015. We followed the CTF spec to generate CTF data, and ideally babeltrace of any version can parse them. However, it is not surprise to me that two implementations (producer and consumer) have different understandings on some parts of the specification. > >>> >> IOW, why do we still need to support 1.1.0? >> > >> > No special reason, 1.1.0 was just used when I did the CTF work in GDB, >> > and was used on my laptop since then. IIRC, 1.1.0 was released in 2013 >> > March, so it isn't very old and it might be used somewhere. >> > Shouldn't we be conservative in this case? > My point is exactly that this is new-ish code, and a moving target. > If bugs are fixed promptly, why go through trouble working around > them in gdb instead of just updating the version in the distro? I prefer the latter, but I am not a distro integrator. > It'd be different if we were talking about a one liner instead of > a good chunk of autoconf/pkg-config glue. > > I'm not strictly objecting your patch, but it does look like > the kind of code that: #1 will need further tweaking in the future; > #2 will become obsolete anyway as time passes anyway. Couple that > with the generator != consumer issue, and it raises eyebrows. > Yeah, the patch isn't perfect. I am fine with your suggestion, and the patch below removes the workaround. >> > In general, GDB and GDBserver uses a set of libraries, what are the >> > criteria of >> > >> > 1. stop supporting a version of a library, such as libbabeltrace 1.1.0 > When the burden of supporting it outweighs the benefits? > That is still too abstract to operate, IMO. >> > 2. stop supporting or using a library, such as the UST stuff in GDBserver, > When nobody wants to maintain it anymore (I personally don't)? OK, GDBserver UST support depends on a quite old ust library, I'll take a look further. -- Yao (é½å°§) Subject: [PATCH] Remove workaround to libbabeltrace 1.1.0 issue When GDB uses recent version of babeltrace, such as 1.2.x, we'll see such error emitted from babeltrace library, (gdb) target ctf .../gdb/testsuite/gdb.trace/actions.ctf [error] Invalid CTF stream: content size is smaller than packet headers. [error] Stream index creation error. [error] Open file stream error. The problem can be reproduce out of GDB too, using babeltrace, $ babeltrace ./fake-packet.ctf/ [error] Invalid CTF stream: content size is smaller than packet headers. [error] Stream index creation error. [error] Open file stream error. Recent babeltrace library becomes more strict on CTF, and complains about one "faked packet" GDB adds, when saving trace data in ctf format from GDB. babeltrace 1.1.0 has a bug that it can't read trace data smaller than a certain size (see https://bugs.lttng.org/issues/450). We workaround it in GDB to append some meaningless data in a faked packet to make sure trace file is large enough (see ctf.c:ctf_end). The babeltrace issue was fixed in 1.1.1 release. However, babeltrace recent release (since 1.1.2) starts to complain about such faked packet. Here is a table shows that whether faked packet or no faked packet is "supported" by various babeltrace releases, faked packet no faked packet 1.1.0 Yes No 1.1.1 Yes Yes 1.1.2 No Yes 1.2.0 No Yes We decide to get rid of this workaround in GDB, and people can build GDB with libbabeltrace >= 1.1.1. In this way, both configure and ctf.c is simpler. Run gdb.trace/* tests in the following combinations: wo/ this pattch 1.1.0 w/ this patch 1.1.1 w/ this patch 1.1.2 w/ this patch 1.2.0 No test results change. gdb: 2014-08-21 Yao Qi <yao@codesourcery.com> * ctf.c (CTF_FILE_MIN_SIZE): Remove. (ctf_end): Remove code. --- gdb/ctf.c | 49 ------------------------------------------------- 1 file changed, 49 deletions(-) diff --git a/gdb/ctf.c b/gdb/ctf.c index df645c0..9fd8c04 100644 --- a/gdb/ctf.c +++ b/gdb/ctf.c @@ -623,11 +623,6 @@ ctf_write_definition_end (struct trace_file_writer *self) self->ops->frame_ops->end (self); } -/* The minimal file size of data stream. It is required by - babeltrace. */ - -#define CTF_FILE_MIN_SIZE 4096 - /* This is the implementation of trace_file_write_ops method end. */ @@ -637,50 +632,6 @@ ctf_end (struct trace_file_writer *self) struct ctf_trace_file_writer *writer = (struct ctf_trace_file_writer *) self; gdb_assert (writer->tcs.content_size == 0); - /* The babeltrace requires or assumes that the size of datastream - file is greater than 4096 bytes. If we don't generate enough - packets and events, create a fake packet which has zero event, - to use up the space. */ - if (writer->tcs.packet_start < CTF_FILE_MIN_SIZE) - { - uint32_t u32; - - /* magic. */ - u32 = CTF_MAGIC; - ctf_save_write_uint32 (&writer->tcs, u32); - - /* content_size. */ - u32 = 0; - ctf_save_write_uint32 (&writer->tcs, u32); - - /* packet_size. */ - u32 = 12; - if (writer->tcs.packet_start + u32 < CTF_FILE_MIN_SIZE) - u32 = CTF_FILE_MIN_SIZE - writer->tcs.packet_start; - - u32 *= TARGET_CHAR_BIT; - ctf_save_write_uint32 (&writer->tcs, u32); - - /* tpnum. */ - u32 = 0; - ctf_save_write (&writer->tcs, (gdb_byte *) &u32, 2); - - /* Enlarge the file to CTF_FILE_MIN_SIZE is it is still less - than that. */ - if (CTF_FILE_MIN_SIZE - > (writer->tcs.packet_start + writer->tcs.content_size)) - { - gdb_byte b = 0; - - /* Fake the content size to avoid assertion failure in - ctf_save_fseek. */ - writer->tcs.content_size = (CTF_FILE_MIN_SIZE - - 1 - writer->tcs.packet_start); - ctf_save_fseek (&writer->tcs, CTF_FILE_MIN_SIZE - 1, - SEEK_SET); - ctf_save_write (&writer->tcs, &b, 1); - } - } } /* This is the implementation of trace_frame_write_ops method -- 1.9.3 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: --with-babeltrace generates many FAILs 2014-08-21 4:57 ` Yao Qi @ 2014-08-21 12:48 ` Pedro Alves 2014-08-22 3:52 ` Yao Qi 0 siblings, 1 reply; 11+ messages in thread From: Pedro Alves @ 2014-08-21 12:48 UTC (permalink / raw) To: Yao Qi, Jan Kratochvil; +Cc: gdb-patches On 08/21/2014 05:53 AM, Yao Qi wrote: > On 08/20/2014 09:56 PM, Pedro Alves wrote: >> > Note that there's a fundamental issue with the workaround -- it >> > assumes that the gdb that generates CTF is the same that consumes it. >> > It's certainly easy to imagine a CTF file generated by a gdb not >> > affected by the bug be consumed by a gdb with the bug. Or the other way >> > around. > No, the workaround doesn't have such assumption. The trace file > generated by GDB with 1.1.0 has a faked packet, and the trace file can > be read by GDB with 1.1.1. Although the faked packet is not necessary, > it doesn't make any error. > > Since 1.1.2, babeltrace becomes more restrict on CTF, and starts to > complain the faked packet GDB adds. The code in GDB was written when > 1.1.0 was released, at that moment, nobody knows how 1.1.2 does. > Likewise, we don't know how does babeltrace behave in 2015. We > followed the CTF spec to generate CTF data, and ideally babeltrace of > any version can parse them. However, it is not surprise to me that > two implementations (producer and consumer) have different > understandings on some parts of the specification. Right. From your description, newer babeltrace rejects the workaround because it's more strict now, which I read as the workaround somehow knowingly violating the CTF spec, hence, the workaround assuming babeltrace or other CTF consumers would remain lax. The point was that CTF generated by a GDB with 1.1.0 wouldn't be readable by a GDB with 1.1.2, and vice-versa. If it remains in place, the assumption that we can infer whether a workaround will be necessary or usable from the babeltrace version that is used to build the producer GDB isn't strictly correct. But maybe "assumption" was a too strong word. (I hope you didn't feel I was pointing fingers at you or something. Certainly not intended!) >> > I'm not strictly objecting your patch, but it does look like >> > the kind of code that: #1 will need further tweaking in the future; >> > #2 will become obsolete anyway as time passes anyway. Couple that >> > with the generator != consumer issue, and it raises eyebrows. >> > > Yeah, the patch isn't perfect. I am fine with your suggestion, and the > patch below removes the workaround. Alright, onward! Thanks. >>>> >> > In general, GDB and GDBserver uses a set of libraries, what are the >>>> >> > criteria of >>>> >> > >>>> >> > 1. stop supporting a version of a library, such as libbabeltrace 1.1.0 >> > When the burden of supporting it outweighs the benefits? >> > > That is still too abstract to operate, IMO. Kind of just as abstract as the question. :-) The "benefit" is being able to take advantage of the library's features. The "burden" is the cost/effort required to use the library and workaround any issues it might have. When there are bugfix releases that postdate a given version, I think the effort to support the older unfixed version isn't so much worth it. The bugs have been fixed at the source, and we can simply require builders/integrators upgrade to the latest bugfix release (that's what bugfix/stable releases are for!). Thus, IMO, the existence of bugfix releases makes the burden of maintaining support for old unfixed versions outweigh the benefits, as we can alternatively just say "not our problem: build against the latest fixed version instead, please". > Subject: [PATCH] Remove workaround to libbabeltrace 1.1.0 issue This looks good to me. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: --with-babeltrace generates many FAILs 2014-08-21 12:48 ` Pedro Alves @ 2014-08-22 3:52 ` Yao Qi 2014-08-26 20:27 ` [for 7.8?] " Jan Kratochvil 0 siblings, 1 reply; 11+ messages in thread From: Yao Qi @ 2014-08-22 3:52 UTC (permalink / raw) To: Pedro Alves; +Cc: Jan Kratochvil, gdb-patches Pedro Alves <palves@redhat.com> writes: > Right. From your description, newer babeltrace rejects the workaround > because it's more strict now, which I read as the workaround > somehow knowingly violating the CTF spec, hence, the workaround > assuming babeltrace or other CTF consumers would remain lax. The > point was that CTF generated by a GDB with 1.1.0 wouldn't be readable > by a GDB with 1.1.2, and vice-versa. If it remains in place, the > assumption that we can infer whether a workaround will be necessary > or usable from the babeltrace version that is used to build the > producer GDB isn't strictly correct. But maybe "assumption" was a > too strong word. (I hope you didn't feel I was pointing fingers at > you or something. Certainly not intended!) No, I didn't feel that :) I'd like people know the status and problem clearly and make people on the same page. >>>>> >> > In general, GDB and GDBserver uses a set of libraries, what are the >>>>> >> > criteria of >>>>> >> > >>>>> >> > 1. stop supporting a version of a library, such as libbabeltrace 1.1.0 >>> > When the burden of supporting it outweighs the benefits? >>> > >> That is still too abstract to operate, IMO. > > Kind of just as abstract as the question. :-) > > The "benefit" is being able to take advantage of the library's features. > The "burden" is the cost/effort required to use the library and workaround > any issues it might have. When there are bugfix releases that postdate > a given version, I think the effort to support the older unfixed version > isn't so much worth it. The bugs have been fixed at the source, and we > can simply require builders/integrators upgrade to the latest bugfix > release (that's what bugfix/stable releases are for!). Thus, IMO, the > existence of bugfix releases makes the burden of maintaining support > for old unfixed versions outweigh the benefits, as we can alternatively > just say "not our problem: build against the latest fixed version > instead, please". This is clear to me. > >> Subject: [PATCH] Remove workaround to libbabeltrace 1.1.0 issue > > This looks good to me. > Patch is pushed in. Thanks. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 11+ messages in thread
* [for 7.8?] Re: --with-babeltrace generates many FAILs 2014-08-22 3:52 ` Yao Qi @ 2014-08-26 20:27 ` Jan Kratochvil 2014-08-27 8:27 ` Yao Qi 0 siblings, 1 reply; 11+ messages in thread From: Jan Kratochvil @ 2014-08-26 20:27 UTC (permalink / raw) To: Yao Qi; +Cc: Pedro Alves, gdb-patches, Joel Brobecker On Fri, 22 Aug 2014 05:48:30 +0200, Yao Qi wrote: > Patch is pushed in. Thanks. Maybe could you push it also for 7.8.1? https://sourceware.org/gdb/wiki/GDB_7.8_Release At least I have pushed there first the CFLAGS=-Wall fix which disclosed this bug a bit more. Jan ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [for 7.8?] Re: --with-babeltrace generates many FAILs 2014-08-26 20:27 ` [for 7.8?] " Jan Kratochvil @ 2014-08-27 8:27 ` Yao Qi 0 siblings, 0 replies; 11+ messages in thread From: Yao Qi @ 2014-08-27 8:27 UTC (permalink / raw) To: Jan Kratochvil; +Cc: Pedro Alves, gdb-patches, Joel Brobecker On 08/27/2014 04:27 AM, Jan Kratochvil wrote: > Maybe could you push it also for 7.8.1? > https://sourceware.org/gdb/wiki/GDB_7.8_Release Yeah, that is reasonable to me. Patch is pushed in to 7.8 branch. The wikipage is updated too. -- Yao (é½å°§) ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-08-27 8:27 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20140816204614.GA7000@host2.jankratochvil.net>
2014-08-19 12:43 ` --with-babeltrace generates many FAILs Yao Qi
2014-08-19 14:08 ` Jan Kratochvil
2014-08-20 4:06 ` Yao Qi
2014-08-20 9:41 ` Pedro Alves
2014-08-20 11:38 ` Yao Qi
2014-08-20 13:56 ` Pedro Alves
2014-08-21 4:57 ` Yao Qi
2014-08-21 12:48 ` Pedro Alves
2014-08-22 3:52 ` Yao Qi
2014-08-26 20:27 ` [for 7.8?] " Jan Kratochvil
2014-08-27 8:27 ` Yao Qi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox