Re: RFA: Fix tracepoints for LOC_COMPUTED - sort of

[Jim Blandy <jimb@redhat.com>]

Daniel Jacobowitz <drow@mvista.com> writes:
> Like the description function, this is pretty minimal; I don't have time to
> write a full evaluator right now, and I don't have access to a tracepoint
> stub, so I suggest someone who does implement this properly.  For now,
> though, this handles the most likely types: DW_OP_reg and DW_OP_fbreg.  It's
> enough to fix the test failures; it's correct as far as it goes; and it
> properly raises an error if it gets confused.
> 
> Is this OK?  Should I file a PR about the need for a proper
> evaluator?

Well, the problem is likely to sit around indefinitely:
1) I don't think anyone's using tracepoints at the moment.
2) Compiling Dwarf 2 bytecodes into ax bytecodes is, in general, not
   a simple thing at all.

So its importance and difficulty both work against it.  Would a
comment be better in this sort of situation?  I don't know.

> Index: dwarf2expr.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/dwarf2expr.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 dwarf2expr.c
> --- dwarf2expr.c	21 Feb 2003 15:24:17 -0000	1.1
> +++ dwarf2expr.c	23 Feb 2003 18:24:14 -0000
> @@ -115,7 +115,7 @@ dwarf_expr_eval (struct dwarf_expr_conte
>     by R, and return the new value of BUF.  Verify that it doesn't extend
>     past BUF_END.  */
>  
> -static unsigned char *
> +unsigned char *
>  read_uleb128 (unsigned char *buf, unsigned char *buf_end, ULONGEST * r)
>  {
>    unsigned shift = 0;
> @@ -141,7 +141,7 @@ read_uleb128 (unsigned char *buf, unsign
>     by R, and return the new value of BUF.  Verify that it doesn't extend
>     past BUF_END.  */
>  
> -static unsigned char *
> +unsigned char *
>  read_sleb128 (unsigned char *buf, unsigned char *buf_end, LONGEST * r)
>  {
>    unsigned shift = 0;
> Index: dwarf2expr.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/dwarf2expr.h,v
> retrieving revision 1.1
> diff -u -p -r1.1 dwarf2expr.h
> --- dwarf2expr.h	21 Feb 2003 15:24:17 -0000	1.1
> +++ dwarf2expr.h	23 Feb 2003 18:24:14 -0000
> @@ -94,4 +94,10 @@ void dwarf_expr_eval (struct dwarf_expr_
>  		      size_t len);
>  CORE_ADDR dwarf_expr_fetch (struct dwarf_expr_context *ctx, int n);
>  
> +
> +unsigned char *read_uleb128 (unsigned char *buf, unsigned char *buf_end,
> +			     ULONGEST * r);
> +unsigned char *read_sleb128 (unsigned char *buf, unsigned char *buf_end,
> +			     LONGEST * r);
> +
>  #endif

You know, dwarf2read.c's read_{un,}signed_leb128 don't actually need
their BFD argument for anything.  The call to bfd_get_8 ignores it;
it's just a char fetch anyway.  In the future, I wonder if we could
just use those everywhere.

> Index: dwarf2loc.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/dwarf2loc.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 dwarf2loc.c
> --- dwarf2loc.c	21 Feb 2003 15:24:17 -0000	1.1
> +++ dwarf2loc.c	23 Feb 2003 18:24:14 -0000
> @@ -26,6 +26,8 @@
>  #include "gdbcore.h"
>  #include "target.h"
>  #include "inferior.h"
> +#include "ax.h"
> +#include "ax-gdb.h"
>  
>  #include "elf/dwarf2.h"
>  #include "dwarf2expr.h"
> @@ -277,11 +279,58 @@ locexpr_describe_location (struct symbol
>    return 1;
>  }
>  
> +void
> +locexpr_tracepoint_var_ref (struct symbol * symbol, struct agent_expr * ax,
> +			    struct axs_value * value)
> +{
> +  /* FIXME: This one is also a bit minimal.  */
> +  struct dwarf2_locexpr_baton *dlbaton = SYMBOL_LOCATION_BATON (symbol);
> +
> +  if (dlbaton->size == 0)
> +    error ("Optimized out.");

The error message ought to be a complete sentence, and mention
SYMBOL_NAME.

> +  if (dlbaton->size == 1
> +      && dlbaton->data[0] >= DW_OP_reg0
> +      && dlbaton->data[0] <= DW_OP_reg31)
> +    {
> +      value->kind = axs_lvalue_register;
> +      value->u.reg = dlbaton->data[0] - DW_OP_reg0;
> +    }
> +  else if (dlbaton->data[0] == DW_OP_regx)
> +    {
> +      ULONGEST reg;
> +      read_uleb128 (dlbaton->data + 1, dlbaton->data + dlbaton->size,
> +		    &reg);
> +      value->kind = axs_lvalue_register;
> +      value->u.reg = reg;
> +    }
> +  else if (dlbaton->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;
> +
> +      TARGET_VIRTUAL_FRAME_POINTER (ax->scope, &frame_reg, &frame_offset);
> +      ax_reg (ax, frame_reg);
> +      ax_const_l (ax, frame_offset);
> +      ax_simple (ax, aop_add);
> +
> +      read_sleb128 (dlbaton->data + 1, dlbaton->data + dlbaton->size,
> +		    &frame_offset);
> +      ax_const_l (ax, frame_offset);
> +      ax_simple (ax, aop_add);
> +      value->kind = axs_lvalue_memory;
> +    }
> +  else
> +    error ("Can't do it.");

Gotta have a better error message, dude.  It only takes a few
seconsd.  Think of the poor user.

You should check that that's actually the end of the expression, too.