Re: --with-babeltrace generates many FAILs

[Yao Qi <yao at codesourcery dot com>]

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