* [PATCH] gdb.trace: Fix string collection for 64-bit platforms. @ 2016-01-21 16:02 Marcin Kościelnicki 2016-01-21 16:14 ` Pedro Alves 0 siblings, 1 reply; 6+ messages in thread From: Marcin Kościelnicki @ 2016-01-21 16:02 UTC (permalink / raw) To: gdb-patches; +Cc: Marcin Kościelnicki String collection always used ref32 to fetch the string pointer. Make it use ref64 when pointeer size is 64-bit. This appeared to work on x86_64 since it's a little-endian platform, and malloc (used in gdb.trace/collection.exp) returns addresses in low 4GB. Noticed and tested on s390x-ibm-linux-gnu, also tested on i686-unknown-linux-gnu and x86_64-unknown-linux-gnu. gdb/ChangeLog: * ax-gdb.c (gen_traced_pop): Use aop_ref64 when appropriate for string collection. --- OK to push? gdb/ChangeLog | 5 +++++ gdb/ax-gdb.c | 5 ++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 45d8ef9..3f4d152 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,8 @@ +2016-01-21 Marcin KoÅ›cielnicki <koriakin@0x04.net> + + * ax-gdb.c (gen_traced_pop): Use aop_ref64 when appropriate for + string collection. + 2016-01-21 Andrew Burgess <andrew.burgess@embecosm.com> * disasm.c (maybe_add_dis_line_entry): Rename to... diff --git a/gdb/ax-gdb.c b/gdb/ax-gdb.c index dd6eee6..d17cb04 100644 --- a/gdb/ax-gdb.c +++ b/gdb/ax-gdb.c @@ -410,7 +410,10 @@ gen_traced_pop (struct gdbarch *gdbarch, if (string_trace) { - ax_simple (ax, aop_ref32); + if (TYPE_LENGTH(value->type) == 4) + ax_simple (ax, aop_ref32); + else + ax_simple (ax, aop_ref64); ax_const_l (ax, ax->trace_string); ax_simple (ax, aop_tracenz); } -- 2.7.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdb.trace: Fix string collection for 64-bit platforms. 2016-01-21 16:02 [PATCH] gdb.trace: Fix string collection for 64-bit platforms Marcin Kościelnicki @ 2016-01-21 16:14 ` Pedro Alves 2016-01-21 16:18 ` Marcin Kościelnicki 2016-01-21 16:43 ` [PATCH v2] " Marcin Kościelnicki 0 siblings, 2 replies; 6+ messages in thread From: Pedro Alves @ 2016-01-21 16:14 UTC (permalink / raw) To: Marcin Kościelnicki, gdb-patches On 01/21/2016 04:02 PM, Marcin KoÅ›cielnicki wrote: > String collection always used ref32 to fetch the string pointer. Make it > use ref64 when pointeer size is 64-bit. "pointer". > > This appeared to work on x86_64 since it's a little-endian platform, and > malloc (used in gdb.trace/collection.exp) returns addresses in low 4GB. > Noticed and tested on s390x-ibm-linux-gnu, also tested on > i686-unknown-linux-gnu and x86_64-unknown-linux-gnu. > > gdb/ChangeLog: > > * ax-gdb.c (gen_traced_pop): Use aop_ref64 when appropriate for > string collection. > --- > OK to push? > > gdb/ChangeLog | 5 +++++ > gdb/ax-gdb.c | 5 ++++- > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/gdb/ChangeLog b/gdb/ChangeLog > index 45d8ef9..3f4d152 100644 > --- a/gdb/ChangeLog > +++ b/gdb/ChangeLog > @@ -1,3 +1,8 @@ > +2016-01-21 Marcin KoÅ›cielnicki <koriakin@0x04.net> > + > + * ax-gdb.c (gen_traced_pop): Use aop_ref64 when appropriate for > + string collection. > + > 2016-01-21 Andrew Burgess <andrew.burgess@embecosm.com> > > * disasm.c (maybe_add_dis_line_entry): Rename to... > diff --git a/gdb/ax-gdb.c b/gdb/ax-gdb.c > index dd6eee6..d17cb04 100644 > --- a/gdb/ax-gdb.c > +++ b/gdb/ax-gdb.c > @@ -410,7 +410,10 @@ gen_traced_pop (struct gdbarch *gdbarch, > > if (string_trace) > { > - ax_simple (ax, aop_ref32); > + if (TYPE_LENGTH(value->type) == 4) Missing space before parens. > + ax_simple (ax, aop_ref32); > + else > + ax_simple (ax, aop_ref64); > + if (TYPE_LENGTH(value->type) == 4) > + ax_simple (ax, aop_ref32); > + else > + ax_simple (ax, aop_ref64); Missing space before parens. I was going to say to check for == 8 instead so that we don't pessimize the 8/16 bits cases, but sounds better even to factor out the switch in gen_fetch, so that the ref8/ref16 are covered as well: switch (TYPE_LENGTH (type)) { case 8 / TARGET_CHAR_BIT: ax_simple (ax, aop_ref8); break; case 16 / TARGET_CHAR_BIT: ax_simple (ax, aop_ref16); break; case 32 / TARGET_CHAR_BIT: ax_simple (ax, aop_ref32); break; case 64 / TARGET_CHAR_BIT: ax_simple (ax, aop_ref64); break; /* Either our caller shouldn't have asked us to dereference that pointer (other code's fault), or we're not implementing something we should be (this code's fault). In any case, it's a bug the user shouldn't see. */ default: internal_error (__FILE__, __LINE__, WDYT? Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdb.trace: Fix string collection for 64-bit platforms. 2016-01-21 16:14 ` Pedro Alves @ 2016-01-21 16:18 ` Marcin Kościelnicki 2016-01-21 16:43 ` [PATCH v2] " Marcin Kościelnicki 1 sibling, 0 replies; 6+ messages in thread From: Marcin Kościelnicki @ 2016-01-21 16:18 UTC (permalink / raw) To: Pedro Alves, gdb-patches On 21/01/16 17:14, Pedro Alves wrote: > On 01/21/2016 04:02 PM, Marcin KoÅ›cielnicki wrote: >> String collection always used ref32 to fetch the string pointer. Make it >> use ref64 when pointeer size is 64-bit. > > "pointer". > >> >> This appeared to work on x86_64 since it's a little-endian platform, and >> malloc (used in gdb.trace/collection.exp) returns addresses in low 4GB. >> Noticed and tested on s390x-ibm-linux-gnu, also tested on >> i686-unknown-linux-gnu and x86_64-unknown-linux-gnu. >> >> gdb/ChangeLog: >> >> * ax-gdb.c (gen_traced_pop): Use aop_ref64 when appropriate for >> string collection. >> --- >> OK to push? >> >> gdb/ChangeLog | 5 +++++ >> gdb/ax-gdb.c | 5 ++++- >> 2 files changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/gdb/ChangeLog b/gdb/ChangeLog >> index 45d8ef9..3f4d152 100644 >> --- a/gdb/ChangeLog >> +++ b/gdb/ChangeLog >> @@ -1,3 +1,8 @@ >> +2016-01-21 Marcin KoÅ›cielnicki <koriakin@0x04.net> >> + >> + * ax-gdb.c (gen_traced_pop): Use aop_ref64 when appropriate for >> + string collection. >> + >> 2016-01-21 Andrew Burgess <andrew.burgess@embecosm.com> >> >> * disasm.c (maybe_add_dis_line_entry): Rename to... >> diff --git a/gdb/ax-gdb.c b/gdb/ax-gdb.c >> index dd6eee6..d17cb04 100644 >> --- a/gdb/ax-gdb.c >> +++ b/gdb/ax-gdb.c >> @@ -410,7 +410,10 @@ gen_traced_pop (struct gdbarch *gdbarch, >> >> if (string_trace) >> { >> - ax_simple (ax, aop_ref32); >> + if (TYPE_LENGTH(value->type) == 4) > > Missing space before parens. > >> + ax_simple (ax, aop_ref32); >> + else >> + ax_simple (ax, aop_ref64); > >> + if (TYPE_LENGTH(value->type) == 4) >> + ax_simple (ax, aop_ref32); >> + else >> + ax_simple (ax, aop_ref64); > > > Missing space before parens. I was going to say to > check for == 8 instead so that we don't pessimize the > 8/16 bits cases, but sounds better even to factor out > the switch in gen_fetch, so that the ref8/ref16 are covered > as well: > > switch (TYPE_LENGTH (type)) > { > case 8 / TARGET_CHAR_BIT: > ax_simple (ax, aop_ref8); > break; > case 16 / TARGET_CHAR_BIT: > ax_simple (ax, aop_ref16); > break; > case 32 / TARGET_CHAR_BIT: > ax_simple (ax, aop_ref32); > break; > case 64 / TARGET_CHAR_BIT: > ax_simple (ax, aop_ref64); > break; > > /* Either our caller shouldn't have asked us to dereference > that pointer (other code's fault), or we're not > implementing something we should be (this code's fault). > In any case, it's a bug the user shouldn't see. */ > default: > internal_error (__FILE__, __LINE__, > > WDYT? > > Thanks, > Pedro Alves > Fair enough, ref16 may actually be useful on some tiny platforms, and it certainly won't hurt to throw in ref8 as well. I'll do that. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] gdb.trace: Fix string collection for 64-bit platforms. 2016-01-21 16:14 ` Pedro Alves 2016-01-21 16:18 ` Marcin Kościelnicki @ 2016-01-21 16:43 ` Marcin Kościelnicki 2016-01-21 16:55 ` Pedro Alves 1 sibling, 1 reply; 6+ messages in thread From: Marcin Kościelnicki @ 2016-01-21 16:43 UTC (permalink / raw) To: palves; +Cc: gdb-patches, Marcin Kościelnicki String collection always used ref32 to fetch the string pointer. Make it use gen_fetch instead. As a side effect, this patch changes dup+const+trace+pop sequence used for collecting the string's address to a trace_quick opcode. This results in a shorter agent expression. This appeared to work on x86_64 since it's a little-endian platform, and malloc (used in gdb.trace/collection.exp) returns addresses in low 4GB. Noticed and tested on s390x-ibm-linux-gnu, also tested on i686-unknown-linux-gnu and x86_64-unknown-linux-gnu. gdb/ChangeLog: * ax-gdb.c (gen_traced_pop): Use gen_fetch for string collection. --- Instead of factoring out the switch, I just delegated to gen_fetch. Turns out we can shave off a few ops this way, too. Likewise tested on s390, s390x, i686, x86_64. gdb/ChangeLog | 4 ++++ gdb/ax-gdb.c | 23 +++++++++++------------ 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 45d8ef9..28173b0 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,7 @@ +2016-01-21 Marcin KoÅ›cielnicki <koriakin@0x04.net> + + * ax-gdb.c (gen_traced_pop): Use gen_fetch for string collection. + 2016-01-21 Andrew Burgess <andrew.burgess@embecosm.com> * disasm.c (maybe_add_dis_line_entry): Rename to... diff --git a/gdb/ax-gdb.c b/gdb/ax-gdb.c index dd6eee6..7c6cb64 100644 --- a/gdb/ax-gdb.c +++ b/gdb/ax-gdb.c @@ -394,26 +394,25 @@ gen_traced_pop (struct gdbarch *gdbarch, case axs_lvalue_memory: { - if (string_trace) - ax_simple (ax, aop_dup); - /* Initialize the TYPE_LENGTH if it is a typedef. */ check_typedef (value->type); - /* There's no point in trying to use a trace_quick bytecode - here, since "trace_quick SIZE pop" is three bytes, whereas - "const8 SIZE trace" is also three bytes, does the same - thing, and the simplest code which generates that will also - work correctly for objects with large sizes. */ - ax_const_l (ax, TYPE_LENGTH (value->type)); - ax_simple (ax, aop_trace); - if (string_trace) { - ax_simple (ax, aop_ref32); + gen_fetch (ax, value->type); ax_const_l (ax, ax->trace_string); ax_simple (ax, aop_tracenz); } + else + { + /* There's no point in trying to use a trace_quick bytecode + here, since "trace_quick SIZE pop" is three bytes, whereas + "const8 SIZE trace" is also three bytes, does the same + thing, and the simplest code which generates that will also + work correctly for objects with large sizes. */ + ax_const_l (ax, TYPE_LENGTH (value->type)); + ax_simple (ax, aop_trace); + } } break; -- 2.7.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] gdb.trace: Fix string collection for 64-bit platforms. 2016-01-21 16:43 ` [PATCH v2] " Marcin Kościelnicki @ 2016-01-21 16:55 ` Pedro Alves 2016-01-21 18:51 ` Marcin Kościelnicki 0 siblings, 1 reply; 6+ messages in thread From: Pedro Alves @ 2016-01-21 16:55 UTC (permalink / raw) To: Marcin Kościelnicki; +Cc: gdb-patches On 01/21/2016 04:43 PM, Marcin KoÅ›cielnicki wrote: > String collection always used ref32 to fetch the string pointer. Make it > use gen_fetch instead. > > As a side effect, this patch changes dup+const+trace+pop sequence used > for collecting the string's address to a trace_quick opcode. This > results in a shorter agent expression. > > This appeared to work on x86_64 since it's a little-endian platform, and > malloc (used in gdb.trace/collection.exp) returns addresses in low 4GB. > Noticed and tested on s390x-ibm-linux-gnu, also tested on > i686-unknown-linux-gnu and x86_64-unknown-linux-gnu. > > gdb/ChangeLog: > > * ax-gdb.c (gen_traced_pop): Use gen_fetch for string collection. > --- > Instead of factoring out the switch, I just delegated to gen_fetch. > Turns out we can shave off a few ops this way, too. Likewise tested > on s390, s390x, i686, x86_64. Even better. This is OK. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] gdb.trace: Fix string collection for 64-bit platforms. 2016-01-21 16:55 ` Pedro Alves @ 2016-01-21 18:51 ` Marcin Kościelnicki 0 siblings, 0 replies; 6+ messages in thread From: Marcin Kościelnicki @ 2016-01-21 18:51 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches On 21/01/16 17:55, Pedro Alves wrote: > On 01/21/2016 04:43 PM, Marcin KoÅ›cielnicki wrote: >> String collection always used ref32 to fetch the string pointer. Make it >> use gen_fetch instead. >> >> As a side effect, this patch changes dup+const+trace+pop sequence used >> for collecting the string's address to a trace_quick opcode. This >> results in a shorter agent expression. >> >> This appeared to work on x86_64 since it's a little-endian platform, and >> malloc (used in gdb.trace/collection.exp) returns addresses in low 4GB. >> Noticed and tested on s390x-ibm-linux-gnu, also tested on >> i686-unknown-linux-gnu and x86_64-unknown-linux-gnu. >> >> gdb/ChangeLog: >> >> * ax-gdb.c (gen_traced_pop): Use gen_fetch for string collection. >> --- >> Instead of factoring out the switch, I just delegated to gen_fetch. >> Turns out we can shave off a few ops this way, too. Likewise tested >> on s390, s390x, i686, x86_64. > > Even better. This is OK. > > Thanks, > Pedro Alves > Thanks, pushed. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-01-21 18:51 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-01-21 16:02 [PATCH] gdb.trace: Fix string collection for 64-bit platforms Marcin Kościelnicki 2016-01-21 16:14 ` Pedro Alves 2016-01-21 16:18 ` Marcin Kościelnicki 2016-01-21 16:43 ` [PATCH v2] " Marcin Kościelnicki 2016-01-21 16:55 ` Pedro Alves 2016-01-21 18:51 ` Marcin Kościelnicki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox