This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/2] Fast tracepoint for powerpc64le
- From: Wei-cheng Wang <cole945 at gmail dot com>
- To: Ulrich Weigand <uweigand at de dot ibm dot com>, palves at redhat dot com
- Cc: gdb-patches at sourceware dot org
- Date: Mon, 02 Mar 2015 01:42:03 +0800
- Subject: Re: [PATCH 1/2] Fast tracepoint for powerpc64le
- Authentication-results: sourceware.org; auth=none
- References: <201502271952 dot t1RJqq7X018591 at d03av02 dot boulder dot ibm dot com>
On 2015/2/28 äå 03:52, Ulrich Weigand wrote:
The tspeed.exp file already has:
# Typically we need a little extra time for this test.
set timeout 180
Is that still not enough?
It should include the time spent in trying different loop counts,
so it would be 11 + 22 + 45 + 90 + 180 = at least 348 seconds in my environment.
(for 10000, 20000, 40000, 80000, 160000 iterations respectively)
If I set timeout to 360, the case will pass.
* tfind.exp: One of the tracepoint is inserted at
`*gdb_recursion_test'. It's not hit because local-entry is called
instead. The 18 FAILs are off-by-one error.
This test case seem a bit more complicated, we may need to split it
in two parts; one that uses a normal "trace gdb_recursion_test"
without the "*", and possibly a second one that specifically tests
that "trace *func" works, using a source file that makes sure to
call func via a function pointers (as in step-bt.c).
How about simply change the code to this? It wouldn't hurt other cases.
And all the failed cases in tfind.exp now pass.
--- a/gdb/testsuite/gdb.trace/actions.c
+++ b/gdb/testsuite/gdb.trace/actions.c
@@ -46,6 +46,8 @@ static union GDB_UNION_TEST
} gdb_union1_test;
void gdb_recursion_test (int, int, int, int, int, int, int);
+typedef void (*gdb_recursion_test_fp) (int, int, int, int, int, int, int);
+gdb_recursion_test_fp gdb_recursion_test_ptr = gdb_recursion_test;
void gdb_recursion_test (int depth,
int q1,
@@ -64,7 +66,7 @@ void gdb_recursion_test (int depth,
q5 = q6; /* gdbtestline 6 */
q6 = q; /* gdbtestline 7 */
if (depth--) /* gdbtestline 8 */
- gdb_recursion_test (depth, q1, q2, q3, q4, q5, q6); /* gdbtestline 9 */
+ gdb_recursion_test_ptr (depth, q1, q2, q3, q4, q5, q6); /* gdbtestline 9 */
}
@@ -103,7 +105,7 @@ unsigned long gdb_c_test( unsigned long *parm )
gdb_structp_test = &gdb_struct1_test;
gdb_structpp_test = &gdb_structp_test;
- gdb_recursion_test (3, (long) parm[1], (long) parm[2], (long) parm[3],
+ gdb_recursion_test_ptr (3, (long) parm[1], (long) parm[2], (long) parm[3],
(long) parm[4], (long) parm[5], (long) parm[6]);
For powerpc32 to work, some data structure/function in tracepoint.c
need to be fixed. For example,
* write_inferior_data_ptr should be fixed for big-endian.
If sizeof (CORE_ADDR) is larger than sizeof (void*), zeros are written.
BTW, I thnink write_inferior_data_pointer provides the same functionality
without this issue. I'm not sure why write_inferior_data_ptr is needed?
This is odd, I don't see the point of this either. Of course, as the
comment says, much of this stuff will break anyway if gdbserver is
compiled differently than the inferior (e.g. a 64-bit gdbserver
debugging a 32-bit inferior), because it assumes the structure layout
is identical. However, if we do have a 32-bit gdbserver, then I don't
see why it shouldn't be possible to debug a 32-bit inferior, just
because CORE_ADDR is a 64-bit type ...
For example, CORE_ADDR ptr = 0x11223344, a 32-bit address,
and sizeof (void *) = 4-byte
|------------ 64-bit CORE_ADDR ---------|
MSB LSB
| 00 | 00 | 00 | 00 | 11 | 22 | 33 | 44 |
Low High Address
|-- 32-bit(void*) --|
&ptr,4 means these zeros are written to inferior.
static int
write_inferior_data_ptr (CORE_ADDR where, CORE_ADDR ptr)
{
return write_inferior_memory (where,
(unsigned char *) &ptr, sizeof (void *));
^^^^ ^^^^^^^^^^^^^^^
}
CORE_ADDR is declared as "unsigned long long" for gdbserver
(in common/gdb/common-types.h)
Ugh. That's a strange construct, and extremely dependent on alignment
rules (as you noticed). I'm not really sure what the best way to fix
this would be. My preference right now would be get rid of "ops" on
the gdbserver side too, and just switch on "type" in the two places
where the ops->send and ops->download routines are called right now.
This makes the data structures the same on gdbserver and IPA, which
simplifies downloading quite a bit. (Also, it keeps the data structure
identical in IPA, which should avoid compatibility issues between
versions.)
That sounds great to me!
Thanks
Wei-cheng,