[RFA] Limited DW_OP_piece support

[RFA] Limited DW_OP_piece support

[Kevin Buettner]

The patch below adds limited DW_OP_piece support to dwarf2expr.c.  I
will post a patch to rs6000-tdep.c which illustrates what a
``dwarf2_compose_register_pieces'' method should look like.

Okay?

Index: dwarf2expr.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2expr.c,v
retrieving revision 1.7
diff -u -p -r1.7 dwarf2expr.c
--- dwarf2expr.c	14 May 2003 22:45:41 -0000	1.7
+++ dwarf2expr.c	22 May 2003 16:39:14 -0000
@@ -456,6 +456,98 @@ execute_stack_op (struct dwarf_expr_cont
 	    ctx->in_reg = 0;
 	  }
 	  break;
+
+	case DW_OP_piece:
+
+	  if (!gdbarch_dwarf2_compose_register_pieces_p (current_gdbarch))
+	    error ("DWARF-2 expression error: DW_OP_piece not supported for "
+		   "this architecture.");
+	  else
+	    {
+	      /* If the pieces consist of 2 or more registers in the
+		 proper order, then gdb "will do the right thing" by
+		 simply using the first register -- assuming, of course
+		 that the corresponding type information is correct. 
+		 Otherwise, complain.  
+		 
+		 For the case that we handle, there should already be
+		 a register pushed on the stack.  Our strategy will
+		 be to push the DW_OP_piece size information and then
+		 push the next register and it's size information, etc.
+		 Once that is done, an architecture specific method will
+		 be called to determine which register should be used
+		 to access the entire object described by the pieces.
+		 
+		 When we've finished the stack should look like this:
+
+		    reg num 1		Bottom of stack (index == 0)
+		    size num 1
+		    .
+		    .
+		    reg num N
+		    size num N		Top of stack (index == 2*N-1)  */
+
+	      CORE_ADDR regnum;
+
+	      /* First check to make sure there's only one thing on the
+		 stack and that's it's a register.  */
+	      if (ctx->stack_len != 1 || !ctx->in_reg)
+		error ("DWARF-2 expression error: DW_OP_piece expression "
+		       "is too complicated.");
+
+	      while (1)
+		{
+		  long nextregnum;
+
+		  /* Fetch the number of bytes for this piece.  */
+		  op_ptr = read_uleb128 (op_ptr, op_end, &uoffset);
+		  dwarf_expr_push (ctx, uoffset);
+
+		  /* Break out if we're done.  */
+		  if (op_ptr >= op_end)
+		    break;
+
+		  /* We expect to see a register next.  */
+		  op = *op_ptr++;
+		  if (DW_OP_reg0 <= op && op <= DW_OP_reg31)
+		    {
+		      reg = op - DW_OP_reg0;
+		      dwarf_expr_push (ctx, reg);
+		    }
+		  else if (op == DW_OP_regx)
+		    {
+		      op_ptr = read_uleb128 (op_ptr, op_end, &reg);
+		      dwarf_expr_push (ctx, reg);
+		    }
+		  else
+		    error ("DWARF-2 expression error: DW_OP_piece expression "
+			   "is too complicated.");
+
+		  /* Make sure that we haven't read too far and that
+		     a DW_OP_piece operator comes next.  */
+
+		  if (op_ptr >= op_end || (*op_ptr!= DW_OP_piece))
+		    error ("DWARF-2 expression error: DW_OP_piece expression "
+			   "is too complicated.");
+
+		  /* Skip over DW_OP_piece operator.  */
+		  op_ptr++;
+		}
+
+	      result = gdbarch_dwarf2_compose_register_pieces (
+		         current_gdbarch, ctx->stack_len / 2, &ctx->stack[0]);
+
+	      if (result < 0)
+		error ("DWARF-2 expression error: DW_OP_piece expression "
+		       "is too complicated.");
+
+	      /* Note: ctx->in_reg is already set.  We want result to be the
+	         only thing on the stack -- it will be pushed below.
+		 Make the stack empty so that this will occur.  */
+	      ctx->stack_len = 0;
+	    }
+	  break;
+
 	case DW_OP_dup:
 	  result = dwarf_expr_fetch (ctx, 0);
 	  break;
