From: Tom Tromey <tom@tromey.com>
To: Pedro Alves <palves@redhat.com>
Cc: Tom Tromey <tom@tromey.com>, gdb-patches@sourceware.org
Subject: Re: [RFA] PR python/20190 - compute TLS symbol without a frame
Date: Sun, 24 Jul 2016 16:53:00 -0000 [thread overview]
Message-ID: <87eg6jm0zt.fsf@tromey.com> (raw)
In-Reply-To: <92513300-637c-e0c8-2736-3b13c33d6f1c@redhat.com> (Pedro Alves's message of "Fri, 22 Jul 2016 12:14:14 +0100")
>>>>> "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
next prev parent reply other threads:[~2016-07-24 16:53 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-03 21:16 Tom Tromey
2016-06-28 14:11 ` Yao Qi
2016-06-28 21:15 ` Tom Tromey
2016-07-22 11:18 ` Pedro Alves
2016-07-22 11:14 ` Pedro Alves
2016-07-24 16:53 ` Tom Tromey [this message]
2016-07-25 16:00 ` Pedro Alves
2016-07-26 16:47 ` Tom Tromey
2016-07-26 17:28 ` Pedro Alves
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87eg6jm0zt.fsf@tromey.com \
--to=tom@tromey.com \
--cc=gdb-patches@sourceware.org \
--cc=palves@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).