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

["Ulrich Weigand" <uweigand@de.ibm.com>]

Wei-cheng Wang wrote:
> 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.

I guess that's OK with me.  Or else we could reduce the number of passes ...

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

That should be OK.

> > 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)

I understood why this is failing with the code as is, I just didn't
understand why the code is that way today :-)   Given Pedro's comment,
I think we should simply remove that function.

> > 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!

OK, let's do it that way.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com