Index: gdbarch.sh
===================================================================
RCS file: /cvs/src/src/gdb/gdbarch.sh,v
retrieving revision 1.237
diff -u -p -r1.237 gdbarch.sh
--- gdbarch.sh	17 May 2003 05:59:58 -0000	1.237
+++ gdbarch.sh	22 May 2003 16:39:16 -0000
@@ -471,6 +471,7 @@ f:2:DWARF_REG_TO_REGNUM:int:dwarf_reg_to
 # to map one to one onto the sdb register numbers.
 f:2:SDB_REG_TO_REGNUM:int:sdb_reg_to_regnum:int sdb_regnr:sdb_regnr:::no_op_reg_to_regnum::0
 f:2:DWARF2_REG_TO_REGNUM:int:dwarf2_reg_to_regnum:int dwarf2_regnr:dwarf2_regnr:::no_op_reg_to_regnum::0
+M:2:DWARF2_COMPOSE_REGISTER_PIECES:long:dwarf2_compose_register_pieces:int count, CORE_ADDR *piece_info:count, piece_info
 f:2:REGISTER_NAME:const char *:register_name:int regnr:regnr:::legacy_register_name::0
 v::DEPRECATED_REGISTER_SIZE:int:deprecated_register_size
 v::DEPRECATED_REGISTER_BYTES:int:deprecated_register_bytes

Re: [RFA] Limited DW_OP_piece support

[Andrew Cagney]

> The patch below adds limited DW_OP_piece support to dwarf2expr.c.  I
> will post a patch to rs6000-tdep.c which illustrates what a
> ``dwarf2_compose_register_pieces'' method should look like.

I think GDB needs to just learn about location lists :-/

Andrew


> Index: dwarf2expr.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/dwarf2expr.c,v
> retrieving revision 1.7
> diff -u -p -r1.7 dwarf2expr.c
> --- dwarf2expr.c	14 May 2003 22:45:41 -0000	1.7
> +++ dwarf2expr.c	22 May 2003 16:39:14 -0000
> @@ -456,6 +456,98 @@ execute_stack_op (struct dwarf_expr_cont
>  	    ctx->in_reg = 0;
>  	  }
>  	  break;
> +
> +	case DW_OP_piece:
> +
> +	  if (!gdbarch_dwarf2_compose_register_pieces_p (current_gdbarch))
> +	    error ("DWARF-2 expression error: DW_OP_piece not supported for "
> +		   "this architecture.");
> +	  else
> +	    {
> +	      /* If the pieces consist of 2 or more registers in the
> +		 proper order, then gdb "will do the right thing" by
> +		 simply using the first register -- assuming, of course
> +		 that the corresponding type information is correct. 
> +		 Otherwise, complain.  
> +		 
> +		 For the case that we handle, there should already be
> +		 a register pushed on the stack.  Our strategy will
> +		 be to push the DW_OP_piece size information and then
> +		 push the next register and it's size information, etc.
> +		 Once that is done, an architecture specific method will
> +		 be called to determine which register should be used
> +		 to access the entire object described by the pieces.
> +		 
> +		 When we've finished the stack should look like this:
> +
> +		    reg num 1		Bottom of stack (index == 0)
> +		    size num 1
> +		    .
> +		    .
> +		    reg num N
> +		    size num N		Top of stack (index == 2*N-1)  */
> +
> +	      CORE_ADDR regnum;
> +
> +	      /* First check to make sure there's only one thing on the
> +		 stack and that's it's a register.  */
> +	      if (ctx->stack_len != 1 || !ctx->in_reg)
> +		error ("DWARF-2 expression error: DW_OP_piece expression "
> +		       "is too complicated.");
> +
> +	      while (1)
> +		{
> +		  long nextregnum;
> +
> +		  /* Fetch the number of bytes for this piece.  */
> +		  op_ptr = read_uleb128 (op_ptr, op_end, &uoffset);
> +		  dwarf_expr_push (ctx, uoffset);
> +
> +		  /* Break out if we're done.  */
> +		  if (op_ptr >= op_end)
> +		    break;
> +
> +		  /* We expect to see a register next.  */
> +		  op = *op_ptr++;
> +		  if (DW_OP_reg0 <= op && op <= DW_OP_reg31)
> +		    {
> +		      reg = op - DW_OP_reg0;
> +		      dwarf_expr_push (ctx, reg);
> +		    }
> +		  else if (op == DW_OP_regx)
> +		    {
> +		      op_ptr = read_uleb128 (op_ptr, op_end, &reg);
> +		      dwarf_expr_push (ctx, reg);
> +		    }
> +		  else
> +		    error ("DWARF-2 expression error: DW_OP_piece expression "
> +			   "is too complicated.");
> +
> +		  /* Make sure that we haven't read too far and that
> +		     a DW_OP_piece operator comes next.  */
> +
> +		  if (op_ptr >= op_end || (*op_ptr!= DW_OP_piece))
> +		    error ("DWARF-2 expression error: DW_OP_piece expression "
> +			   "is too complicated.");
> +
> +		  /* Skip over DW_OP_piece operator.  */
> +		  op_ptr++;
> +		}
> +
> +	      result = gdbarch_dwarf2_compose_register_pieces (
> +		         current_gdbarch, ctx->stack_len / 2, &ctx->stack[0]);
> +
> +	      if (result < 0)
> +		error ("DWARF-2 expression error: DW_OP_piece expression "
> +		       "is too complicated.");
> +
> +	      /* Note: ctx->in_reg is already set.  We want result to be the
> +	         only thing on the stack -- it will be pushed below.
> +		 Make the stack empty so that this will occur.  */
> +	      ctx->stack_len = 0;
> +	    }
> +	  break;
> +
>  	case DW_OP_dup:
>  	  result = dwarf_expr_fetch (ctx, 0);
>  	  break;
> Index: gdbarch.sh
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbarch.sh,v
> retrieving revision 1.237
> diff -u -p -r1.237 gdbarch.sh
> --- gdbarch.sh	17 May 2003 05:59:58 -0000	1.237
> +++ gdbarch.sh	22 May 2003 16:39:16 -0000
> @@ -471,6 +471,7 @@ f:2:DWARF_REG_TO_REGNUM:int:dwarf_reg_to
>  # to map one to one onto the sdb register numbers.
>  f:2:SDB_REG_TO_REGNUM:int:sdb_reg_to_regnum:int sdb_regnr:sdb_regnr:::no_op_reg_to_regnum::0
>  f:2:DWARF2_REG_TO_REGNUM:int:dwarf2_reg_to_regnum:int dwarf2_regnr:dwarf2_regnr:::no_op_reg_to_regnum::0
> +M:2:DWARF2_COMPOSE_REGISTER_PIECES:long:dwarf2_compose_register_pieces:int count, CORE_ADDR *piece_info:count, piece_info
>  f:2:REGISTER_NAME:const char *:register_name:int regnr:regnr:::legacy_register_name::0
>  v::DEPRECATED_REGISTER_SIZE:int:deprecated_register_size
>  v::DEPRECATED_REGISTER_BYTES:int:deprecated_register_bytes
> 
> 

