[PATCH] Fix agent code generate bug of ref

[PATCH] Fix agent code generate bug of ref

[Hui Zhu]

[-- Attachment #1: Type: text/plain, Size: 3258 bytes --]

Hi,

I meet a issue with tracepoint and Linux kernel is:
(gdb) list update_curr
590		cfs_rq->load_unacc_exec_time += delta_exec;
591	#endif
592	}
593	
594	static void update_curr(struct cfs_rq *cfs_rq)
595	{
596		struct sched_entity *curr = cfs_rq->curr;
597		u64 now = rq_of(cfs_rq)->clock;
598		unsigned long delta_exec;
599	
(gdb)
600		if (unlikely(!curr))
601			return;
602	
603		/*
604		 * Get the amount of time the current task was running
605		 * since the last time we changed load (this cannot
606		 * overflow on 32 bits):
607		 */
608		delta_exec = (unsigned long)(now - curr->exec_start);
609		if (!delta_exec)
(gdb) trace 609
Tracepoint 1 at 0xffffffff8104ced6: file
/home/teawater/kernel/taobao-kernel/tmp/linux-2.6.32-220.23.1.el5/kernel/sched_fair.c,
line 609.
(gdb) actions
Enter actions for tracepoint 1, one per line.
End with a line saying just "end".
>collect now
>end
(gdb) tstart

Then it will failed in kernel part, the address that send to kernel to
collect is 0x978.  But the right address of now is 0xffff880002215578.

I check the agent code that is got is:
(gdb) maintenance agent -at sched_fair.c:609, now
Scope: 0xffffffff8104ced6
Reg mask: 20
  0  reg 5
  3  const16 128
  6  add
  7  ref8
  8  const16 2232
 11  add
 12  const8 8
 14  trace
 15  end

I think ref8 is not right because this acode try to get a address from
Linux kernel.  It should be ref64.
I check the code of function dwarf2_compile_expr_to_ax:
	case DW_OP_deref:
	case DW_OP_deref_size:
	  {
	    int size;

	    if (op == DW_OP_deref_size)
	      size = *op_ptr++;
	    else
	      size = addr_size;

	    switch (size)
	      {
	      case 8:
		ax_simple (expr, aop_ref8);
aop_ref8 means ref 8 bits.  So use addr_size is not right, I add first
patch fix-op_deref-size.txt to change addr_size to addr_size_bits.
Then the first issue is fixed.

And I found that GDB generate right code the collect value from
0xffff880002215578.

But after that, I still got error when I tfind:
(gdb) tfind
Found trace frame 0, tracepoint 1
#0  update_curr (cfs_rq=0xffff880002214d28, cfs_rq@entry=<error
reading variable: PC not available>)
    at /home/teawater/kernel/taobao-kernel/tmp/linux-2.6.32-220.23.1.el5/kernel/sched_fair.c:609
609		if (!delta_exec)
(gdb) p now
Cannot access memory at address 0xffff880002214da8

This issue is because aop just collect value of now, but not ref
address.  But GDB need this value.
So I add second patch trace_def_if_trace.txt to call ax_trace_quick if need.

Then the aop will be changed to:
(gdb) maintenance agent -at sched_fair.c:609, now
Scope: 0xffffffff8104ced6
Reg mask: 20
  0  reg 5
  3  const16 128
  6  add
  7  trace_quick 8
  9  ref64
 10  const16 2232
 13  add
 14  const8 8
 16  trace
 17  end

Then all the issue of this tracepoint is fixed.  But I failed with
make a test code with it.  So I just can repeoduce in this Linux
kernel code.

I suggest the next release just this 2 patches because this 2 issue
will affect some code of aop.

Thanks,
Hui



2013-03-10  Hui Zhu  <hui_zhu@mentor.com>

	* dwarf2loc.c (dwarf2_compile_expr_to_ax): Change addr_size to
	addr_size_bits if DW_OP_deref.


2013-03-10  Hui Zhu  <hui_zhu@mentor.com>

	* dwarf2loc.c (dwarf2_compile_expr_to_ax): Call ax_trace_quick
	if need.

[-- Attachment #2: fix-op_deref-size.txt --]
[-- Type: text/plain, Size: 259 bytes --]

--- a/dwarf2loc.c
+++ b/dwarf2loc.c
@@ -2929,7 +2929,7 @@ dwarf2_compile_expr_to_ax (struct agent_
 	    if (op == DW_OP_deref_size)
 	      size = *op_ptr++;
 	    else
-	      size = addr_size;
+	      size = addr_size_bits;
 
 	    switch (size)
 	      {

[-- Attachment #3: trace_def_if_trace.txt --]
[-- Type: text/plain, Size: 256 bytes --]

--- a/dwarf2loc.c
+++ b/dwarf2loc.c
@@ -2931,6 +2931,9 @@ dwarf2_compile_expr_to_ax (struct agent_
 	    else
 	      size = addr_size_bits;
 
+	    if (trace_kludge)
+	      ax_trace_quick (expr, size / 8);
+
 	    switch (size)
 	      {
 	      case 8:

Re: [PATCH] Fix agent code generate bug of ref

[Yao Qi]

On 03/10/2013 01:00 PM, Hui Zhu wrote:
> @@ -2929,7 +2929,7 @@ dwarf2_compile_expr_to_ax (struct agent_
>   	    if (op == DW_OP_deref_size)
>   	      size = *op_ptr++;
>   	    else
> -	      size = addr_size;
> +	      size = addr_size_bits;

The problem here is SIZE is "size in bytes", but
>
>   	    switch (size)
>   	      {

SIZE is checked as "size in bits".  Your fix is right, but not complete. 
  We also have to fix it when op is DW_OP_deref_size, something like the 
patch below.

I am not familiar with this area, so I might be wrong.

-- 
Yao (齐尧)

@@ -2931,7 +2931,7 @@ dwarf2_compile_expr_to_ax (struct agent_expr 
*expr, struct axs_value *loc,
  	    else
  	      size = addr_size;

-	    switch (size)
+	    switch (size * TARGET_CHAR_BIT)
  	      {
  	      case 8:
  		ax_simple (expr, aop_ref8);

Re: [PATCH] Fix agent code generate bug of ref

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> SIZE is checked as "size in bits".  Your fix is right, but not
Yao> complete. We also have to fix it when op is DW_OP_deref_size,
Yao> something like the patch below.

Yao> I am not familiar with this area, so I might be wrong.

You are correct, but IMO it is simpler to divide each of the case
constants by 8.

Tom

Re: [PATCH] Fix agent code generate bug of ref

[Tom Tromey]

Tom> You are correct, but IMO it is simpler to divide each of the case
Tom> constants by 8.

... or perhaps use the existing utility function
dwarf2loc.c:access_memory, which also handles trace_kludge.

Tom

Re: [PATCH] Fix agent code generate bug of ref

[Hui Zhu]

On Mon, Mar 11, 2013 at 10:56 PM, Tom Tromey <tromey@redhat.com> wrote:
> Tom> You are correct, but IMO it is simpler to divide each of the case
> Tom> constants by 8.
>
> ... or perhaps use the existing utility function
> dwarf2loc.c:access_memory, which also handles trace_kludge.
>
> Tom

Hi Tom,

Sorry for my misunderstand in your mail.  Accord to discussion with
Yao in IRC.  I merge 2 patches together.
And I found that gdb_assert of access_memory.  It should use nbytes.

Please help me review this patch.

Thanks,
Hui

2013-03-12  Yao Qi  <yao@codesourcery.com>
	    Hui Zhu  <hui_zhu@mentor.com>

	* dwarf2loc.c (access_memory): Change nbits to nbytes in gdb_assert.
	(dwarf2_compile_expr_to_ax): Call access_memory in DW_OP_deref and
	DW_OP_deref_size.

--- a/dwarf2loc.c
+++ b/dwarf2loc.c
@@ -2539,7 +2539,7 @@ access_memory (struct gdbarch *arch, str
 {
   ULONGEST nbytes = (nbits + 7) / 8;

-  gdb_assert (nbits > 0 && nbits <= sizeof (LONGEST));
+  gdb_assert (nbytes > 0 && nbytes <= sizeof (LONGEST));

   if (trace_kludge)
     ax_trace_quick (expr, nbytes);
@@ -2933,26 +2933,7 @@ dwarf2_compile_expr_to_ax (struct agent_
 	    else
 	      size = addr_size;

-	    switch (size)
-	      {
-	      case 8:
-		ax_simple (expr, aop_ref8);
-		break;
-	      case 16:
-		ax_simple (expr, aop_ref16);
-		break;
-	      case 32:
-		ax_simple (expr, aop_ref32);
-		break;
-	      case 64:
-		ax_simple (expr, aop_ref64);
-		break;
-	      default:
-		/* Note that get_DW_OP_name will never return
-		   NULL here.  */
-		error (_("Unsupported size %d in %s"),
-		       size, get_DW_OP_name (op));
-	      }
+	    access_memory (arch, expr, size * TARGET_CHAR_BIT);
 	  }
 	  break;

Re: [PATCH] Fix agent code generate bug of ref

[Tom Tromey]

>>>>> "Hui" == Hui Zhu <teawater@gmail.com> writes:

Hui> Sorry for my misunderstand in your mail.  Accord to discussion with
Hui> Yao in IRC.  I merge 2 patches together.
Hui> And I found that gdb_assert of access_memory.  It should use nbytes.

Thanks for doing this.

Hui> 2013-03-12  Yao Qi  <yao@codesourcery.com>
Hui> 	    Hui Zhu  <hui_zhu@mentor.com>
Hui> 	* dwarf2loc.c (access_memory): Change nbits to nbytes in gdb_assert.
Hui> 	(dwarf2_compile_expr_to_ax): Call access_memory in DW_OP_deref and
Hui> 	DW_OP_deref_size.

Hui> +  gdb_assert (nbytes > 0 && nbytes <= sizeof (LONGEST));

Hui> -	      default:
Hui> -		/* Note that get_DW_OP_name will never return
Hui> -		   NULL here.  */
Hui> -		error (_("Unsupported size %d in %s"),
Hui> -		       size, get_DW_OP_name (op));
Hui> -	      }

I think we need a sanity check before calling access_memory.
Otherwise, bad DWARF will be able to crash gdb.
The patch is ok with that change.

Tom

Re: [PATCH] Fix agent code generate bug of ref

[Hui Zhu]

On 03/12/13 22:16, Tom Tromey wrote:
>>>>>> "Hui" == Hui Zhu <teawater@gmail.com> writes:
>
> Hui> Sorry for my misunderstand in your mail.  Accord to discussion with
> Hui> Yao in IRC.  I merge 2 patches together.
> Hui> And I found that gdb_assert of access_memory.  It should use nbytes.
>
> Thanks for doing this.
>
> Hui> 2013-03-12  Yao Qi  <yao@codesourcery.com>
> Hui> 	    Hui Zhu  <hui_zhu@mentor.com>
> Hui> 	* dwarf2loc.c (access_memory): Change nbits to nbytes in gdb_assert.
> Hui> 	(dwarf2_compile_expr_to_ax): Call access_memory in DW_OP_deref and
> Hui> 	DW_OP_deref_size.
>
> Hui> +  gdb_assert (nbytes > 0 && nbytes <= sizeof (LONGEST));
>
> Hui> -	      default:
> Hui> -		/* Note that get_DW_OP_name will never return
> Hui> -		   NULL here.  */
> Hui> -		error (_("Unsupported size %d in %s"),
> Hui> -		       size, get_DW_OP_name (op));
> Hui> -	      }
>
> I think we need a sanity check before calling access_memory.
> Otherwise, bad DWARF will be able to crash gdb.
> The patch is ok with that change.
>
> Tom
>

According to the discussion with Tom in IRC.
I add a check before call access_memory.
+	    if (size != 1 && size != 2 && size != 4 && size != 8)
+	      error (_("Refn doesn't support size %d"),
+		     size * TARGET_CHAR_BIT);

Checked in http://sourceware.org/ml/gdb-cvs/2013-03/msg00102.html

Thanks,
Hui

Re: [PATCH] Fix agent code generate bug of ref

[Tom Tromey]

>>>>> "Hui" == Hui Zhu <hui_zhu@mentor.com> writes:

Hui> According to the discussion with Tom in IRC.
Hui> I add a check before call access_memory.
Hui> +	    if (size != 1 && size != 2 && size != 4 && size != 8)
Hui> +	      error (_("Refn doesn't support size %d"),
Hui> +		     size * TARGET_CHAR_BIT);

The old error message mentioned the actual opcode name.
I think "Refn" is less clear, since it isn't the name of anything
in DWARF.

Tom

Re: [PATCH] Fix agent code generate bug of ref

[Hui Zhu]

On Tue, Mar 12, 2013 at 11:16 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Hui" == Hui Zhu <hui_zhu@mentor.com> writes:
>
> Hui> According to the discussion with Tom in IRC.
> Hui> I add a check before call access_memory.
> Hui> +      if (size != 1 && size != 2 && size != 4 && size != 8)
> Hui> +        error (_("Refn doesn't support size %d"),
> Hui> +               size * TARGET_CHAR_BIT);
>
> The old error message mentioned the actual opcode name.
> I think "Refn" is less clear, since it isn't the name of anything
> in DWARF.
>
> Tom

What about change it to "DW_OP_deref and DW_OP_deref_size"?

Thanks,
Hui

Re: [PATCH] Fix agent code generate bug of ref

[Tom Tromey]

>>>>> "Hui" == Hui Zhu <teawater@gmail.com> writes:

Hui> On Tue, Mar 12, 2013 at 11:16 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>>> "Hui" == Hui Zhu <hui_zhu@mentor.com> writes:
>> 
Hui> According to the discussion with Tom in IRC.
Hui> I add a check before call access_memory.
Hui> +      if (size != 1 && size != 2 && size != 4 && size != 8)
Hui> +        error (_("Refn doesn't support size %d"),
Hui> +               size * TARGET_CHAR_BIT);

>> The old error message mentioned the actual opcode name.
>> I think "Refn" is less clear, since it isn't the name of anything
>> in DWARF.

Hui> What about change it to "DW_OP_deref and DW_OP_deref_size"?

It seems simple to just resurrect the old error message.

Tom

Re: [PATCH] Fix agent code generate bug of ref

[Hui Zhu]

On Tue, Mar 12, 2013 at 11:27 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Hui" == Hui Zhu <teawater@gmail.com> writes:
>
> Hui> On Tue, Mar 12, 2013 at 11:16 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>>>> "Hui" == Hui Zhu <hui_zhu@mentor.com> writes:
>>>
> Hui> According to the discussion with Tom in IRC.
> Hui> I add a check before call access_memory.
> Hui> +      if (size != 1 && size != 2 && size != 4 && size != 8)
> Hui> +        error (_("Refn doesn't support size %d"),
> Hui> +               size * TARGET_CHAR_BIT);
>
>>> The old error message mentioned the actual opcode name.
>>> I think "Refn" is less clear, since it isn't the name of anything
>>> in DWARF.
>
> Hui> What about change it to "DW_OP_deref and DW_OP_deref_size"?
>
> It seems simple to just resurrect the old error message.
>
> Tom

Oops.  I forgot that.
Check in http://sourceware.org/ml/gdb-cvs/2013-03/msg00103.html to use
old error.

Thanks,
Hui

Re: [PATCH] Fix agent code generate bug of ref

[Tom Tromey]

Hui> Oops.  I forgot that.
Hui> Check in http://sourceware.org/ml/gdb-cvs/2013-03/msg00103.html to use
Hui> old error.

Thank you.

Tom

Re: [PATCH] Fix agent code generate bug of ref

[Hui Zhu]

On Mon, Mar 11, 2013 at 10:38 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:
>
> Yao> SIZE is checked as "size in bits".  Your fix is right, but not
> Yao> complete. We also have to fix it when op is DW_OP_deref_size,
> Yao> something like the patch below.
>
> Yao> I am not familiar with this area, so I might be wrong.
>
> You are correct, but IMO it is simpler to divide each of the case
> constants by 8.
>
> Tom

Hi Tom,

Post a new version change 8 to TARGET_CHAR_BIT.

Thanks,
Hui

2013-03-12  Yao Qi  <yao@codesourcery.com>
	    Hui Zhu  <hui_zhu@mentor.com>

	* dwarf2loc.c (dwarf2_compile_expr_to_ax): Change use bits number
	in DW_OP_deref and DW_OP_deref_size.

--- a/dwarf2loc.c
+++ b/dwarf2loc.c
@@ -2933,7 +2933,7 @@ dwarf2_compile_expr_to_ax (struct agent_
 	    else
 	      size = addr_size;

-	    switch (size)
+	    switch (size * TARGET_CHAR_BIT)
 	      {
 	      case 8:
 		ax_simple (expr, aop_ref8);