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

[Pedro Alves <palves at redhat dot com>]

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