Re: [RFA] Limited DW_OP_piece support

[Daniel Jacobowitz]

On Thu, May 22, 2003 at 10:00:39AM -0700, Kevin Buettner wrote:
> The patch below adds limited DW_OP_piece support to dwarf2expr.c.  I
> will post a patch to rs6000-tdep.c which illustrates what a
> ``dwarf2_compose_register_pieces'' method should look like.
> 
> Okay?

I would really strongly prefer that we not do it this way.

You'll notice that there are no other gdbarch calls in the expression
evaluator.  There might be some implicit ones through macros, for
instance there is TARGET_ADDR_BIT.  That needs to be fixed properly
some day already.

Instead, IMHO, we should devise a way to represent multiple locations
in the evaluator's return value.  This is not suggesting the complete
overhaul that we need to support multiple locations in the rest of GDB.
Then have the expression evaluator properly return a list of locations,
and have the massaging done via gdbarch in the evaluator's client. 
Does that sound reasonable?

Maybe a flag to the evaluator which says whether we're looking for a
location, for better sanity checking.  Not sure about that part.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] Limited DW_OP_piece support

[Kevin Buettner]

On May 22,  1:53pm, Andrew Cagney wrote:

> > The patch below adds limited DW_OP_piece support to dwarf2expr.c.  I
> > will post a patch to rs6000-tdep.c which illustrates what a
> > ``dwarf2_compose_register_pieces'' method should look like.
> 
> I think GDB needs to just learn about location lists :-/

It certainly should, but I fail to see how that relates to the
addition of DW_OP_piece support to GDB's DWARF 2 location expression
evaluator.  I think that what you meant to say is that GDB should
fully (and generically) support arbitrary location expressions. 
(Remember that the term "location list" is used to describe objects
whose location changes during its lifetime.)

