Re: [RFA] PR python/20190 - compute TLS symbol without a frame

[Pedro Alves <palves at redhat dot com>]

On 06/03/2016 10:16 PM, Tom Tromey wrote:

> diff --git a/gdb/defs.h b/gdb/defs.h
> index ed51396..ec90d30 100644
> --- a/gdb/defs.h
> +++ b/gdb/defs.h
> @@ -617,6 +617,22 @@ enum gdb_osabi
>  extern double atof (const char *);	/* X3.159-1989  4.10.1.1 */
>  #endif
>  
> +/* Enumerate the requirements a symbol has in order to be evaluated.
> +   These are listed in order of "strength" -- a later entry subsumes
> +   earlier ones.  */
> +
> +enum symbol_needs_kind
> +{
> +  /* No special requirements -- just memory.  */
> +  SYMBOL_NEEDS_NONE,
> +
> +  /* The symbol needs registers.  */
> +  SYMBOL_NEEDS_REGISTERS,

I think we should put a comment somewhere explaining _why_ is
this distinction useful to have.  Around here is probably a good
place.  IIUC, the reason is being able to read TLS symbols
from within a frame unwinder, when we don't have a frame yet,
because we're building it.

> +
> +  /* The symbol needs a frame.  */
> +  SYMBOL_NEEDS_FRAME
> +};
> +
>  /* Dynamic target-system-dependent parameters for GDB.  */
>  #include "gdbarch.h"
>  
> diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
> index adb0ac2..5020f82 100644
> --- a/gdb/dwarf2loc.c
> +++ b/gdb/dwarf2loc.c
> @@ -2729,7 +2729,7 @@ dwarf2_compile_property_to_c (struct ui_file *stream,
>  
>  struct needs_frame_baton
>  {
> -  int needs_frame;
> +  enum symbol_needs_kind needs;
>    struct dwarf2_per_cu_data *per_cu;
>  };
>  
> @@ -2739,7 +2739,7 @@ needs_frame_read_addr_from_reg (void *baton, int regnum)
>  {
>    struct needs_frame_baton *nf_baton = (struct needs_frame_baton *) baton;
>  
> -  nf_baton->needs_frame = 1;
> +  nf_baton->needs = SYMBOL_NEEDS_FRAME;
>    return 1;
>  }
>  
> @@ -2751,7 +2751,7 @@ needs_frame_get_reg_value (void *baton, struct type *type, int regnum)
>  {
>    struct needs_frame_baton *nf_baton = (struct needs_frame_baton *) baton;
>  
> -  nf_baton->needs_frame = 1;
> +  nf_baton->needs = SYMBOL_NEEDS_FRAME;
>    return value_zero (type, not_lval);
>  }
>  
> @@ -2772,7 +2772,7 @@ needs_frame_frame_base (void *baton, const gdb_byte **start, size_t * length)
>    *start = &lit0;
>    *length = 1;
>  
> -  nf_baton->needs_frame = 1;
> +  nf_baton->needs = SYMBOL_NEEDS_FRAME;
>  }
>  
>  /* CFA accesses require a frame.  */
> @@ -2782,7 +2782,7 @@ needs_frame_frame_cfa (void *baton)
>  {
>    struct needs_frame_baton *nf_baton = (struct needs_frame_baton *) baton;
>  
> -  nf_baton->needs_frame = 1;
> +  nf_baton->needs = SYMBOL_NEEDS_FRAME;
>    return 1;
>  }
>  
> @@ -2792,7 +2792,8 @@ needs_frame_tls_address (void *baton, CORE_ADDR offset)
>  {
>    struct needs_frame_baton *nf_baton = (struct needs_frame_baton *) baton;
>  
> -  nf_baton->needs_frame = 1;
> +  if (nf_baton->needs != SYMBOL_NEEDS_FRAME)
> +    nf_baton->needs = SYMBOL_NEEDS_REGISTERS;

Should we write this as:

  if (nf_baton->needs < SYMBOL_NEEDS_REGISTERS)
    nf_baton->needs = SYMBOL_NEEDS_REGISTERS;

?

May make it clearer there's ordering implied?

>    return 1;
>  }
>  
> @@ -2816,7 +2817,7 @@ needs_dwarf_reg_entry_value (struct dwarf_expr_context *ctx,
>  {
>    struct needs_frame_baton *nf_baton = (struct needs_frame_baton *) ctx->baton;
>  
> -  nf_baton->needs_frame = 1;
> +  nf_baton->needs = SYMBOL_NEEDS_FRAME;
>  
>    /* The expression may require some stub values on DWARF stack.  */
>    dwarf_expr_push_address (ctx, 0, 0);
> @@ -2861,7 +2862,7 @@ static const struct dwarf_expr_context_funcs needs_frame_ctx_funcs =
>  /* Return non-zero iff the location expression at DATA (length SIZE)
>     requires a frame to evaluate.  */
>  
> -static int
> +static enum symbol_needs_kind
>  dwarf2_loc_desc_needs_frame (const gdb_byte *data, size_t size,
>  			     struct dwarf2_per_cu_data *per_cu)

I think the method name should be updated too, as well as the
intro comment.  Both are still talking about "frame".
For the comment, maybe replace it with the standard
"Implementation of foo method of bar.", thus deferring to the
centralized documentation in the ops definition.

>  {
> @@ -2871,7 +2872,7 @@ dwarf2_loc_desc_needs_frame (const gdb_byte *data, size_t size,
>    struct cleanup *old_chain;
>    struct objfile *objfile = dwarf2_per_cu_objfile (per_cu);
>  
> -  baton.needs_frame = 0;
> +  baton.needs = SYMBOL_NEEDS_NONE;
>    baton.per_cu = per_cu;
>  
>    ctx = new_dwarf_expr_context ();
> @@ -2902,7 +2903,9 @@ dwarf2_loc_desc_needs_frame (const gdb_byte *data, size_t size,
>  
>    do_cleanups (old_chain);
>  
> -  return baton.needs_frame || in_reg;
> +  if (in_reg)
> +    baton.needs = SYMBOL_NEEDS_FRAME;
> +  return baton.needs;
>  }
>  
>  /* A helper function that throws an unimplemented error mentioning a
> @@ -3736,7 +3739,7 @@ locexpr_read_variable_at_entry (struct symbol *symbol, struct frame_info *frame)
>  }
>  
>  /* Return non-zero iff we need a frame to evaluate SYMBOL.  */
> -static int
> +static enum symbol_needs_kind
>  locexpr_read_needs_frame (struct symbol *symbol)

Ditto.  Etc.

>  {
>    struct dwarf2_locexpr_baton *dlbaton
> @@ -4530,7 +4533,7 @@ loclist_read_variable_at_entry (struct symbol *symbol, struct frame_info *frame)
>  }
>  
>  /* Return non-zero iff we need a frame to evaluate SYMBOL.  */
> -static int
> +static enum symbol_needs_kind
>  loclist_read_needs_frame (struct symbol *symbol)
>  {
>    /* If there's a location list, then assume we need to have a frame
> @@ -4539,7 +4542,7 @@ loclist_read_needs_frame (struct symbol *symbol)
>       is disabled in GCC at the moment until we figure out how to
>       represent it.  */
>  
> -  return 1;
> +  return SYMBOL_NEEDS_FRAME;
>  }
>  
>  /* Print a natural-language description of SYMBOL to STREAM.  This
> diff --git a/gdb/findvar.c b/gdb/findvar.c
> index a39d897..71bc48d 100644
> --- a/gdb/findvar.c
> +++ b/gdb/findvar.c
> @@ -337,11 +337,10 @@ address_to_signed_pointer (struct gdbarch *gdbarch, struct type *type,
>    store_signed_integer (buf, TYPE_LENGTH (type), byte_order, addr);
>  }
>  
> -/* Will calling read_var_value or locate_var_value on SYM end
> -   up caring what frame it is being evaluated relative to?  SYM must
> -   be non-NULL.  */
> -int
> -symbol_read_needs_frame (struct symbol *sym)
> +/* See value.h.  */
> +
> +enum symbol_needs_kind
> +symbol_read_needs (struct symbol *sym)
>  {
>    if (SYMBOL_COMPUTED_OPS (sym) != NULL)
>      return SYMBOL_COMPUTED_OPS (sym)->read_needs_frame (sym);
> @@ -358,7 +357,7 @@ symbol_read_needs_frame (struct symbol *sym)
>      case LOC_REF_ARG:
>      case LOC_REGPARM_ADDR:
>      case LOC_LOCAL:
> -      return 1;
> +      return SYMBOL_NEEDS_FRAME;
>  
>      case LOC_UNDEF:
>      case LOC_CONST:
> @@ -374,9 +373,17 @@ symbol_read_needs_frame (struct symbol *sym)
>      case LOC_CONST_BYTES:
>      case LOC_UNRESOLVED:
>      case LOC_OPTIMIZED_OUT:
> -      return 0;
> +      return SYMBOL_NEEDS_NONE;
>      }
> -  return 1;
> +  return SYMBOL_NEEDS_FRAME;
> +}
> +
> +/* See value.h.  */
> +
> +int
> +symbol_read_needs_frame (struct symbol *sym)
> +{
> +  return symbol_read_needs (sym) == SYMBOL_NEEDS_FRAME;
>  }
>  
>  /* Private data to be used with minsym_lookup_iterator_cb.  */
> @@ -574,6 +581,7 @@ default_read_var_value (struct symbol *var, const struct block *var_block,
>    struct value *v;
>    struct type *type = SYMBOL_TYPE (var);
>    CORE_ADDR addr;
> +  enum symbol_needs_kind sym_need;
>  
>    /* Call check_typedef on our type to make sure that, if TYPE is
>       a TYPE_CODE_TYPEDEF, its length is set to the length of the target type
> @@ -582,8 +590,11 @@ default_read_var_value (struct symbol *var, const struct block *var_block,
>       set the returned value type description correctly.  */
>    check_typedef (type);
>  
> -  if (symbol_read_needs_frame (var))
> +  sym_need = symbol_read_needs (var);
> +  if (sym_need == SYMBOL_NEEDS_FRAME)
>      gdb_assert (frame != NULL);
> +  else if (sym_need == SYMBOL_NEEDS_REGISTERS && !target_has_registers)
> +    error (_("Cannnot read `%s' without registers"), SYMBOL_PRINT_NAME (var));

Cannnnnnnnnnnnnot.  :-)


>  
>    if (frame != NULL)
>      frame = get_hosting_frame (var, var_block, frame);
> diff --git a/gdb/symtab.h b/gdb/symtab.h
> index 6f00baf..4e452d6 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -629,7 +629,8 @@ struct symbol_computed_ops
>       frame FRAME.  If the variable has been optimized out, return
>       zero.
>  
> -     Iff `read_needs_frame (SYMBOL)' is zero, then FRAME may be zero.  */
> +     Iff `read_needs_frame (SYMBOL)' is not SYMBOL_NEEDS_FRAME, then
> +     FRAME may be zero.  */
>  
>    struct value *(*read_variable) (struct symbol * symbol,
>  				  struct frame_info * frame);
> @@ -640,8 +641,9 @@ struct symbol_computed_ops
>    struct value *(*read_variable_at_entry) (struct symbol *symbol,
>  					   struct frame_info *frame);
>  
> -  /* Return non-zero if we need a frame to find the value of the SYMBOL.  */
> -  int (*read_needs_frame) (struct symbol * symbol);
> +  /* Return the requirements we need a frame to find the value of the
> +     SYMBOL.  */

I can't seem to parse this sentence.  Did you mean to remove "a frame",
like in:

  /* Return the requirements we need to find the value of the
     SYMBOL.  */

?

> +  enum symbol_needs_kind (*read_needs_frame) (struct symbol * symbol);
>  

As per comments above, I think we should rename this.  Leaving "frame" here
is now confusing, IMO.

>    /* Write to STREAM a natural-language description of the location of
>       SYMBOL, in the context of ADDR.  */
> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
> index 3b305a6..6ace0f4 100644
> --- a/gdb/testsuite/ChangeLog
> +++ b/gdb/testsuite/ChangeLog
> @@ -1,3 +1,9 @@
> +2016-06-03  Tom Tromey  <tom@tromey.com>
> +
> +	PR python/20190:
> +	* gdb.threads/tls.exp (check_thread_local): Add python symbol
> +	test.
> +
>  2016-06-02  Tom Tromey  <tom@tromey.com>
>  
>  	PR python/18984:
> diff --git a/gdb/testsuite/gdb.threads/tls.exp b/gdb/testsuite/gdb.threads/tls.exp
> index 29384e5..f9c0c27 100644
> --- a/gdb/testsuite/gdb.threads/tls.exp
> +++ b/gdb/testsuite/gdb.threads/tls.exp
> @@ -14,6 +14,8 @@
>  # You should have received a copy of the GNU General Public License
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>  
> +load_lib gdb-python.exp
> +
>  standard_testfile tls.c tls2.c
>  
>  if [istarget "*-*-linux"] then {
> @@ -70,6 +72,14 @@ proc check_thread_local {number} {
>  	    "= $expected_value" \
>  	    "${number} thread local storage"
>  
> +    if {![skip_python_tests]} {
> +	gdb_test_no_output \
> +	    "python sym = gdb.lookup_symbol('a_thread_local')\[0\]" \
> +	    "${number} look up a_thread_local symbol"
> +	gdb_test "python print(sym.value())" "$expected_value" \
> +	    "${number} get symbol value without frame"

I'm confused on what this is testing, and on whether this is
exercising the code changes.  Is there really no frame here?
AFAICS, this proc is always called with some thread selected,
so there should be a frame?

Thanks,
Pedro Alves