* [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