BTW, I notice that I forgot the ChangeLog entries for my patch.  Here
they are:

	* dwarf2expr.c (execute_stack_op): Add limited DW_OP_piece support.
	* gdbarch.sh (dwarf2_compose_register_pieces): New method.
	* gdbarch.c, gdbarch.h: Regenerate.

Kevin

Re: [RFA] Limited DW_OP_piece support

[Andrew Cagney]

> On May 22,  1:53pm, Andrew Cagney wrote:
> 
> 
>> > The patch below adds limited DW_OP_piece support to dwarf2expr.c.  I
>> > will post a patch to rs6000-tdep.c which illustrates what a
>> > ``dwarf2_compose_register_pieces'' method should look like.
> 
>> 
>> I think GDB needs to just learn about location lists :-/
> 
> 
> It certainly should, but I fail to see how that relates to the
> addition of DW_OP_piece support to GDB's DWARF 2 location expression
> evaluator.  I think that what you meant to say is that GDB should
> fully (and generically) support arbitrary location expressions. 
> (Remember that the term "location list" is used to describe objects
> whose location changes during its lifetime.)

Yes, too many context sensative terms.  A list of sub-locations.

Given a ``struct value'', the [list of] sub-locations (pieces?) that 
describe how to find that values raw bytes.

Andrew


> BTW, I notice that I forgot the ChangeLog entries for my patch.  Here
> they are:
> 
> 	* dwarf2expr.c (execute_stack_op): Add limited DW_OP_piece support.
> 	* gdbarch.sh (dwarf2_compose_register_pieces): New method.
> 	* gdbarch.c, gdbarch.h: Regenerate.
> 
> Kevin
> 

Re: [RFA] Limited DW_OP_piece support

[Kevin Buettner]

On May 22,  2:19pm, Daniel Jacobowitz wrote:

> On Thu, May 22, 2003 at 10:00:39AM -0700, Kevin Buettner wrote:
> > The patch below adds limited DW_OP_piece support to dwarf2expr.c.  I
> > will post a patch to rs6000-tdep.c which illustrates what a
> > ``dwarf2_compose_register_pieces'' method should look like.
> > 
> > Okay?
> 
> I would really strongly prefer that we not do it this way.
> 
> You'll notice that there are no other gdbarch calls in the expression
> evaluator.  There might be some implicit ones through macros, for
> instance there is TARGET_ADDR_BIT.  That needs to be fixed properly
> some day already.
> 
> Instead, IMHO, we should devise a way to represent multiple locations
> in the evaluator's return value.  This is not suggesting the complete
> overhaul that we need to support multiple locations in the rest of GDB.
> Then have the expression evaluator properly return a list of locations,
> and have the massaging done via gdbarch in the evaluator's client. 
> Does that sound reasonable?

I must admit that it sure sounded reasonable when I first read it. 
I've been looking at the code to see how doable it is, and it's
looking less reasonable to me now.  It appears to me that there are
multiple clients and it seems ugly to do the massaging that you speak
of in multiple places.  (Or perhaps I misunderstand who the client
is?)

I don't want to argue too vigorously for the limited DW_OP_piece patch
that I submitted because I agree that it'd be better to have it
implemented in a more generic manner.  The one thing that my patch
does have going for it though is that, aside from the addition of the
gdbarch method and the architecture dependent support, it consists
solely of the addition of one case to a switch statement.  This makes
it very easy to back out when we finally do things "right".  Simply
delete that case (and the gdbarch method) and it's out.  I'd even be
willing to add some comments and possibly an ifdef to that case to
further delimit it from the rest of the code.  The comments would make
it clear that this is a stopgap measure that will eventually be
removed and would describe precisely how to remove it.

Kevin

Re: [RFA] Limited DW_OP_piece support

[Daniel Jacobowitz]

