[PATCH] Handle LOC_COMPUTED variables in tracepoint actions

[PATCH] Handle LOC_COMPUTED variables in tracepoint actions

[Stan Shebs]

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

The last time that anybody used tracepoints much, stabs was still the 
norm and Dwarf 2 the newbie, and so nobody thought much about handling 
variables with computed locations.  Nowadays many local variables have 
computed locations, and we have to generate appropriate agent expression 
bytecodes.  Worse, the top-level encoding machinery for tracepoint 
actions will issue instructions to collect variables directly, not using 
bytecodes, so we need to detect when bytecodes are necessary.  (In 
theory we could always generate bytecodes for collection expressions; 
that would make a good cleanup for later.)

This patch doesn't handle every possible computed location, that's a big 
project of its own (basically translating arbitrary dwarf2 bytecode 
sequences into equivalent agent expression bytecodes), it just handles 
frame/base register combinations that are typical for local vars.  Even 
the simplified version is a little arcane, so I'll leave it up for 
comments for a few days before committing.

Stan

2009-12-18  Stan Shebs  <stan@codesourcery.com>

    * ax-gdb.h (gen_trace_for_var): Declare.
    * ax-gdb.c (gen_trace_for_var): New function.
    * dwarf2loc.c (dwarf_expr_frame_base_1): New function, split from...
    (dwarf_expr_frame_base): ...here.
    (dwarf2_tracepoint_var_ref): Add computed location case.
    * tracepoint.c (collect_symbol): Add scope arg and LOC_COMPUTED
    case.
    (add_local_symbols): Update call to collect_symbol.
    (encode_actions): Ditto.


