Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
* [unavailable values part 1, 16/17] don't merge almost but not quite adjacent memory ranges to collect
@ 2011-02-07 14:35 Pedro Alves
  2011-02-14 12:01 ` Jan Kratochvil
  0 siblings, 1 reply; 13+ messages in thread
From: Pedro Alves @ 2011-02-07 14:35 UTC (permalink / raw)
  To: gdb-patches

While writing a test for a rtti related patch, I noticed that
the test was failing because somehow, GDB was managing to print
a pointer's value, although the tracepoint wasn't told to collect
it.  The reason the pointer was being collected, turned out to
be that when GDB is encoding the tracepoint actions, it merges
overlapping and adjacent ranges that should be collected into
a single memory collection request (fine), but, it also
merges ranges if the hole between them is narrower than
or equal to MAX_REGISTER_SIZE.  E.g., if the tracepoint's
actions wants to collect both:

 [0x10000, 0x10004) and  [0x10004, 0x10008), 

then GDB merges the actions into a single collect memory
action like:

 [0x10000, 0x10008)

That's fine.  But if the tracepoint wants to
collect 

 [0x10000, 0x10004) and  [0x10008, 0x10010),

then GDB still merges these into

 [0x10000, 0x10010)

thus collecting [0x10004, 0x10008) as well.

My guess is that this was an attempt at minimizing trace
buffer overhead on the target side.  But, this is bogus
for several reasons:

 - MAX_REGISTER_SIZE is really a wrong constant for this.
   GDB's internal maximum register size supported is really
   unrelated to this.  It has been recently bumped to 64
   recently.  That's a lot of waste in the trace buffer. 
   Obviously, we could come up with our own constant for
   this, but,

 - An optimization like this is really the target's business
   to do, because only the target knows its trace buffer's
   layout, and what is the gap width where merging becomes
   beneficial.

 - Collecting memory mapped registers come to mind, where
   reading more than you've asked for is a bad idea.

 - Given the agent expression generator's status quo, you
   only get memory range actions on the target if you collect
   globals anyway.  Anything else, the agent expression
   generator outputs a trace agent expression.  E.g.,

(gdb) maint agent struct_b.ef
Scope: 0x400711
Reg mask: 00
  0  const32 6295936
  5  const8 4
  7  add
  8  const8 4
 10  trace
 11  end

(gdb) maint agent struct_b.d 
Scope: 0x400711
Reg mask: 00
  0  const32 6295936
  5  const8 4
  7  trace
  8  end

(gdb) maint agent tarray [1].b
Scope: 0x400711
Reg mask: 00
  0  const32 6295840
  5  const8 1
  7  const8 8
  9  mul
 10  add
 11  zero_ext 64
 13  const8 4
 15  add
 16  const8 4
 18  trace
 19  end

These encode as an 'X' action.  Notice how we always output
base + arithmetic, even if the offsets are constants.  With this,
it's doubtful that the "optimization" has ever payed off.

 - It may be that this was added as a means of avoiding confusion
   between 0 (not collected), and 0 (really 0), when collecting
   only a few fields of a structure or array, but per the point
   above, that can't really have ever worked.

For these reasons, I'm removing the heuristic from GDB.

-- 
Pedro Alves

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

	gdb/testuite/
	* gdb.trace/unavailable.cc (a, b, c): New globals.
	(main): Set and clear them.
	* gdb.trace/unavailable.exp (gdb_collect_globals_test): Collect
	`a' and `c', and check that `b' isn't collected, although `a' and
	`c' are.

	gdb/
	* tracepoint.c (memrange_sortmerge): Don't merge ranges that are
	almost but not quite adjacent.

---
 gdb/testsuite/gdb.trace/unavailable.cc  |   22 ++++++++++++++++++++++
 gdb/testsuite/gdb.trace/unavailable.exp |   12 ++++++++++++
 gdb/tracepoint.c                        |   11 +++++------
 3 files changed, 39 insertions(+), 6 deletions(-)

Index: src/gdb/testsuite/gdb.trace/unavailable.cc
===================================================================
--- src.orig/gdb/testsuite/gdb.trace/unavailable.cc	2011-02-07 13:24:23.726706003 +0000
+++ src/gdb/testsuite/gdb.trace/unavailable.cc	2011-02-07 13:27:53.276706002 +0000
@@ -71,6 +71,25 @@ struct tuple
 
 struct tuple tarray[8];
 
+/* Test for overcollection.  GDB used to merge memory ranges to
+   collect if they were close enough --- say, collect `a' and 'c'
+   below, and you'd get 'b' as well.  This had been presumably done to
+   cater for some target's inefficient trace buffer layout, but it is
+   really not GDB's business to assume how the target manages its
+   buffer.  If the target wants to overcollect, that's okay, since it
+   knows what is and what isn't safe to touch (think memory mapped
+   registers), and knows it's buffer layout.
+
+   The test assumes these three variables are laid out consecutively
+   in memory.  Unfortunately, we can't use an array instead, since the
+   agent expression generator does not even do constant folding,
+   meaning that anything that's more complicated than collecting a
+   global will generate an agent expression action to evaluate on the
+   target, instead of a simple "collect memory" action.  */
+int a;
+int b;
+int c;
+
 /* Random tests.  */
 
 struct StructA
