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

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

[Tom Tromey]

PR python/20190 arose from an exception I noticed when trying to use
the Python unwinder for Spider Monkey in Firefox.

The problem is that the unwinder wants to examine the value of a
thread-local variable.  However, sympy_value rejects this because
symbol_read_needs_frame returns true for a TLS variable.

This problem arose once before, though in a different context:

https://sourceware.org/bugzilla/show_bug.cgi?id=11803

At the time Pedro and Daniel pointed out a simpler way to fix that bug
(see links in 20190 if you are interested); but for this new bug I
couldn't think of a similar fix and ended up implementing Daniel's
other suggestion:

https://sourceware.org/ml/gdb-patches/2010-07/msg00393.html

That is, this patch makes it possible to detect whether a symbol needs
a specific frame, or whether it just needs the inferior to have
registers.

Built and regtested on x86-64 Fedora 23.

2016-06-03  Tom Tromey  <tom@tromey.com>

	PR python/20190:
	* value.h (symbol_read_needs): Declare.
	(symbol_read_needs_frame): Add comment.
	* symtab.h (struct symbol_computed_ops) <read_variable>: Update
	comment.
	<read_needs_frame>: Change return type.
	* findvar.c (symbol_read_needs): New function.
	(symbol_read_needs_frame): Rewrite.
	(default_read_var_value): Use symbol_read_needs.
	* dwarf2loc.c (struct needs_frame_baton) <needs>: Renamed from
	needs_frame.  Changed type.
	(needs_frame_read_addr_from_reg, needs_frame_get_reg_value)
	(needs_frame_frame_base, needs_frame_frame_cfa)
	(needs_frame_tls_address, needs_dwarf_reg_entry_value): Update.
	(dwarf2_loc_desc_needs_frame, locexpr_read_needs_frame)
	(loclist_read_needs_frame): Change return type.
	* defs.h (enum symbol_needs_kind): New.

2016-06-03  Tom Tromey  <tom@tromey.com>

	PR python/20190:
	* gdb.threads/tls.exp (check_thread_local): Add python symbol
	test.
---
 gdb/ChangeLog                     | 20 ++++++++++++++++++++
 gdb/defs.h                        | 16 ++++++++++++++++
 gdb/dwarf2loc.c                   | 29 ++++++++++++++++-------------
 gdb/findvar.c                     | 29 ++++++++++++++++++++---------
 gdb/symtab.h                      |  8 +++++---
 gdb/testsuite/ChangeLog           |  6 ++++++
 gdb/testsuite/gdb.threads/tls.exp | 10 ++++++++++
 gdb/value.h                       |  7 +++++++
 8 files changed, 100 insertions(+), 25 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index af4ddcc..ab9c3eb 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,23 @@
+2016-06-03  Tom Tromey  <tom@tromey.com>
+
+	PR python/20190:
+	* value.h (symbol_read_needs): Declare.
+	(symbol_read_needs_frame): Add comment.
+	* symtab.h (struct symbol_computed_ops) <read_variable>: Update
+	comment.
+	<read_needs_frame>: Change return type.
+	* findvar.c (symbol_read_needs): New function.
+	(symbol_read_needs_frame): Rewrite.
+	(default_read_var_value): Use symbol_read_needs.
+	* dwarf2loc.c (struct needs_frame_baton) <needs>: Renamed from
+	needs_frame.  Changed type.
+	(needs_frame_read_addr_from_reg, needs_frame_get_reg_value)
+	(needs_frame_frame_base, needs_frame_frame_cfa)
+	(needs_frame_tls_address, needs_dwarf_reg_entry_value): Update.
+	(dwarf2_loc_desc_needs_frame, locexpr_read_needs_frame)
+	(loclist_read_needs_frame): Change return type.
+	* defs.h (enum symbol_needs_kind): New.
+
 2016-06-02  Jon Turney  <jon.turney@dronecode.org.uk>
 
 	* windows-nat.c (handle_output_debug_string): Return type of
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,
+
+  /* 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;
   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)
 {
@@ -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)
 {
   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);
 }
 \f
-/* 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));
 
   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.  */
