From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25114 invoked by alias); 7 Feb 2011 14:35:37 -0000 Received: (qmail 24994 invoked by uid 22791); 7 Feb 2011 14:35:35 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,TW_CP,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 07 Feb 2011 14:35:26 +0000 Received: (qmail 27676 invoked from network); 7 Feb 2011 14:35:24 -0000 Received: from unknown (HELO scottsdale.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 7 Feb 2011 14:35:24 -0000 To: gdb-patches@sourceware.org Subject: [unavailable values part 1, 16/17] don't merge almost but not quite adjacent memory ranges to collect From: Pedro Alves Date: Mon, 07 Feb 2011 14:35:00 -0000 MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201102071435.20804.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-02/txt/msg00147.txt.bz2 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 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, , 3\\}" \ + "No overcollect of almost but not quite adjacent memory ranges" + # Check 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 */