[RFA] 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 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(-)
  

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.
  

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

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
  

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
  

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

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