Re: [unavailable values part 1, 16/17] don't merge almost but not quite adjacent memory ranges to collect

[Pedro Alves <pedro@codesourcery.com>]

On Monday 14 February 2011 12:01:16, Jan Kratochvil wrote:
> On Mon, 07 Feb 2011 15:35:20 +0100, Pedro Alves wrote:
> > --- src.orig/gdb/tracepoint.c	2011-02-07 13:17:26.276706003 +0000
> > +++ src/gdb/tracepoint.c	2011-02-07 13:27:53.276706002 +0000
> > @@ -841,13 +841,12 @@ memrange_sortmerge (struct collection_li
> >      {
> >        for (a = 0, b = 1; b < memranges->next_memrange; b++)
> >  	{
> > -	  if (memranges->list[a].type == memranges->list[b].type &&
> > -	      memranges->list[b].start - memranges->list[a].end <=
> > -	      MAX_REGISTER_SIZE)
> > +	  /* If memrange b overlaps or is adjacent to memrange a,
> > +	     merge them.  */
> > +	  if (memranges->list[a].type == memranges->list[b].type
> > +	      && memranges->list[b].start <= memranges->list[a].end)
> >  	    {
> > -	      /* memrange b starts before memrange a ends; merge them.  */
> > -	      if (memranges->list[b].end > memranges->list[a].end)
> > -		memranges->list[a].end = memranges->list[b].end;
> > +	      memranges->list[a].end = memranges->list[b].end;
> >  	      continue;		/* next b, same a */
> >  	    }
> >  	  a++;			/* next a */
> 
> It is an unrelated issue to this patch but this function is not a general
> normalizer for overlapping ranges, with bug(s) similar to
> normalize_mem_ranges.  But maybe it does not have to be so general, all the
> possible contents of the tracing protocol are unknown to me.

Whoops, nice catch.  It can happen.  E.g.,

actions
>collect {char[64]}0x400640
>collect {char[32]}0x400640
end

we'd tell the target to collect [0x400640, 0x400640+32) instead
of [0x400640, 0x400640+64).

While writting the test in the patch below I tripped
on an internal error:

 >collect {int [4]}globalarr2
 ../../src/gdb/ax-gdb.c:2053: internal-error: gen_expr: OP_MEMVAL operand isn't an rvalue???
 A problem internal to GDB has been detected,
 further debugging may prove unreliable.

... bah.

I applied the patch below.  The test fails without the fix.

Thanks!

-- 
Pedro Alves

2011-02-16  Pedro Alves  <pedro@codesourcery.com>
	    Jan Kratochvil  <jan.kratochvil@redhat.com>

	gdb/
	* tracepoint.c (memrange_sortmerge): Fix list A's end calculation.

2011-02-16  Pedro Alves  <pedro@codesourcery.com>

	gdb/testsuite/
	* collection.c (globalarr2): New global.
	(main): Initialize it before collecting, and and clear it
	afterwards.
	* collection.exp (gdb_collect_globals_test): Test collecting
	overlapping memory ranges.

---
 gdb/testsuite/gdb.trace/collection.c   |    6 ++++++
 gdb/testsuite/gdb.trace/collection.exp |   24 +++++++++++++++++++++++-
 gdb/tracepoint.c                       |    3 ++-
 3 files changed, 31 insertions(+), 2 deletions(-)

Index: src/gdb/testsuite/gdb.trace/collection.c
===================================================================
--- src.orig/gdb/testsuite/gdb.trace/collection.c	2011-02-16 12:48:45.436002004 +0000
+++ src/gdb/testsuite/gdb.trace/collection.c	2011-02-16 12:49:02.446001996 +0000
@@ -26,6 +26,7 @@ double       globald;
 test_struct  globalstruct;
 test_struct *globalp;
 int          globalarr[16];
+int          globalarr2[4];
 
 struct global_pieces {
   unsigned int a;
@@ -237,6 +238,9 @@ main (argc, argv, envp)
   for (i = 0; i < 15; i++)
     globalarr[i] = i;
 
+  for (i = 0; i < 4; i++)
+    globalarr2[i] = i;
+
   mystruct.memberc = 101;
   mystruct.memberi = 102;
   mystruct.memberf = 103.3;
@@ -283,6 +287,8 @@ main (argc, argv, envp)
   globalp = 0;
   for (i = 0; i < 15; i++)
     globalarr[i] = 0;
+  for (i = 0; i < 4; i++)
+    globalarr2[i] = 0;
 
   end ();
   return 0;
Index: src/gdb/testsuite/gdb.trace/collection.exp
===================================================================
--- src.orig/gdb/testsuite/gdb.trace/collection.exp	2011-02-16 12:48:45.436002004 +0000
+++ src/gdb/testsuite/gdb.trace/collection.exp	2011-02-16 12:49:02.446001996 +0000
@@ -457,13 +457,29 @@ proc gdb_collect_globals_test { } {
 	}
     }
 
+    # Use use this to test collecting overlapping memory ranges
+    # (making use of UNOP_MEMVAL, as objects don't usually overlap
+    # other objects).  Note that globalarr2 should not be collected in
+    # any other way so that a regression test below can be effective.
+
+    set globalarr2_addr ""
+    set test "get address of globalarr2"
+    gdb_test_multiple "p /x &globalarr2" $test {
+	-re " = (0x\[0-9a-f\]+)\r\n$gdb_prompt $" {
+	    set globalarr2_addr $expect_out(1,string)
+	    pass $test
+	}
+    }
+
     gdb_test "trace $testline" \
 	    "Tracepoint \[0-9\]+ at .*" \
 	    "collect globals: set tracepoint"
     gdb_trace_setactions "collect globals: define actions" \
 	    "" \
 	    "collect globalc, globali, globalf, globald" "^$" \
-	    "collect globalstruct, globalp, globalarr" "^$"
+	    "collect globalstruct, globalp, globalarr" "^$" \
+	    "collect \{int \[4\]\}$globalarr2_addr" "^$" \
+	    "collect \{int \[2\]\}$globalarr2_addr" "^$"
 
     # Begin the test.
     run_trace_experiment "globals" globals_test_func
@@ -508,6 +524,12 @@ proc gdb_collect_globals_test { } {
 	    "\\$\[0-9\]+ = 3$cr" \
 	    "collect globals: collected global array element #3"
 
+    # Check that we didn't mess up sort&merging memory ranges to
+    # collect.
+    gdb_test "print globalarr2" \
+	"\\$\[0-9\]+ = \\{0, 1, 2, 3\\}$cr" \
+	"collect globals: collected global array 2"
+
     gdb_test "tfind none" \
 	    "#0  end .*" \
 	    "collect globals: cease trace debugging"
Index: src/gdb/tracepoint.c
===================================================================
--- src.orig/gdb/tracepoint.c	2011-02-16 12:51:44.000000000 +0000
+++ src/gdb/tracepoint.c	2011-02-16 12:51:54.236001996 +0000
@@ -846,7 +846,8 @@ memrange_sortmerge (struct collection_li
 	  if (memranges->list[a].type == memranges->list[b].type
 	      && memranges->list[b].start <= memranges->list[a].end)
 	    {
-	      memranges->list[a].end = memranges->list[b].end;
+	      if (memranges->list[b].end > memranges->list[a].end)
+		memranges->list[a].end = memranges->list[b].end;
 	      continue;		/* next b, same a */
 	    }
 	  a++;			/* next a */