@@ -185,6 +204,7 @@ main (int argc, char **argv, char **envp
   memcpy (g_string_unavail, g_const_string, sizeof (g_const_string));
   memcpy (g_string_partial, g_const_string, sizeof (g_const_string));
   g_string_p = g_const_string;
+  a = 1; b = 2; c = 3;
 
   /* Call test functions, so they can be traced and data collected.  */
   i = 0;
@@ -212,6 +232,8 @@ main (int argc, char **argv, char **envp
   memset (g_string_partial, 0, sizeof (g_string_partial));
   g_string_p = NULL;
 
+  a = b = c = 0;
+
   g_int = 0;
 
   g_structref.clear ();
Index: src/gdb/testsuite/gdb.trace/unavailable.exp
===================================================================
--- src.orig/gdb/testsuite/gdb.trace/unavailable.exp	2011-02-07 13:24:23.726706003 +0000
+++ src/gdb/testsuite/gdb.trace/unavailable.exp	2011-02-07 13:27:53.276706002 +0000
@@ -92,6 +92,9 @@ proc gdb_collect_globals_test { } {
 	"collect struct_b.struct_a.array\[2\]" "^$" \
 	"collect struct_b.struct_a.array\[100\]" "^$" \
 	\
+	"collect a" "^$" \
+	"collect c" "^$" \
+	\
 	"collect tarray\[0\].a" "^$" \
 	"collect tarray\[1\].a" "^$" \
 	"collect tarray\[3\].a" "^$" \
@@ -145,6 +148,15 @@ proc gdb_collect_globals_test { } {
 
     gdb_test "print /x struct_b.struct_a.array\[2\]" " = 0xaaaaaaaa"
 
+    # Check the target doesn't overcollect.  GDB used to merge memory
+    # ranges to collect if they were close enough (collecting the hole
+    # as well), but does not do that anymore.  It's plausible that a
+    # target may do this on its end, but as of this writing, no known
+    # target does it.
+    gdb_test "print {a, b, c}" \
+	" = \\{1, <unavailable>, 3\\}" \
+	"No overcollect of almost but not quite adjacent memory ranges"
+
     # Check <unavailable> isn't confused with 0 in array element repetitions
 
     gdb_test_no_output "set print repeat 1"
Index: src/gdb/tracepoint.c
===================================================================
--- 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 */


^ permalink raw reply	[flat|nested] 13+ messages in thread
end of thread, other threads:[~2011-02-17 16:20 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-02-07 14:35 [unavailable values part 1, 16/17] don't merge almost but not quite adjacent memory ranges to collect Pedro Alves
2011-02-14 12:01 ` Jan Kratochvil
2011-02-16 13:10   ` Pedro Alves
2011-02-16 18:00     ` Tom Tromey
2011-02-16 18:03       ` Pedro Alves
2011-02-16 18:07         ` Tom Tromey
2011-02-16 18:09           ` Pedro Alves
2011-02-16 18:09       ` Tom Tromey
2011-02-16 21:01         ` FYI: handle DW_OP_call_frame_cfa in AX compiler (Was: [unavailable values part 1, 16/17] don't merge almost but not quite adjacent memory ranges to collect) Tom Tromey
2011-02-16 21:31           ` FYI: handle DW_OP_call_frame_cfa in AX compiler Pedro Alves
2011-02-16 21:40             ` Tom Tromey
2011-02-16 23:27               ` Pedro Alves
2011-02-17 16:24                 ` Tom Tromey

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox