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

[Tom Tromey <tom at tromey dot com>]

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