This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA] PR python/20190 - compute TLS symbol without a frame
- From: Pedro Alves <palves at redhat dot com>
- To: Tom Tromey <tom at tromey dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Mon, 25 Jul 2016 17:00:17 +0100
- Subject: Re: [RFA] PR python/20190 - compute TLS symbol without a frame
- Authentication-results: sourceware.org; auth=none
- References: <1464988574-17075-1-git-send-email-tom@tromey.com> <92513300-637c-e0c8-2736-3b13c33d6f1c@redhat.com> <87eg6jm0zt.fsf@tromey.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