Re: [patch] circ.exp

[Pedro Alves <palves@redhat.com>]

On 04/16/2013 07:03 PM, Abid, Hafiz wrote:

> Thanks for the review and explnation about the circular behavior.
> 
>> It'd have been useful if you had pointed out what were the issues.
>> You must have come to the conclusion to you were going to refactor
>> the code _after_ you've analyzed them.
> Here are the problems that I observed.

Thanks, that's very useful.

> 3) When buffer is small and we can not have all 10 frames in it, it was testing
> for "tfind 9" which tries to find the 10th frame which is never there due to small
> buffer size.  
> I may be wrong but I think it should rather test for "tfind tracepoint
> 9" which will give it a frame related to tracepoint 9.

How interesting.  The trace frame number is a detail of the
target, actually.  There's nothing in GDB (IIRC) that expects
the first traceframe to be traceframe number 1.  When you do
"tfind 4", that asks the target to select traceframe 4 -- the 4
is sent to the target.  It must have been that the original target that
supported tracepoints, the one this test was written for originally years
and years ago recorded the trace frame number in the traceframe itself.
But that's not how GDBserver works.  GDBserver counts traceframes from the
first recorded traceframe instead:

/* Given a traceframe number NUM, find the NUMth traceframe in the
   buffer.  */

static struct traceframe *
find_traceframe (int num)
{
  struct traceframe *tframe;
  int tfnum = 0;

  for (tframe = FIRST_TRACEFRAME ();
       tframe->tpnum != 0;
       tframe = NEXT_TRACEFRAME (tframe))
    {
      if (tfnum == num)
	return tframe;
      ++tfnum;
    }


We should update the comments and test messages too then:

#   1) frame zero will be overwritten and therefore unavailable
#   2) the earliest frame in the buffer will be other-than-zero
#   3) frame nine will be available (unlike "small buffer" case).

s/other-than-zero/for a tracepoint other than 1/
s/frame nine/the frame for tracepoint 9/

    gdb_test \
	"tfind tracepoint 9" ".*Found trace frame $decimal, tracepoint 9.*" \
	"find frame nine"

s/frame /frame for tracepoint/

etc.

Then might as well make all similar tfinds in the non-circular cases
use "tfind tracepoint" too for consistency.



> 4) Many tests were expecting current stack frame to be printed as result of
> tfind command e.g. "#0  func9 .*".  But this only happens when traceframe_number
> is -1.  In the following code, old_frame_id equal to current frame id unless
> traceframe_number < 0.
>

I'm confused.  Does the tfind in question switch to a different
frame or not?

>   if (!(type == tfind_number && num == -1)
>       && (has_stack_frames () || traceframe_number >= 0))
>     old_frame_id = get_frame_id (get_current_frame ());
> ...
>   if (frame_id_eq (old_frame_id,
>                get_frame_id (get_current_frame ())))
>     print_what = SRC_LINE;
>       else
>     print_what = SRC_AND_LOC;


>> As a follow up, it'd be nice to use gdb features/commands instead
>> of manually sending packets to probe support.  E.g, use tstatus to
>> check for support:
> I thought we meant to test both through commands and packets. But if you
> feel that we should only use commands then I can do that in next version.

I feel that we should only use commands.  There's no good reason
to limit the test to the remote protocol.  As soon as someone implements
tracepoints on some other target (native or something else), we'd
need to stop using direct RSP packets anyway.  The test used a direct
packet because there was no user-accessible support for changing the
trace buffer size.  It was just a hack.

Thanks.  This is looking great.

BTW, I realized this is the existing style in the test, but
these "if (...) then return 1" sprinkled around are really
unnecessary.

+    if [setup_tracepoints] then { return 1 }
+    if [gdb_test "break end" "Breakpoint $decimal.*" \
+	    "breakpoint at end"] then { return 1 }
+    if [gdb_test "tstart" \
+	    "\[\r\n\]*" \
+	    "start trace experiment"] then { return 1 }

I'd zap them.

-- 
Pedro Alves