[-- Attachment #2: loccomp-patch-1 --]
[-- Type: text/plain, Size: 9221 bytes --]

Index: ax-gdb.c
===================================================================
RCS file: /cvs/src/src/gdb/ax-gdb.c,v
retrieving revision 1.53
diff -p -r1.53 ax-gdb.c
*** ax-gdb.c	14 Jul 2009 21:40:30 -0000	1.53
--- ax-gdb.c	18 Dec 2009 19:27:46 -0000
*************** gen_expr (struct expression *exp, union 
*** 1768,1773 ****
--- 1768,1797 ----
  }
  \f
  
+ struct agent_expr *
+ gen_trace_for_var (CORE_ADDR scope, struct symbol *var)
+ {
+   struct cleanup *old_chain = 0;
+   struct agent_expr *ax = new_agent_expr (scope);
+   struct axs_value value;
+ 
+   old_chain = make_cleanup_free_agent_expr (ax);
+ 
+   trace_kludge = 1;
+   gen_var_ref (NULL, ax, &value, var);
+ 
+   /* Make sure we record the final object, and get rid of it.  */
+   gen_traced_pop (ax, &value);
+ 
+   /* Oh, and terminate.  */
+   ax_simple (ax, aop_end);
+ 
+   /* We have successfully built the agent expr, so cancel the cleanup
+      request.  If we add more cleanups that we always want done, this
+      will have to get more complicated.  */
+   discard_cleanups (old_chain);
+   return ax;
+ }
  
  /* Generating bytecode from GDB expressions: driver */
  
Index: ax-gdb.h
===================================================================
RCS file: /cvs/src/src/gdb/ax-gdb.h,v
retrieving revision 1.12
diff -p -r1.12 ax-gdb.h
*** ax-gdb.h	14 Jul 2009 21:40:30 -0000	1.12
--- ax-gdb.h	18 Dec 2009 19:27:46 -0000
*************** struct axs_value
*** 99,104 ****
--- 99,106 ----
     function to discover which registers the expression uses.  */
  extern struct agent_expr *gen_trace_for_expr (CORE_ADDR, struct expression *);
  
+ extern struct agent_expr *gen_trace_for_var (CORE_ADDR, struct symbol *);
+ 
  extern struct agent_expr *gen_eval_for_expr (CORE_ADDR, struct expression *);
  
  #endif /* AX_GDB_H */
Index: dwarf2loc.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2loc.c,v
retrieving revision 1.68
diff -p -r1.68 dwarf2loc.c
*** dwarf2loc.c	15 Sep 2009 16:20:53 -0000	1.68
--- dwarf2loc.c	18 Dec 2009 19:27:46 -0000
***************
*** 41,46 ****
--- 41,50 ----
  #include "gdb_string.h"
  #include "gdb_assert.h"
  
+ static void
+ dwarf_expr_frame_base_1 (struct symbol *framefunc, CORE_ADDR pc,
+ 			 gdb_byte **start, size_t * length);
+ 
  /* A helper function for dealing with location lists.  Given a
     symbol baton (BATON) and a pc value (PC), find the appropriate
     location expression, set *LOCEXPR_LENGTH, and return a pointer
*************** dwarf_expr_frame_base (void *baton, gdb_
*** 166,181 ****
       something has gone wrong.  */
    gdb_assert (framefunc != NULL);
  
    if (SYMBOL_LOCATION_BATON (framefunc) == NULL)
      *start = NULL;
    else if (SYMBOL_COMPUTED_OPS (framefunc) == &dwarf2_loclist_funcs)
      {
        struct dwarf2_loclist_baton *symbaton;
-       struct frame_info *frame = debaton->frame;
  
        symbaton = SYMBOL_LOCATION_BATON (framefunc);
!       *start = find_location_expression (symbaton, length,
! 					 get_frame_address_in_block (frame));
      }
    else
      {
--- 170,192 ----
       something has gone wrong.  */
    gdb_assert (framefunc != NULL);
  
+   dwarf_expr_frame_base_1 (framefunc,
+ 			   get_frame_address_in_block (debaton->frame),
+ 			   start, length);
+ }
+ 
+ static void
+ dwarf_expr_frame_base_1 (struct symbol *framefunc, CORE_ADDR pc,
+ 			 gdb_byte **start, size_t * length)
+ {
    if (SYMBOL_LOCATION_BATON (framefunc) == NULL)
      *start = NULL;
    else if (SYMBOL_COMPUTED_OPS (framefunc) == &dwarf2_loclist_funcs)
      {
        struct dwarf2_loclist_baton *symbaton;
  
        symbaton = SYMBOL_LOCATION_BATON (framefunc);
!       *start = find_location_expression (symbaton, length, pc);
      }
    else
      {
*************** dwarf2_tracepoint_var_ref (struct symbol
*** 617,637 ****
      }
    else if (data[0] == DW_OP_fbreg)
      {
!       /* And this is worse than just minimal; we should honor the frame base
! 	 as above.  */
!       int frame_reg;
        LONGEST frame_offset;
        gdb_byte *buf_end;
  
        buf_end = read_sleb128 (data + 1, data + size, &frame_offset);
        if (buf_end != data + size)
  	error (_("Unexpected opcode after DW_OP_fbreg for symbol \"%s\"."),
  	       SYMBOL_PRINT_NAME (symbol));
  
-       gdbarch_virtual_frame_pointer (gdbarch,
- 				     ax->scope, &frame_reg, &frame_offset);
        ax_reg (ax, frame_reg);
!       ax_const_l (ax, frame_offset);
        ax_simple (ax, aop_add);
  
        value->kind = axs_lvalue_memory;
--- 628,679 ----
      }
    else if (data[0] == DW_OP_fbreg)
      {
!       struct block *b;
!       struct symbol *framefunc;
!       int frame_reg = 0;
        LONGEST frame_offset;
        gdb_byte *buf_end;
+       gdb_byte *base_data;
+       size_t base_size;
+       LONGEST base_offset = 0;
+ 
+       b = block_for_pc (ax->scope);
+ 
+       if (!b)
+ 	error (_("No block found for address"));
+ 
+       framefunc = block_linkage_function (b);
+ 
+       if (!framefunc)
+ 	error (_("No function found for block"));
+ 
+       dwarf_expr_frame_base_1 (framefunc, ax->scope,
+ 			       &base_data, &base_size);
+ 
+       if (base_data[0] >= DW_OP_breg0
+ 	   && base_data[0] <= DW_OP_breg31)
+ 	{
+ 	  frame_reg = base_data[0] - DW_OP_breg0;
+ 	  buf_end = read_sleb128 (base_data + 1, base_data + base_size, &base_offset);
+ 	  if (buf_end != base_data + base_size)
+ 	    error (_("Unexpected opcode after DW_OP_breg%u for symbol \"%s\"."),
+ 		   frame_reg, SYMBOL_PRINT_NAME (symbol));
+ 	}
+       else
+ 	{
+ 	  /* We don't know what to do with the frame base expression,
+ 	     so we can't trace this variable; give up.  */
+ 	  error (_("Cannot generate expression to collect symbol \"%s\"."),
+ 		 SYMBOL_PRINT_NAME (symbol));
+ 	}
  
        buf_end = read_sleb128 (data + 1, data + size, &frame_offset);
        if (buf_end != data + size)
  	error (_("Unexpected opcode after DW_OP_fbreg for symbol \"%s\"."),
  	       SYMBOL_PRINT_NAME (symbol));
  
        ax_reg (ax, frame_reg);
!       ax_const_l (ax, base_offset + frame_offset);
        ax_simple (ax, aop_add);
  
        value->kind = axs_lvalue_memory;
Index: tracepoint.c
===================================================================
RCS file: /cvs/src/src/gdb/tracepoint.c,v
retrieving revision 1.126
diff -p -r1.126 tracepoint.c
*** tracepoint.c	14 Jul 2009 21:40:30 -0000	1.126
--- tracepoint.c	18 Dec 2009 19:27:46 -0000
*************** static void
*** 725,731 ****
  collect_symbol (struct collection_list *collect, 
  		struct symbol *sym,
  		struct gdbarch *gdbarch,
! 		long frame_regno, long frame_offset)
  {
    unsigned long len;
    unsigned int reg;
--- 725,732 ----
  collect_symbol (struct collection_list *collect, 
  		struct symbol *sym,
  		struct gdbarch *gdbarch,
! 		long frame_regno, long frame_offset,
! 		CORE_ADDR scope)
  {
    unsigned long len;
    unsigned int reg;
*************** collect_symbol (struct collection_list *
*** 817,822 ****
--- 818,867 ----
        printf_filtered ("%s has been optimized out of existence.\n",
  		       SYMBOL_PRINT_NAME (sym));
        break;
+ 
+     case LOC_COMPUTED:
+       {
+ 	struct agent_expr *aexpr;
+ 	struct cleanup *old_chain1 = NULL;
+ 	struct agent_reqs areqs;
+ 
+ 	aexpr = gen_trace_for_var (scope, sym);
+ 
+ 	old_chain1 = make_cleanup_free_agent_expr (aexpr);
+ 
+ 	ax_reqs (aexpr, &areqs);
+ 	if (areqs.flaw != agent_flaw_none)
+ 	  error (_("malformed expression"));
+ 	
+ 	if (areqs.min_height < 0)
+ 	  error (_("gdb: Internal error: expression has min height < 0"));
+ 	if (areqs.max_height > 20)
+ 	  error (_("expression too complicated, try simplifying"));
+ 
+ 	discard_cleanups (old_chain1);
+ 	add_aexpr (collect, aexpr);
+ 
+ 	/* take care of the registers */
+ 	if (areqs.reg_mask_len > 0)
+ 	  {
+ 	    int ndx1, ndx2;
+ 
+ 	    for (ndx1 = 0; ndx1 < areqs.reg_mask_len; ndx1++)
+ 	      {
+ 		QUIT;	/* allow user to bail out with ^C */
+ 		if (areqs.reg_mask[ndx1] != 0)
+ 		  {
+ 		    /* assume chars have 8 bits */
+ 		    for (ndx2 = 0; ndx2 < 8; ndx2++)
+ 		      if (areqs.reg_mask[ndx1] & (1 << ndx2))
+ 			/* it's used -- record it */
+ 			add_register (collect, 
+ 				      ndx1 * 8 + ndx2);
+ 		  }
+ 	      }
+ 	  }
+       }
+       break;
      }
  }
  
*************** add_local_symbols (struct collection_lis
*** 843,849 ****
  	    {
  	      count++;
  	      collect_symbol (collect, sym, gdbarch,
! 			      frame_regno, frame_offset);
  	    }
  	}
        if (BLOCK_FUNCTION (block))
--- 888,894 ----
  	    {
  	      count++;
  	      collect_symbol (collect, sym, gdbarch,
! 			      frame_regno, frame_offset, pc);
  	    }
  	}
        if (BLOCK_FUNCTION (block))
*************** encode_actions (struct breakpoint *t, ch
*** 1122,1128 ****
  				      exp->elts[2].symbol,
  				      t->gdbarch,
  				      frame_reg,
! 				      frame_offset);
  		      break;
  
  		    default:	/* full-fledged expression */
--- 1167,1174 ----
  				      exp->elts[2].symbol,
  				      t->gdbarch,
  				      frame_reg,
! 				      frame_offset,
! 				      t->loc->address);
  		      break;
  
  		    default:	/* full-fledged expression */

Re: [PATCH] Handle LOC_COMPUTED variables in tracepoint actions

[Tom Tromey]

>>>>> "Stan" == Stan Shebs <stan@codesourcery.com> writes:

Stan> The last time that anybody used tracepoints much, stabs was still
Stan> the norm and Dwarf 2 the newbie, and so nobody thought much about
Stan> handling variables with computed locations.

Stan> This patch doesn't handle every possible computed location, that's
Stan> a big project of its own (basically translating arbitrary dwarf2
Stan> bytecode sequences into equivalent agent expression bytecodes)

Fun project :-).  Let me suggest a feature request for this in
bugzilla...

Stan> Even the simplified version is a little arcane, so I'll leave it
Stan> up for comments for a few days before committing.

This patch looks pretty good to me.

Does this handle DW_OP_call_frame_cfa?  I didn't see any explicit
support for it, but I could be missing something.  IIUC, recent versions
of gcc tend to emit this for all local variables.

I also have a couple tiny nits to pick.
  
Stan> + struct agent_expr *
Stan> + gen_trace_for_var (CORE_ADDR scope, struct symbol *var)

Intro comment.

Stan> + static void
Stan> + dwarf_expr_frame_base_1 (struct symbol *framefunc, CORE_ADDR pc,
Stan> + 			 gdb_byte **start, size_t * length);

Extra space before "length".  This occurs twice.

Stan> + 	  /* We don't know what to do with the frame base expression,
Stan> + 	     so we can't trace this variable; give up.  */
Stan> + 	  error (_("Cannot generate expression to collect symbol \"%s\"."),
Stan> + 		 SYMBOL_PRINT_NAME (symbol));

As a user I would appreciate it if this error told me more about what
was wrong.  Like, maybe it could mention that the DWARF expression is
too complicated, or maybe ask me to file a bug report.

Tom

Re: [PATCH] Handle LOC_COMPUTED variables in tracepoint actions

[Stan Shebs]

Tom Tromey wrote:
>>>>>> "Stan" == Stan Shebs <stan@codesourcery.com> writes:
>>>>>>             
>
> Stan> The last time that anybody used tracepoints much, stabs was still
> Stan> the norm and Dwarf 2 the newbie, and so nobody thought much about
> Stan> handling variables with computed locations.
>
> Stan> This patch doesn't handle every possible computed location, that's
> Stan> a big project of its own (basically translating arbitrary dwarf2
> Stan> bytecode sequences into equivalent agent expression bytecodes)
>
> Fun project :-).  Let me suggest a feature request for this in
> bugzilla...
>
> Stan> Even the simplified version is a little arcane, so I'll leave it
> Stan> up for comments for a few days before committing.
>
> This patch looks pretty good to me.
>
> Does this handle DW_OP_call_frame_cfa?  I didn't see any explicit
> support for it, but I could be missing something.  IIUC, recent versions
> of gcc tend to emit this for all local variables.
>   
I don't think so - it's probably an accident waiting to happen. :-)
> Stan> + 	  /* We don't know what to do with the frame base expression,
> Stan> + 	     so we can't trace this variable; give up.  */
> Stan> + 	  error (_("Cannot generate expression to collect symbol \"%s\"."),
> Stan> + 		 SYMBOL_PRINT_NAME (symbol));
>
> As a user I would appreciate it if this error told me more about what
> was wrong.  Like, maybe it could mention that the DWARF expression is
> too complicated, or maybe ask me to file a bug report.
>   
I tinkered with the wording a bit, but it's difficult to explain.  In 
general the compiler is producing output that the bytecode compiler 
doesn't know how to handle, and there's not much the user can do about 
it.  It's severe enough that we should give up on the tracing run, so we 
need it to be an error rather than a warning, but internal_error is a 
bit excessive, since regular debugging still works, and the debugger 
state hasn't been corrupted in any way.

Stan