Re: [PATCH 2/2] Find the next matched trace file in 'tfile target'.

[Pedro Alves <palves at redhat dot com>]

On 02/25/2013 02:57 AM, Yao Qi wrote:
> The previous patch exposes a bug in tfile target when finding a trace
> frame.  Every time, GDB will scan tfile from the starting offset and
> initialize trace frame number to zero.  When TYPE is not tfind_number,
> it means GDB wants to find the *next* matched trace frame of current
> one.  So we need to check the tfile trace frame number iterator is
> greater than the current trace frame number (which means *next*).
> This is mainly what this patch does.

Thanks.  Good catch.

> diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
> index ca104aa..f7a3650 100644
> --- a/gdb/tracepoint.c
> +++ b/gdb/tracepoint.c
> @@ -4324,22 +4324,34 @@ tfile_trace_find (enum trace_find_type type, int num,
>  	  break;
>  	case tfind_pc:
>  	  tfaddr = tfile_get_traceframe_address (tframe_offset);
> -	  if (tfaddr == addr1)
> +	  if (tfaddr == addr1
> +	      /* Looks for the next trace frame if matched.  */
> +	      && (tfnum > traceframe_number
> +		  || (tfnum == traceframe_number && tfnum == 0)))
>  	    found = 1;
>  	  break;
>  	case tfind_tp:
>  	  tp = get_tracepoint (num);
> -	  if (tp && tpnum == tp->number_on_target)
> +	  if (tp && tpnum == tp->number_on_target
> +	      /* Looks for the next trace frame if matched.  */
> +	      && (tfnum > traceframe_number
> +		  || (tfnum == traceframe_number && tfnum == 0)))
>  	    found = 1;
>  	  break;
>  	case tfind_range:
>  	  tfaddr = tfile_get_traceframe_address (tframe_offset);
> -	  if (addr1 <= tfaddr && tfaddr <= addr2)
> +	  if (addr1 <= tfaddr && tfaddr <= addr2
> +	      /* Looks for the next trace frame if matched.  */
> +	      && (tfnum > traceframe_number
> +		  || (tfnum == traceframe_number && tfnum == 0)))
>  	    found = 1;
>  	  break;
>  	case tfind_outside:
>  	  tfaddr = tfile_get_traceframe_address (tframe_offset);
> -	  if (!(addr1 <= tfaddr && tfaddr <= addr2))
> +	  if (!(addr1 <= tfaddr && tfaddr <= addr2)
> +	      /* Looks for the next trace frame if matched.  */
> +	      && (tfnum > traceframe_number
> +		  || (tfnum == traceframe_number && tfnum == 0)))

I'm confused on why this bit of the predicate

 (tfnum == traceframe_number && tfnum == 0)

is necessary.  traceframe_number is -1 when not looking
at a traceframe yet, so "tfnum > traceframe_number"
should be sufficient, no?

I find it clearer to move the frame skipping a bit higher
up, even before the specific tfind tp/range/etc. matching.
Doing it this way also avoids unnecessary read/lseek system
calls done by tfile_get_traceframe_address.

WDYT?

commit f837913c7a153a0e40a15f4a283deea201cbcbe7
Author: Pedro Alves <palves@redhat.com>
Date:   Tue Feb 26 19:43:21 2013 +0000

    Find the next matched trace file in 'tfile target'.
    
    The previous patch exposes a bug in the tfile target when finding a
    trace frame.  Every time, GDB will scan tfile from the starting offset
    and look for a matching trace frame starting from frame zero.  But,
    except when TYPE is tfind_number, GDB wants to find the *next* matched
    trace frame after the current selected frame.  So we need to check the
    tfile trace frame number iterator is greater than the current trace
    frame number (which means *next*), meaning skip all frames up to and
    including the current.
    
    Regression tested on x86_64 Fedora 17.
    
    gdb/
    2013-02-25  Yao Qi  <yao@codesourcery.com>
    	    Pedro Alves  <palves@redhat.com>
    
    	* tracepoint.c (tfile_trace_find): For tfind
    	pc/tp/range/outside, look for the next trace frame instead of
    	always starting from frame 0.

diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index ca104aa..9a80aa3 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -4316,35 +4316,46 @@ tfile_trace_find (enum trace_find_type type, int num,
                                      ((gdb_byte *) &data_size, 4,
 				      gdbarch_byte_order (target_gdbarch ()));
       offset += 4;
-      switch (type)
+
+      if (type == tfind_number)
 	{
-	case tfind_number:
+	  /* Looking for a specific trace frame.  */
 	  if (tfnum == num)
 	    found = 1;
-	  break;
-	case tfind_pc:
-	  tfaddr = tfile_get_traceframe_address (tframe_offset);
-	  if (tfaddr == addr1)
-	    found = 1;
-	  break;
-	case tfind_tp:
-	  tp = get_tracepoint (num);
-	  if (tp && tpnum == tp->number_on_target)
-	    found = 1;
-	  break;
-	case tfind_range:
-	  tfaddr = tfile_get_traceframe_address (tframe_offset);
-	  if (addr1 <= tfaddr && tfaddr <= addr2)
-	    found = 1;
-	  break;
-	case tfind_outside:
-	  tfaddr = tfile_get_traceframe_address (tframe_offset);
-	  if (!(addr1 <= tfaddr && tfaddr <= addr2))
-	    found = 1;
-	  break;
-	default:
-	  internal_error (__FILE__, __LINE__, _("unknown tfind type"));
 	}
+      else
+	{
+	  /* Start from the _next_ trace frame.  */
+	  if (tfnum > traceframe_number)
+	    {
+	      switch (type)
+		{
+		case tfind_pc:
+		  tfaddr = tfile_get_traceframe_address (tframe_offset);
+		  if (tfaddr == addr1)
+		    found = 1;
+		  break;
+		case tfind_tp:
+		  tp = get_tracepoint (num);
+		  if (tp && tpnum == tp->number_on_target)
+		    found = 1;
+		  break;
+		case tfind_range:
+		  tfaddr = tfile_get_traceframe_address (tframe_offset);
+		  if (addr1 <= tfaddr && tfaddr <= addr2)
+		    found = 1;
+		  break;
+		case tfind_outside:
+		  tfaddr = tfile_get_traceframe_address (tframe_offset);
+		  if (!(addr1 <= tfaddr && tfaddr <= addr2))
+		    found = 1;
+		  break;
+		default:
+		  internal_error (__FILE__, __LINE__, _("unknown tfind type"));
+		}
+	    }
+	}
+
       if (found)
 	{
 	  if (tpp)