On Thu, May 22, 2003 at 02:19:31PM -0700, Kevin Buettner wrote:
> On May 22,  2:19pm, Daniel Jacobowitz wrote:
> 
> > On Thu, May 22, 2003 at 10:00:39AM -0700, Kevin Buettner wrote:
> > > The patch below adds limited DW_OP_piece support to dwarf2expr.c.  I
> > > will post a patch to rs6000-tdep.c which illustrates what a
> > > ``dwarf2_compose_register_pieces'' method should look like.
> > > 
> > > Okay?
> > 
> > I would really strongly prefer that we not do it this way.
> > 
> > You'll notice that there are no other gdbarch calls in the expression
> > evaluator.  There might be some implicit ones through macros, for
> > instance there is TARGET_ADDR_BIT.  That needs to be fixed properly
> > some day already.
> > 
> > Instead, IMHO, we should devise a way to represent multiple locations
> > in the evaluator's return value.  This is not suggesting the complete
> > overhaul that we need to support multiple locations in the rest of GDB.
> > Then have the expression evaluator properly return a list of locations,
> > and have the massaging done via gdbarch in the evaluator's client. 
> > Does that sound reasonable?
> 
> I must admit that it sure sounded reasonable when I first read it. 
> I've been looking at the code to see how doable it is, and it's
> looking less reasonable to me now.  It appears to me that there are
> multiple clients and it seems ugly to do the massaging that you speak
> of in multiple places.  (Or perhaps I misunderstand who the client
> is?)

I'm suggesting that the massaging be done in the caller of
dwarf_expr_eval.  There are three of them at present: one which only
cares about whether we need a frame, and two for locations.  One's the
frame base, and the other's via dwarf2_evaluate_loc_desc.

For the moment, I believe everything you need could be done in
dwarf2_evaluate_loc_desc.  The frame base will not (on current
platforms, etc.) use DW_OP_piece, and that call should be going away
anyway.  There will be more calls, as we use the evaluator for more,
but it's not clear how they should react to DW_OP_piece.

Another alternative is to do it in dwarf_expr_eval.  This would
probably want us to separate it into two functions: one for evaluating
an expression as a location, and one otherwise.  i.e. there are times
when DW_OP_piece should be handled, and times when it is not valid. 
They can have different return signatures.

Does that make more sense?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] Limited DW_OP_piece support

[Kevin Buettner]

On May 22,  5:29pm, Daniel Jacobowitz wrote:

> On Thu, May 22, 2003 at 02:19:31PM -0700, Kevin Buettner wrote:
> > On May 22,  2:19pm, Daniel Jacobowitz wrote:
> > 
> > > On Thu, May 22, 2003 at 10:00:39AM -0700, Kevin Buettner wrote:
> > > > The patch below adds limited DW_OP_piece support to dwarf2expr.c.  I
> > > > will post a patch to rs6000-tdep.c which illustrates what a
> > > > ``dwarf2_compose_register_pieces'' method should look like.
> > > > 
> > > > Okay?
> > > 
> > > I would really strongly prefer that we not do it this way.
> > > 
> > > You'll notice that there are no other gdbarch calls in the expression
> > > evaluator.  There might be some implicit ones through macros, for
> > > instance there is TARGET_ADDR_BIT.  That needs to be fixed properly
> > > some day already.
> > > 
> > > Instead, IMHO, we should devise a way to represent multiple locations
> > > in the evaluator's return value.  This is not suggesting the complete
> > > overhaul that we need to support multiple locations in the rest of GDB.
> > > Then have the expression evaluator properly return a list of locations,
> > > and have the massaging done via gdbarch in the evaluator's client. 
> > > Does that sound reasonable?
> > 
> > I must admit that it sure sounded reasonable when I first read it. 
> > I've been looking at the code to see how doable it is, and it's
> > looking less reasonable to me now.  It appears to me that there are
> > multiple clients and it seems ugly to do the massaging that you speak
> > of in multiple places.  (Or perhaps I misunderstand who the client
> > is?)
> 
> I'm suggesting that the massaging be done in the caller of
> dwarf_expr_eval.  There are three of them at present: one which only
> cares about whether we need a frame, and two for locations.  One's the
> frame base, and the other's via dwarf2_evaluate_loc_desc.
> 
> For the moment, I believe everything you need could be done in
> dwarf2_evaluate_loc_desc.  The frame base will not (on current
> platforms, etc.) use DW_OP_piece, and that call should be going away
> anyway.  There will be more calls, as we use the evaluator for more,
> but it's not clear how they should react to DW_OP_piece.
> 
> Another alternative is to do it in dwarf_expr_eval.  This would
> probably want us to separate it into two functions: one for evaluating
> an expression as a location, and one otherwise.  i.e. there are times
> when DW_OP_piece should be handled, and times when it is not valid. 
> They can have different return signatures.
> 
> Does that make more sense?

Yeah.  I was going too far up the stack.

Thanks,

Kevin