+  enum symbol_needs_kind (*read_needs_frame) (struct symbol * symbol);
 
   /* 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"
+    }
+
     gdb_test "p K::another_thread_local" \
 	    "= $me_variable" \
 	    "${number} another thread local storage"
diff --git a/gdb/value.h b/gdb/value.h
index f8ec854..1790258 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -671,6 +671,13 @@ extern struct value *value_of_register (int regnum, struct frame_info *frame);
 
 struct value *value_of_register_lazy (struct frame_info *frame, int regnum);
 
+/* Return the symbol's reading requirement.  */
+
+extern enum symbol_needs_kind symbol_read_needs (struct symbol *);
+
+/* Return true if the symbol needs a frame.  This is a wrapper for
+   symbol_read_needs that simply checks for SYMBOL_NEEDS_FRAME.  */
+
 extern int symbol_read_needs_frame (struct symbol *);
 
 extern struct value *read_var_value (struct symbol *var,
-- 
2.5.5

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

[Yao Qi]

Hi Tom,

On Fri, Jun 3, 2016 at 10:16 PM, Tom Tromey <tom@tromey.com> wrote:
> PR python/20190 arose from an exception I noticed when trying to use
> the Python unwinder for Spider Monkey in Firefox.
>
> The problem is that the unwinder wants to examine the value of a
> thread-local variable.  However, sympy_value rejects this because
> symbol_read_needs_frame returns true for a TLS variable.
>
> This problem arose once before, though in a different context:
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=11803
>
> At the time Pedro and Daniel pointed out a simpler way to fix that bug
> (see links in 20190 if you are interested); but for this new bug I
> couldn't think of a similar fix and ended up implementing Daniel's
> other suggestion:
>
> https://sourceware.org/ml/gdb-patches/2010-07/msg00393.html
>
> That is, this patch makes it possible to detect whether a symbol needs
> a specific frame, or whether it just needs the inferior to have
> registers.
>

I don't understand why your original attempt fixing PR11803
(https://sourceware.org/ml/gdb-patches/2010-07/msg00374.html) doesn't work
here.  IMO, it doesn't require frame when resolving tls.  In the process of
getting address of tls variable, the offset is available in the debug info, gdb
gets the module address first, then delegate target (linux-thread-db or remote)
to get the address.  Frame is not involved in this process.

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

[Tom Tromey]

>>>>> "Yao" == Yao Qi <qiyaoltc@gmail.com> writes:

Yao> I don't understand why your original attempt fixing PR11803
Yao> (https://sourceware.org/ml/gdb-patches/2010-07/msg00374.html)
Yao> doesn't work here.

The difference is in what happens when you try to refer to a __thread
variable without an inferior.  I think this was pointed out in one of
the follow-ups.

With the original patch, the failure mode looks like:

    (gdb) print a_thread_local 
    Cannot find thread-local storage for process 0, executable file /home/tromey/gdb/build/gdb/testsuite/outputs/gdb.threads/tls/tls:
    Cannot find thread-local variables on this target

With the current patch the result is nicer:

    (gdb) print a_thread_local 
    Cannnot read `a_thread_local' without registers

Tom

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

[Pedro Alves]

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);
>  }
>  \f
> -/* 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

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

[Pedro Alves]

On 06/28/2016 10:15 PM, Tom Tromey wrote:
>>>>>> "Yao" == Yao Qi <qiyaoltc@gmail.com> writes:
> 
> Yao> I don't understand why your original attempt fixing PR11803
> Yao> (https://sourceware.org/ml/gdb-patches/2010-07/msg00374.html)
> Yao> doesn't work here.
> 
> The difference is in what happens when you try to refer to a __thread
> variable without an inferior.  I think this was pointed out in one of
> the follow-ups.
> 
> With the original patch, the failure mode looks like:
> 
>     (gdb) print a_thread_local 
>     Cannot find thread-local storage for process 0, executable file /home/tromey/gdb/build/gdb/testsuite/outputs/gdb.threads/tls/tls:
>     Cannot find thread-local variables on this target
> 
> With the current patch the result is nicer:
> 
>     (gdb) print a_thread_local 
>     Cannnot read `a_thread_local' without registers

Is this / should this be tested somewhere?

Thanks,
Pedro Alves

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

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

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

I added this to the enum.

Pedro> Should we write this as:
Pedro>   if (nf_baton->needs < SYMBOL_NEEDS_REGISTERS)
Pedro>      nf_baton-> needs = SYMBOL_NEEDS_REGISTERS;
Pedro> ?
Pedro> May make it clearer there's ordering implied?

I don't think it matters much either way, but I went ahead and changed
it.

>> +static enum symbol_needs_kind
>> dwarf2_loc_desc_needs_frame (const gdb_byte *data, size_t size,
>> struct dwarf2_per_cu_data *per_cu)

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

I've now changed a number of function names here and tried to update all
the comments.

>> +    error (_("Cannnot read `%s' without registers"), SYMBOL_PRINT_NAME (var));

Pedro> Cannnnnnnnnnnnnot.  :-)

I changed this one to "Khaaaaaan!!!!".

Pedro> I can't seem to parse this sentence.  Did you mean to remove "a frame",
Pedro> like in:
Pedro>   /* Return the requirements we need to find the value of the
Pedro>      SYMBOL.  */
Pedro> ?
>> +  enum symbol_needs_kind (*read_needs_frame) (struct symbol * symbol);
Pedro> As per comments above, I think we should rename this.  Leaving
Pedro> "frame" here is now confusing, IMO.

I completely rewrote this to:

  /* Find the "symbol_needs_kind" value for the given symbol.  This
     value determines whether reading the symbol needs memory (e.g., a
     global variable), just registers (a thread-local), or a frame (a
     local variable).  */
  enum symbol_needs_kind (*get_symbol_read_needs) (struct symbol * symbol);

>> +    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"

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

It's a bit subtle and I had to go digging again to remind myself of why
this test works.

Basically it boils down to py-symbol.c:sympy_value:

      if (symbol_read_needs_frame (symbol) && frame_info == NULL)
	error (_("symbol requires a frame to compute its value"));

Here, frame_info comes from the caller -- and in the test we're
explicitly not passing in a frame.  So, this attempt to get the symbol's
value is rejected.

However, it ought to work, because a frame isn't necessary to compute
the value.

> With the current patch the result is nicer:
> 
>     (gdb) print a_thread_local 
>     Cannnot read `a_thread_local' without registers

Pedro> Is this / should this be tested somewhere?

I added a test for this.

Tom

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

[Pedro Alves]

On 07/24/2016 05:53 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> I think we should put a comment somewhere explaining _why_ is
> Pedro> this distinction useful to have.  Around here is probably a good
> Pedro> place.  IIUC, the reason is being able to read TLS symbols
> Pedro> from within a frame unwinder, when we don't have a frame yet,
> Pedro> because we're building it.
> 
> I added this to the enum.
> 
> Pedro> Should we write this as:
> Pedro>   if (nf_baton->needs < SYMBOL_NEEDS_REGISTERS)
> Pedro>      nf_baton-> needs = SYMBOL_NEEDS_REGISTERS;
> Pedro> ?
> Pedro> May make it clearer there's ordering implied?
> 
> I don't think it matters much either way, but I went ahead and changed
> it.

Thanks.  I should have probably explained why I thought
I'd suggest it.  My reasoning was that while this:

  if (nf_baton->needs != SYMBOL_NEEDS_FRAME)
    nf_baton->needs = SYMBOL_NEEDS_REGISTERS;

should be read as:

- only frob "nf_baton->needs" it not set to a stricter (higher)
  value yet

and requires the reader processing that:

- SYMBOL_NEEDS_FRAME is higher than SYMBOL_NEEDS_REGISTERS

- SYMBOL_NEEDS_FRAME is the _only_ value that is higher
  than SYMBOL_NEEDS_REGISTERS (which could no longer be true
  someday)

this:

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

is self-explanatory for being written in terms of a
single enum value.

> I completely rewrote this to:
> 
>   /* Find the "symbol_needs_kind" value for the given symbol.  This
>      value determines whether reading the symbol needs memory (e.g., a
>      global variable), just registers (a thread-local), or a frame (a
>      local variable).  */
>   enum symbol_needs_kind (*get_symbol_read_needs) (struct symbol * symbol);

That's great, thanks.

> 
>>> +    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"
> 
> Pedro> I'm confused on what this is testing, and on whether this is
> Pedro> exercising the code changes.  Is there really no frame here?
> Pedro> AFAICS, this proc is always called with some thread selected,
> Pedro> so there should be a frame?
> 
> It's a bit subtle and I had to go digging again to remind myself of why
> this test works.
> 
> Basically it boils down to py-symbol.c:sympy_value:
> 
>       if (symbol_read_needs_frame (symbol) && frame_info == NULL)
> 	error (_("symbol requires a frame to compute its value"));
> 
> Here, frame_info comes from the caller -- and in the test we're
> explicitly not passing in a frame.  So, this attempt to get the symbol's
> value is rejected.

I see.  Maybe add some small comment to the test mentioning
that "symbol.value" takes an optional frame argument and we're
purposely not passing one?

> 
> However, it ought to work, because a frame isn't necessary to compute
> the value.
> 
>> With the current patch the result is nicer:
>>
>>     (gdb) print a_thread_local 
>>     Cannnot read `a_thread_local' without registers
> 
> Pedro> Is this / should this be tested somewhere?
> 
> I added a test for this.

Great.

Thanks,
Pedro Alves

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

[Tom Tromey]

I tried sending this once before, but for some reason it appears not to
have gone through.

Pedro> I see.  Maybe add some small comment to the test mentioning
Pedro> that "symbol.value" takes an optional frame argument and we're
Pedro> purposely not passing one?

Here's a version that adds this comment.

thanks,
Tom


commit e840129c9e28e4cafabed4986ad131efb689bbd3
Author: Tom Tromey <tom@tromey.com>
Date:   Fri Jun 3 14:11:08 2016 -0600

    PR python/20190 - compute TLS symbol without a frame
    
    PR python/20190 arose from an exception I noticed when trying to use
    the Python unwinder for Spider Monkey in Firefox.
    
    The problem is that the unwinder wants to examine the value of a
    thread-local variable.  However, sympy_value rejects this because
    symbol_read_needs_frame returns true for a TLS variable.
    
    This problem arose once before, though in a different context:
    
    https://sourceware.org/bugzilla/show_bug.cgi?id=11803
    
    At the time Pedro and Daniel pointed out a simpler way to fix that bug
    (see links in 20190 if you are interested); but for this new bug I
    couldn't think of a similar fix and ended up implementing Daniel's
    other suggestion:
    
    https://sourceware.org/ml/gdb-patches/2010-07/msg00393.html
    
    That is, this patch makes it possible to detect whether a symbol needs
    a specific frame, or whether it just needs the inferior to have
    registers.
    
    Built and regtested on x86-64 Fedora 24.
    
    2016-07-25  Tom Tromey  <tom@tromey.com>
    
    	* symtab.c (register_symbol_computed_impl): Update.
    	PR python/20190:
    	* value.h (symbol_read_needs): Declare.
    	(symbol_read_needs_frame): Add comment.
    	* symtab.h (struct symbol_computed_ops) <read_variable>: Update
    	comment.
    	<get_symbol_read_needs>: Rename.  Change return type.
    	* findvar.c (symbol_read_needs): New function.
    	(symbol_read_needs_frame): Rewrite.
    	(default_read_var_value): Use symbol_read_needs.
    	* dwarf2loc.c (struct symbol_needs_baton): Rename.
    	<needs>: Renamed from needs_frame.  Changed type.
    	(needs_frame_read_addr_from_reg, symbol_needs_get_reg_value)
    	(symbol_needs_read_mem, symbol_needs_frame_base)
    	(symbol_needs_frame_cfa, symbol_needs_tls_address)
    	(symbol_needs_dwarf_call): Rename.
    	(needs_dwarf_reg_entry_value): Update.
    	(symbol_needs_ctx_funcs, dwarf2_loc_desc_get_symbol_read_needs):
    	Rename and update.
    	(locexpr_get_symbol_read_needs, loclist_symbol_needs): Likewise.
    	(dwarf2_locexpr_funcs, dwarf2_loclist_funcs): Update.
    	* defs.h (enum symbol_needs_kind): New.
    
    2016-07-25  Tom Tromey  <tom@tromey.com>
    
    	PR python/20190:
    	* gdb.threads/tls.exp (check_thread_local): Add python symbol
    	test.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 3940188..82611b7 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,28 @@
+2016-07-25  Tom Tromey  <tom@tromey.com>
+
+	* symtab.c (register_symbol_computed_impl): Update.
+	PR python/20190:
+	* value.h (symbol_read_needs): Declare.
+	(symbol_read_needs_frame): Add comment.
+	* symtab.h (struct symbol_computed_ops) <read_variable>: Update
+	comment.
+	<get_symbol_read_needs>: Rename.  Change return type.
+	* findvar.c (symbol_read_needs): New function.
+	(symbol_read_needs_frame): Rewrite.
+	(default_read_var_value): Use symbol_read_needs.
+	* dwarf2loc.c (struct symbol_needs_baton): Rename.
+	<needs>: Renamed from needs_frame.  Changed type.
+	(needs_frame_read_addr_from_reg, symbol_needs_get_reg_value)
+	(symbol_needs_read_mem, symbol_needs_frame_base)
+	(symbol_needs_frame_cfa, symbol_needs_tls_address)
+	(symbol_needs_dwarf_call): Rename.
+	(needs_dwarf_reg_entry_value): Update.
+	(symbol_needs_ctx_funcs, dwarf2_loc_desc_get_symbol_read_needs):
+	Rename and update.
+	(locexpr_get_symbol_read_needs, loclist_symbol_needs): Likewise.
+	(dwarf2_locexpr_funcs, dwarf2_loclist_funcs): Update.
+	* defs.h (enum symbol_needs_kind): New.
+
 2016-07-25  Simon Marchi  <simon.marchi@ericsson.com>
 
 	* top.h (make_delete_ui_cleanup): New declaration.
diff --git a/gdb/defs.h b/gdb/defs.h
index 5088390..fee5f41 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -629,6 +629,25 @@ 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.  This fine-grained distinction is important because
+   it allows for the evaluation of a TLS symbol during unwinding --
+   when unwinding one has access to registers, but not the frame
+   itself, because that is being constructed.  */
+
+enum symbol_needs_kind
+{
+  /* No special requirements -- just memory.  */
+  SYMBOL_NEEDS_NONE,
+
+  /* The symbol needs registers.  */
+  SYMBOL_NEEDS_REGISTERS,
+
+  /* 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 548e468..e60475f 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -2726,21 +2726,21 @@ dwarf2_compile_property_to_c (struct ui_file *stream,
 }
 
 \f
-/* Helper functions and baton for dwarf2_loc_desc_needs_frame.  */
+/* Helper functions and baton for dwarf2_loc_desc_get_symbol_read_needs.  */
 
-struct needs_frame_baton
+struct symbol_needs_baton
 {
-  int needs_frame;
+  enum symbol_needs_kind needs;
   struct dwarf2_per_cu_data *per_cu;
 };
 
 /* Reads from registers do require a frame.  */
 static CORE_ADDR
-needs_frame_read_addr_from_reg (void *baton, int regnum)
+symbol_needs_read_addr_from_reg (void *baton, int regnum)
 {
-  struct needs_frame_baton *nf_baton = (struct needs_frame_baton *) baton;
+  struct symbol_needs_baton *nf_baton = (struct symbol_needs_baton *) baton;
 
-  nf_baton->needs_frame = 1;
+  nf_baton->needs = SYMBOL_NEEDS_FRAME;
   return 1;
 }
 
@@ -2748,61 +2748,64 @@ needs_frame_read_addr_from_reg (void *baton, int regnum)
    Reads from registers do require a frame.  */
 
 static struct value *
-needs_frame_get_reg_value (void *baton, struct type *type, int regnum)
+symbol_needs_get_reg_value (void *baton, struct type *type, int regnum)
 {
-  struct needs_frame_baton *nf_baton = (struct needs_frame_baton *) baton;
+  struct symbol_needs_baton *nf_baton = (struct symbol_needs_baton *) baton;
 
-  nf_baton->needs_frame = 1;
+  nf_baton->needs = SYMBOL_NEEDS_FRAME;
   return value_zero (type, not_lval);
 }
 
 /* Reads from memory do not require a frame.  */
 static void
-needs_frame_read_mem (void *baton, gdb_byte *buf, CORE_ADDR addr, size_t len)
+symbol_needs_read_mem (void *baton, gdb_byte *buf, CORE_ADDR addr, size_t len)
 {
   memset (buf, 0, len);
 }
 
 /* Frame-relative accesses do require a frame.  */
 static void
-needs_frame_frame_base (void *baton, const gdb_byte **start, size_t * length)
+symbol_needs_frame_base (void *baton, const gdb_byte **start, size_t * length)
 {
   static gdb_byte lit0 = DW_OP_lit0;
-  struct needs_frame_baton *nf_baton = (struct needs_frame_baton *) baton;
+  struct symbol_needs_baton *nf_baton = (struct symbol_needs_baton *) baton;
 
   *start = &lit0;
   *length = 1;
 
-  nf_baton->needs_frame = 1;
+  nf_baton->needs = SYMBOL_NEEDS_FRAME;
 }
 
 /* CFA accesses require a frame.  */
 
 static CORE_ADDR
-needs_frame_frame_cfa (void *baton)
+symbol_needs_frame_cfa (void *baton)
 {
-  struct needs_frame_baton *nf_baton = (struct needs_frame_baton *) baton;
+  struct symbol_needs_baton *nf_baton = (struct symbol_needs_baton *) baton;
 
-  nf_baton->needs_frame = 1;
+  nf_baton->needs = SYMBOL_NEEDS_FRAME;
   return 1;
 }
 
-/* Thread-local accesses do require a frame.  */
+/* Thread-local accesses require registers, but not a frame.  */
 static CORE_ADDR
-needs_frame_tls_address (void *baton, CORE_ADDR offset)
+symbol_needs_tls_address (void *baton, CORE_ADDR offset)
 {
-  struct needs_frame_baton *nf_baton = (struct needs_frame_baton *) baton;
+  struct symbol_needs_baton *nf_baton = (struct symbol_needs_baton *) baton;
 
-  nf_baton->needs_frame = 1;
+  if (nf_baton->needs <= SYMBOL_NEEDS_REGISTERS)
+    nf_baton->needs = SYMBOL_NEEDS_REGISTERS;
   return 1;
 }
 
-/* Helper interface of per_cu_dwarf_call for dwarf2_loc_desc_needs_frame.  */
+/* Helper interface of per_cu_dwarf_call for
+   dwarf2_loc_desc_get_symbol_read_needs.  */
 
 static void
-needs_frame_dwarf_call (struct dwarf_expr_context *ctx, cu_offset die_offset)
+symbol_needs_dwarf_call (struct dwarf_expr_context *ctx, cu_offset die_offset)
 {
-  struct needs_frame_baton *nf_baton = (struct needs_frame_baton *) ctx->baton;
+  struct symbol_needs_baton *nf_baton =
+    (struct symbol_needs_baton *) ctx->baton;
 
   per_cu_dwarf_call (ctx, die_offset, nf_baton->per_cu,
 		     ctx->funcs->get_frame_pc, ctx->baton);
@@ -2815,9 +2818,10 @@ needs_dwarf_reg_entry_value (struct dwarf_expr_context *ctx,
 			     enum call_site_parameter_kind kind,
 			     union call_site_parameter_u kind_u, int deref_size)
 {
-  struct needs_frame_baton *nf_baton = (struct needs_frame_baton *) ctx->baton;
+  struct symbol_needs_baton *nf_baton =
+    (struct symbol_needs_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);
@@ -2841,38 +2845,39 @@ needs_get_obj_addr (void *baton)
   return 1;
 }
 
-/* Virtual method table for dwarf2_loc_desc_needs_frame below.  */
+/* Virtual method table for dwarf2_loc_desc_get_symbol_read_needs
+   below.  */
 
-static const struct dwarf_expr_context_funcs needs_frame_ctx_funcs =
+static const struct dwarf_expr_context_funcs symbol_needs_ctx_funcs =
 {
-  needs_frame_read_addr_from_reg,
-  needs_frame_get_reg_value,
-  needs_frame_read_mem,
-  needs_frame_frame_base,
-  needs_frame_frame_cfa,
-  needs_frame_frame_cfa,	/* get_frame_pc */
-  needs_frame_tls_address,
-  needs_frame_dwarf_call,
+  symbol_needs_read_addr_from_reg,
+  symbol_needs_get_reg_value,
+  symbol_needs_read_mem,
+  symbol_needs_frame_base,
+  symbol_needs_frame_cfa,
+  symbol_needs_frame_cfa,	/* get_frame_pc */
+  symbol_needs_tls_address,
+  symbol_needs_dwarf_call,
   NULL,				/* get_base_type */
   needs_dwarf_reg_entry_value,
   needs_get_addr_index,
   needs_get_obj_addr
 };
 
-/* Return non-zero iff the location expression at DATA (length SIZE)
-   requires a frame to evaluate.  */
+/* Compute the correct symbol_needs_kind value for the location
+   expression at DATA (length SIZE).  */
 
-static int
-dwarf2_loc_desc_needs_frame (const gdb_byte *data, size_t size,
-			     struct dwarf2_per_cu_data *per_cu)
+static enum symbol_needs_kind
+dwarf2_loc_desc_get_symbol_read_needs (const gdb_byte *data, size_t size,
+				       struct dwarf2_per_cu_data *per_cu)
 {
-  struct needs_frame_baton baton;
+  struct symbol_needs_baton baton;
   struct dwarf_expr_context *ctx;
   int in_reg;
   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 ();
@@ -2884,7 +2889,7 @@ dwarf2_loc_desc_needs_frame (const gdb_byte *data, size_t size,
   ctx->ref_addr_size = dwarf2_per_cu_ref_addr_size (per_cu);
   ctx->offset = dwarf2_per_cu_text_offset (per_cu);
   ctx->baton = &baton;
-  ctx->funcs = &needs_frame_ctx_funcs;
+  ctx->funcs = &symbol_needs_ctx_funcs;
 
   dwarf_expr_eval (ctx, data, size);
 
@@ -2903,7 +2908,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,15 +3743,17 @@ locexpr_read_variable_at_entry (struct symbol *symbol, struct frame_info *frame)
 				     dlbaton->size);
 }
 
-/* Return non-zero iff we need a frame to evaluate SYMBOL.  */
-static int
-locexpr_read_needs_frame (struct symbol *symbol)
+/* Implementation of get_symbol_read_needs from
+   symbol_computed_ops.  */
+
+static enum symbol_needs_kind
+locexpr_get_symbol_read_needs (struct symbol *symbol)
 {
   struct dwarf2_locexpr_baton *dlbaton
     = (struct dwarf2_locexpr_baton *) SYMBOL_LOCATION_BATON (symbol);
 
-  return dwarf2_loc_desc_needs_frame (dlbaton->data, dlbaton->size,
-				      dlbaton->per_cu);
+  return dwarf2_loc_desc_get_symbol_read_needs (dlbaton->data, dlbaton->size,
+						dlbaton->per_cu);
 }
 
 /* Return true if DATA points to the end of a piece.  END is one past
@@ -4473,7 +4482,7 @@ locexpr_generate_c_location (struct symbol *sym, struct ui_file *stream,
 const struct symbol_computed_ops dwarf2_locexpr_funcs = {
   locexpr_read_variable,
   locexpr_read_variable_at_entry,
-  locexpr_read_needs_frame,
+  locexpr_get_symbol_read_needs,
   locexpr_describe_location,
   0,	/* location_has_loclist */
   locexpr_tracepoint_var_ref,
@@ -4530,9 +4539,11 @@ loclist_read_variable_at_entry (struct symbol *symbol, struct frame_info *frame)
   return value_of_dwarf_block_entry (SYMBOL_TYPE (symbol), frame, data, size);
 }
 
-/* Return non-zero iff we need a frame to evaluate SYMBOL.  */
-static int
-loclist_read_needs_frame (struct symbol *symbol)
+/* Implementation of get_symbol_read_needs from
+   symbol_computed_ops.  */
+
+static enum symbol_needs_kind
+loclist_symbol_needs (struct symbol *symbol)
 {
   /* If there's a location list, then assume we need to have a frame
      to choose the appropriate location expression.  With tracking of
@@ -4540,7 +4551,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
@@ -4684,7 +4695,7 @@ loclist_generate_c_location (struct symbol *sym, struct ui_file *stream,
 const struct symbol_computed_ops dwarf2_loclist_funcs = {
   loclist_read_variable,
   loclist_read_variable_at_entry,
-  loclist_read_needs_frame,
+  loclist_symbol_needs,
   loclist_describe_location,
   1,	/* location_has_loclist */
   loclist_tracepoint_var_ref,
diff --git a/gdb/findvar.c b/gdb/findvar.c
index c64ec06..6e28a29 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -337,14 +337,13 @@ address_to_signed_pointer (struct gdbarch *gdbarch, struct type *type,
   store_signed_integer (buf, TYPE_LENGTH (type), byte_order, addr);
 }
 \f
-/* 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);
+    return SYMBOL_COMPUTED_OPS (sym)->get_symbol_read_needs (sym);
 
   switch (SYMBOL_CLASS (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.  */
@@ -575,6 +582,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
@@ -583,8 +591,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 (_("Cannot read `%s' without registers"), SYMBOL_PRINT_NAME (var));
 
   if (frame != NULL)
     frame = get_hosting_frame (var, var_block, frame);
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 5c4305d..e776a0c 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -6092,7 +6092,7 @@ register_symbol_computed_impl (enum address_class aclass,
   gdb_assert (ops != NULL);
   gdb_assert (ops->tracepoint_var_ref != NULL);
   gdb_assert (ops->describe_location != NULL);
-  gdb_assert (ops->read_needs_frame != NULL);
+  gdb_assert (ops->get_symbol_read_needs != NULL);
   gdb_assert (ops->read_variable != NULL);
 
   return result;
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 6f00baf..9df4efb 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,11 @@ 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);
+  /* Find the "symbol_needs_kind" value for the given symbol.  This
+     value determines whether reading the symbol needs memory (e.g., a
+     global variable), just registers (a thread-local), or a frame (a
+     local variable).  */
+  enum symbol_needs_kind (*get_symbol_read_needs) (struct symbol * symbol);
 
   /* 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 609d262..065225c 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2016-07-25  Tom Tromey  <tom@tromey.com>
+
+	PR python/20190:
+	* gdb.threads/tls.exp (check_thread_local): Add python symbol
+	test.
+
 2016-07-25  Simon Marchi  <simon.marchi@ericsson.com>
 
 	* gdb.base/new-ui.exp (do_test_invalid_args): New
diff --git a/gdb/testsuite/gdb.threads/tls.exp b/gdb/testsuite/gdb.threads/tls.exp
index 29384e5..5c07844 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,17 @@ 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"
+	# We intentionally do not pass a frame to "value" here.  If a
+	# TLS variable requires a frame, this will fail.  However, if
+	# it does not require a frame, then it will succeed.
+	gdb_test "python print(sym.value())" "$expected_value" \
+	    "${number} get symbol value without frame"
+    }
+
     gdb_test "p K::another_thread_local" \
 	    "= $me_variable" \
 	    "${number} another thread local storage"
@@ -139,6 +152,10 @@ proc check_thread_stack {number spin_threads spin_threads_level} {
 }
 
 clean_restart ${binfile}
+
+gdb_test "print a_thread_local" \
+    "Cannot read .a_thread_local. without registers"
+
 if ![runto_main] then {
    fail "Can't run to main"
    return 0
diff --git a/gdb/value.h b/gdb/value.h
index 0b417b4..3299e87 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -672,6 +672,13 @@ extern struct value *value_of_register (int regnum, struct frame_info *frame);
 
 struct value *value_of_register_lazy (struct frame_info *frame, int regnum);
 
+/* Return the symbol's reading requirement.  */
+
+extern enum symbol_needs_kind symbol_read_needs (struct symbol *);
+
+/* Return true if the symbol needs a frame.  This is a wrapper for
+   symbol_read_needs that simply checks for SYMBOL_NEEDS_FRAME.  */
+
 extern int symbol_read_needs_frame (struct symbol *);
 
 extern struct value *read_var_value (struct symbol *var,

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

[Pedro Alves]

On 07/26/2016 05:47 PM, Tom Tromey wrote:
> I tried sending this once before, but for some reason it appears not to
> have gone through.
> 
> Pedro> I see.  Maybe add some small comment to the test mentioning
> Pedro> that "symbol.value" takes an optional frame argument and we're
> Pedro> purposely not passing one?
> 
> Here's a version that adds this comment.

LGTM.

Thanks,
Pedro Alves