[PATCH] Disallow pseudo-registers in agent expression.

[PATCH] Disallow pseudo-registers in agent expression.

[andrzej zaborowski]

Collecting a pseudo-register in a tracepoint action gives a failed
assert and probably wouldn't work anyway:

(gdb) action
Enter actions for tracepoint 1, one per line.
End with a line saying just "end".
> collect $pc
regcache.c:163: internal-error: register_type: Assertion `regnum >= 0
&& regnum < descr->nr_cooked_registers' failed.
A problem internal to GDB has been detected,

The patch also prevents a segfault when "action" or "while-stepping"
is ended with a ^D.

Attached is a mailer-unmangled version of the same file:
diff --git a/gdb/ax-gdb.c b/gdb/ax-gdb.c
--- a/gdb/ax-gdb.c
+++ b/gdb/ax-gdb.c
@@ -1607,6 +1607,8 @@ gen_expr (union exp_element **pc, struct agent_expr *ax,
        if (reg == -1)
          internal_error (__FILE__, __LINE__,
                          _("Register $%s not available"), name);
+       if (reg >= gdbarch_num_regs (current_gdbarch))
+         error (_("GDB agent expressions cannot use pseudo-registers."));
        value->kind = axs_lvalue_register;
        value->u.reg = reg;
        value->type = register_type (current_gdbarch, reg);
index 2d744ae..ba57fee 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -862,8 +862,11 @@ read_actions (struct tracepoint *t)
        line = gdb_readline (0);

       if (!line)
-       line = "end";
-
+       {
+         line = xstrdup ("end");
+         printf_filtered ("end\n");
+       }
+
       linetype = validate_actionline (&line, t);
       if (linetype == BADLINE)
        continue;               /* already warned -- collect another line */

Re: [PATCH] Disallow pseudo-registers in agent expression.

[andrzej zaborowski]

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

On 26/01/2008, andrzej zaborowski <balrogg@gmail.com> wrote:
> Attached is a mailer-unmangled version of the same file:

Oops

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gdb-trace-syn.patch --]
[-- Type: text/x-patch; name=gdb-trace-syn.patch, Size: 1081 bytes --]

2008-01-26  Andrzej Zaborowski  <balrog@zabor.org>
	* ax-gdb.c (gen_expr): Check that register is raw or cooked.
	* tracepoint.c (read_actions): Actions need to be free-able.

diff --git a/gdb/ax-gdb.c b/gdb/ax-gdb.c
--- a/gdb/ax-gdb.c
+++ b/gdb/ax-gdb.c
@@ -1607,6 +1607,8 @@ gen_expr (union exp_element **pc, struct agent_expr *ax,
 	if (reg == -1)
 	  internal_error (__FILE__, __LINE__,
 			  _("Register $%s not available"), name);
+	if (reg >= gdbarch_num_regs (current_gdbarch))
+	  error (_("GDB agent expressions cannot use pseudo-registers."));
 	value->kind = axs_lvalue_register;
 	value->u.reg = reg;
 	value->type = register_type (current_gdbarch, reg);
index 2d744ae..ba57fee 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -862,8 +862,11 @@ read_actions (struct tracepoint *t)
 	line = gdb_readline (0);
 
       if (!line)
-	line = "end";
-      
+	{
+	  line = xstrdup ("end");
+	  printf_filtered ("end\n");
+	}
+
       linetype = validate_actionline (&line, t);
       if (linetype == BADLINE)
 	continue;		/* already warned -- collect another line */

Re: [PATCH] Disallow pseudo-registers in agent expression.

[Jim Blandy]

On some targets, all the user-visible registers are pseudo-registers.
What we need is a gdbarch method (optional) that the tracepoint code
could call, passing it a struct agent_expr and a pseudo-register
number, to have the architecture code append the appropriate bytecodes
to access that register.

Re: [PATCH] Disallow pseudo-registers in agent expression.

[andrzej zaborowski]

On 27/01/2008, Jim Blandy <jimb@red-bean.com> wrote:
> On some targets, all the user-visible registers are pseudo-registers.
> What we need is a gdbarch method (optional) that the tracepoint code
> could call, passing it a struct agent_expr and a pseudo-register
> number, to have the architecture code append the appropriate bytecodes
> to access that register.

That's ofcourse doable but sounds tough and bug-prone. The mapping of
numbers to raw registers is to some extent obvious and unlikely to
change, while the pseudo-registers may change with gdb version so the
target has more gdb version dependence.

Independently of that there's currenly a failed assertion so the
target can't do anything if a pseudo-register is used.

Re: [PATCH] Disallow pseudo-registers in agent expression.

[Jim Blandy]

On Jan 28, 2008 3:51 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
> On 27/01/2008, Jim Blandy <jimb@red-bean.com> wrote:
> > On some targets, all the user-visible registers are pseudo-registers.
> > What we need is a gdbarch method (optional) that the tracepoint code
> > could call, passing it a struct agent_expr and a pseudo-register
> > number, to have the architecture code append the appropriate bytecodes
> > to access that register.
>
> That's ofcourse doable but sounds tough and bug-prone. The mapping of
> numbers to raw registers is to some extent obvious and unlikely to
> change, while the pseudo-registers may change with gdb version so the
> target has more gdb version dependence.

I'm not sure I expressed myself well.  I was imagining something like this:

*** ax-gdb.c	07 Jan 2008 08:31:13 -0800	1.40
--- ax-gdb.c	28 Jan 2008 17:21:28 -0800	
***************
*** 1607,1615 ****
  	if (reg == -1)
  	  internal_error (__FILE__, __LINE__,
  			  _("Register $%s not available"), name);
! 	value->kind = axs_lvalue_register;
! 	value->u.reg = reg;
! 	value->type = register_type (current_gdbarch, reg);
        }
        break;

--- 1607,1627 ----
  	if (reg == -1)
  	  internal_error (__FILE__, __LINE__,
  			  _("Register $%s not available"), name);
!         if (reg < gdbarch_num_regs (current_gdbarch))
!           {
!             value->kind = axs_lvalue_register;
!             value->u.reg = reg;
!             value->type = register_type (current_gdbarch, reg);
!           }
!         else
!           {
!             /* It's a pseudo-register reference; ask the architecture
!                to generate appropriate bytecode for it.  */
!             if (! gdbarch_gen_pseudo_reg_ax (current_gdbarch, ax, value, reg))
!               error (_("Couldn't generate agent expression"
!                        " to refer to register '%s'"),
!                      gdbarch_register_name (current_gdbarch, reg));
!           }
        }
        break;

This seems pretty reasonable to me: only the architecture knows how
pseudo-register values are computed from the raw register values --- a
pseudo-register might be two raw registers spliced together, for
example --- so only the architecture knows the right agent bytecode to
generate.

Re: [PATCH] Disallow pseudo-registers in agent expression.

[Jim Blandy]

Andrzej, is this idea something that you would be interested in
implementing?  It'd be great to get this working.

On Jan 28, 2008 5:28 PM, Jim Blandy <jimb@red-bean.com> wrote:
> On Jan 28, 2008 3:51 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
> > On 27/01/2008, Jim Blandy <jimb@red-bean.com> wrote:
> > > On some targets, all the user-visible registers are pseudo-registers.
> > > What we need is a gdbarch method (optional) that the tracepoint code
> > > could call, passing it a struct agent_expr and a pseudo-register
> > > number, to have the architecture code append the appropriate bytecodes
> > > to access that register.
> >
> > That's ofcourse doable but sounds tough and bug-prone. The mapping of
> > numbers to raw registers is to some extent obvious and unlikely to
> > change, while the pseudo-registers may change with gdb version so the
> > target has more gdb version dependence.
>
> I'm not sure I expressed myself well.  I was imagining something like this:
>
> *** ax-gdb.c    07 Jan 2008 08:31:13 -0800      1.40
> --- ax-gdb.c    28 Jan 2008 17:21:28 -0800
> ***************
> *** 1607,1615 ****
>         if (reg == -1)
>           internal_error (__FILE__, __LINE__,
>                           _("Register $%s not available"), name);
> !       value->kind = axs_lvalue_register;
> !       value->u.reg = reg;
> !       value->type = register_type (current_gdbarch, reg);
>         }
>         break;
>
> --- 1607,1627 ----
>         if (reg == -1)
>           internal_error (__FILE__, __LINE__,
>                           _("Register $%s not available"), name);
> !         if (reg < gdbarch_num_regs (current_gdbarch))
> !           {
> !             value->kind = axs_lvalue_register;
> !             value->u.reg = reg;
> !             value->type = register_type (current_gdbarch, reg);
> !           }
> !         else
> !           {
> !             /* It's a pseudo-register reference; ask the architecture
> !                to generate appropriate bytecode for it.  */
> !             if (! gdbarch_gen_pseudo_reg_ax (current_gdbarch, ax, value, reg))
> !               error (_("Couldn't generate agent expression"
> !                        " to refer to register '%s'"),
> !                      gdbarch_register_name (current_gdbarch, reg));
> !           }
>         }
>         break;
>
> This seems pretty reasonable to me: only the architecture knows how
> pseudo-register values are computed from the raw register values --- a
> pseudo-register might be two raw registers spliced together, for
> example --- so only the architecture knows the right agent bytecode to
> generate.
>

Re: [PATCH] Disallow pseudo-registers in agent expression.

[Jim Blandy]

I've committed the following, to fix part of this.

gdb/ChangeLog:
2008-02-05  Jim Blandy  <jimb@red-bean.com>

	* ax-gdb.c (gen_expr): Yield ordinary error if asked to trace a
	pseudoregister, not an internal error.
	Reported by: Andrzej Zaborowski

diff -r fb37ff1fb896 -r da7d103c89a5 gdb/ax-gdb.c
--- a/gdb/ax-gdb.c	Tue Feb 05 07:36:35 2008 -0800
+++ b/gdb/ax-gdb.c	Tue Feb 05 07:56:09 2008 -0800
@@ -1607,6 +1607,10 @@ gen_expr (union exp_element **pc, struct
 	if (reg == -1)
 	  internal_error (__FILE__, __LINE__,
 			  _("Register $%s not available"), name);
+	if (reg >= gdbarch_num_regs (current_gdbarch))
+	  error (_("'%s' is a pseudo-register; "
+		   "GDB cannot yet trace pseudoregister contents."),
+		 name);
 	value->kind = axs_lvalue_register;
 	value->u.reg = reg;
 	value->type = register_type (current_gdbarch, reg);

Re: [PATCH] Disallow pseudo-registers in agent expression.

[Jim Blandy]

I've committed the following, to fix the second issue.  I don't think
this change is large enough to need a copyright assignment.

Thank you very much, Andrzej!

gdb/ChangeLog:
2008-02-05  Andrzej Zaborowski  <balrogg@gmail.com>

	* tracepoint.c (read_actions): Handle end-of-text indicator
	in action list properly.  (Committed by Jim Blandy)

Index: gdb/tracepoint.c
===================================================================
RCS file: /cvs/src/src/gdb/tracepoint.c,v
retrieving revision 1.100
diff -u -r1.100 tracepoint.c
--- gdb/tracepoint.c	11 Jan 2008 13:34:15 -0000	1.100
+++ gdb/tracepoint.c	5 Feb 2008 16:05:30 -0000
@@ -862,7 +862,10 @@
 	line = gdb_readline (0);

       if (!line)
-	line = "end";
+        {
+          line = xstrdup ("end");
+          printf_filtered ("end\n");
+        }

       linetype = validate_actionline (&line, t);
       if (linetype == BADLINE)

Re: [PATCH] Disallow pseudo-registers in agent expression.

[andrzej zaborowski]

Hi, sorry for not responding earlier.

On 05/02/2008, Jim Blandy <jimb@red-bean.com> wrote:
> Andrzej, is this idea something that you would be interested in
> implementing?  It'd be great to get this working.

As far as I understand it's still a change that requires implementing
new bits in architecture code, because architectures don't know how to
make bytecode for pseudo-registers right now.
Apart from little time I have zero knowledge about 95% of the
architectures in gdb :(

But yes, the approach is reasonable.

Thanks for pushing the fixes.
Regards

Re: [PATCH] Disallow pseudo-registers in agent expression.

[Jim Blandy]

On Feb 5, 2008 10:22 AM, andrzej zaborowski <balrogg@gmail.com> wrote:
> As far as I understand it's still a change that requires implementing
> new bits in architecture code, because architectures don't know how to
> make bytecode for pseudo-registers right now.

That's right.  The suggested patch I posted doesn't show this, but the
way to do it would be to make it a gdbarch method with a predicate
indicating whether the architecture provides the method or not (an 'M'
entry in gdbarch.sh; search for "M -> " in there for docs), and then
have ax-gdb.c check for the presence of the method, use it if present,
and throw an error otherwise.  This would allow us to implement the
method only on the architectures on which some contributor was
motivated to implement it.

> Apart from little time I have zero knowledge about 95% of the
> architectures in gdb :(

Well, there are a lot of architectures.  I think many of our
contributors know only a few.