Re: [PATCH 1/2] Fast tracepoint for powerpc64le

[Wei-cheng Wang <cole945 at gmail 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,