public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Simon Marchi <simark@simark.ca>
To: Thiago Jung Bauermann <thiago.bauermann@linaro.org>,
	Luis Machado <luis.machado@arm.com>
Cc: Andrew Burgess <aburgess@redhat.com>,
	Thiago Jung Bauermann via Gdb-patches
	<gdb-patches@sourceware.org>
Subject: Re: [PATCH v3 4/8] gdbserver/linux-aarch64: When thread stops, update its target description
Date: Wed, 1 Feb 2023 22:47:20 -0500	[thread overview]
Message-ID: <e6c0ec52-d8d3-decf-7363-8e7d81e1f882@simark.ca> (raw)
In-Reply-To: <87y1pgaib6.fsf@linaro.org>



On 2/1/23 21:54, Thiago Jung Bauermann wrote:
> 
> Luis Machado <luis.machado@arm.com> writes:
> 
>> On 2/1/23 16:21, Simon Marchi wrote:
>>>
>>>>> diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
>>>>> index 221de85aa2ee..b52eb23cc444 100644
>>>>> --- a/gdbserver/linux-low.h
>>>>> +++ b/gdbserver/linux-low.h
>>>>> @@ -604,6 +604,12 @@ class linux_process_target : public process_stratum_target
>>>>>     /* Architecture-specific setup for the current thread.  */
>>>>>     virtual void low_arch_setup () = 0;
>>>>>   +  /* Allows arch-specific code to set the thread's target description when the
>>>>> +     inferior stops.  Returns nullptr if no thread-specific target description
>>>>> +     is necessary.  */
>>>>> +  virtual const struct target_desc *
>>>>> +    get_thread_tdesc (const thread_info *thread);
>>>>
>>>> I think the comment for this function is not correct.  The function does
>>>> not SET the thread's target description, but just GETS a target
>>>> description suitable for `thread`.  It's the caller's job to do the
>>>> setting.
>>> This comment also gave me pause.  How does a getter set something.  I
>>> then understood that it allowed the arch-specific code to provide a
>>> thread-specific tdesc.  I would suggest just:
>>
>> FWIW, I read it as "the functions *allows* arch-specific code to set".
>> So it doesn't set on its own, but it does allow something else to do
>> it.
> 
> Yes, that's what was in my mind when I wrote the comment. But I agree
> it's unclear, and I adopted Simon's suggested version.
> 
>>> The other thought I had while re-reading the patch is why do we need to
>>> return and store nullptr if the thread target description is the same as
>>> the main one for the process.  get_thread_tdesc could just return
>>> process_info->tdesc if we don't need a separate tdesc, and we would
>>> store that same pointer in thread_info->tdesc.
> 
> We don't need to return and store nullptr if the thread target
> description is the same as the main one for the process. Things will
> work fine if we do as you suggest. IIRC my private branch worked liked
> that for a while, before I changed it to the current version.
> 
> I changed it because I thought it was a clearer mental model if
> thread_info->tdesc is nullptr when there's not thread-specific target
> description. I can make the get_thread_tdesc method always return a
> valid target description if you think it's better that way.

Either way works.

>>> And get_thread_tdesc would just return that (in fact,
>>> get_thread_tdesc might not be necessary then). Perhaps it makes some
>>> things more complicated down the road, but I can't think of anything.
> 
> Sorry, I don't understand this part. get_thread_tdesc is necessary
> because it's the hook that allows arch-specific code to provide a target
> description for the thread. I don't see how it can become unnecessary.
> 
> Perhaps you mean the get_thread_target_desc function? Sorry about the
> names being so similar, I spent some time trying to think of a better
> name for either the method or the function but failed.

Err yeah, I meant the free function that returns the process' tdesc if
the thread doesn't have one.

> In any case, it wouldn't be possible to make get_thread_target_desc just
> return thread_info->tdesc because at least the way these patches are
> currently written, when the inferior starts or a new thread of the
> inferior is spawned thread_info->tdesc is nullptr. gdbserver will only
> call get_thread_tdesc after the first stop (in get_thread_regcache, in
> the process of obtaining the pc register), so we will need to cope with
> that situation.

Ok.  Would it work if a new thread initially inherited the tdesc from
its process?

>> Sounds reasonable.
>>
>> Moving towards thread-specific target descriptions/gdbarch would be a positive thing given
>> the SVE precedent. The process-wide target description/gdbarch no
>> longer reflects the correct settings for each thread on AArch64's with SVE support.
> 
> In the first version of these patches I removed the process-wide target
> description and moved it to thread_info, but it was a big patch that
> touched many targets. I can bring it back if you think it's worth it.

At least for the register description, if we decide it's now a
per-thread thing, what does it mean to have a process-wide description
anyway?  I think it would make sense to get rid of it, that could help
confirm our model works (and it would remove the chance of  using the
wrong tdesc by mistake for a thread that has its own tdesc).  It's
probably something that can be done incrementally though.

Simon

  reply	other threads:[~2023-02-02  3:47 UTC|newest]

Thread overview: 94+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-30  4:45 [PATCH v3 0/8] gdbserver improvements for AArch64 SVE support Thiago Jung Bauermann
2023-01-30  4:45 ` [PATCH v3 1/8] gdbserver: Add assert in find_register_by_number Thiago Jung Bauermann
2023-01-31 17:05   ` Simon Marchi
2023-01-31 19:49     ` Thiago Jung Bauermann
2023-02-01 15:43       ` Simon Marchi
2023-01-30  4:45 ` [PATCH v3 2/8] gdbserver: Add PID parameter to linux_get_auxv and linux_get_hwcap Thiago Jung Bauermann
2023-02-01  9:07   ` Luis Machado
2023-02-01 10:54   ` Andrew Burgess
2023-02-01 16:01     ` Simon Marchi
2023-02-01 19:33       ` Thiago Jung Bauermann
2023-02-01 19:53         ` Simon Marchi
2023-02-01 21:55           ` Thiago Jung Bauermann
2023-02-06 19:54   ` Pedro Alves
2023-02-06 20:16     ` Simon Marchi
2023-02-07 15:19       ` Pedro Alves
2023-02-07 21:47         ` Thiago Jung Bauermann
2023-02-09  1:31           ` Simon Marchi
2023-02-10  3:54             ` Thiago Jung Bauermann
2023-02-07 22:28     ` Thiago Jung Bauermann
2023-01-30  4:45 ` [PATCH v3 3/8] gdbserver/linux-aarch64: Factor out function to get aarch64_features Thiago Jung Bauermann
2023-02-01  8:59   ` Luis Machado
2023-02-01 16:04     ` Simon Marchi
2023-02-01 22:13       ` Thiago Jung Bauermann
2023-01-30  4:45 ` [PATCH v3 4/8] gdbserver/linux-aarch64: When thread stops, update its target description Thiago Jung Bauermann
2023-02-01  9:05   ` Luis Machado
2023-02-01 11:06   ` Andrew Burgess
2023-02-01 16:21     ` Simon Marchi
2023-02-01 16:32       ` Luis Machado
2023-02-02  2:54         ` Thiago Jung Bauermann
2023-02-02  3:47           ` Simon Marchi [this message]
2023-02-03  3:47             ` Thiago Jung Bauermann
2023-02-03 11:13               ` Luis Machado
2023-02-04 15:26                 ` Thiago Jung Bauermann
2023-02-03 11:11             ` Luis Machado
2023-02-04 15:21               ` Thiago Jung Bauermann
2023-02-06  9:07                 ` Luis Machado
2023-02-06 12:15                   ` Thiago Jung Bauermann
2023-02-06 20:29                 ` Pedro Alves
2023-02-07  8:11                   ` Luis Machado
2023-02-07 14:39                     ` Thiago Jung Bauermann
2023-02-03 10:57           ` Luis Machado
2023-02-04  6:18             ` Thiago Jung Bauermann
2023-02-06 20:26           ` Pedro Alves
2023-02-07 21:06             ` Thiago Jung Bauermann
2023-02-09  2:46               ` Simon Marchi
2023-02-10  3:29                 ` Thiago Jung Bauermann
2023-02-10 14:56                   ` Luis Machado
2023-02-10 15:04                     ` Tom Tromey
2023-02-10 15:28                       ` Luis Machado
2023-02-10 17:26                       ` Thiago Jung Bauermann
2023-02-10 21:01                         ` Simon Marchi
2023-01-30  4:45 ` [PATCH v3 5/8] gdbserver: Transmit target description ID in thread list and stop reply Thiago Jung Bauermann
2023-01-30 12:52   ` Eli Zaretskii
2023-01-30 14:05     ` Thiago Jung Bauermann
2023-02-01  9:39   ` Luis Machado
2023-02-01 12:07   ` Andrew Burgess
2023-02-01 13:03     ` Eli Zaretskii
2023-02-01 17:37     ` Simon Marchi
2023-02-02 20:36       ` Thiago Jung Bauermann
2023-02-02 20:56         ` Simon Marchi
2023-02-01 20:46     ` Simon Marchi
2023-02-02 21:43       ` Thiago Jung Bauermann
2023-02-01 14:51   ` Andrew Burgess
2023-02-01 17:03     ` Simon Marchi
2023-02-02 19:52       ` Thiago Jung Bauermann
2023-02-02 20:51         ` Simon Marchi
2023-02-03  2:44           ` Thiago Jung Bauermann
2023-02-03 16:29             ` Andrew Burgess
2023-02-04  6:08               ` Thiago Jung Bauermann
2023-02-03 11:22       ` Luis Machado
2023-02-03 12:50         ` Simon Marchi
2023-01-30  4:45 ` [PATCH v3 6/8] gdb/remote: Parse tdesc field in stop reply and threads list XML Thiago Jung Bauermann
2023-02-01  9:52   ` Luis Machado
2023-02-05  0:06     ` Thiago Jung Bauermann
2023-02-06  9:10       ` Luis Machado
2023-02-01 14:32   ` Andrew Burgess
2023-02-01 19:50     ` Simon Marchi
2023-02-01 20:16       ` Simon Marchi
2023-02-03 11:27         ` Luis Machado
2023-02-03 13:19           ` Simon Marchi
2023-02-03 16:33             ` Andrew Burgess
2023-01-30  4:45 ` [PATCH v3 7/8] gdb/aarch64: Detect vector length changes when debugging remotely Thiago Jung Bauermann
2023-02-01  9:58   ` Luis Machado
2023-02-01 15:26   ` Andrew Burgess
2023-02-01 20:20     ` Simon Marchi
2023-02-03 11:31       ` Luis Machado
2023-02-03 16:38       ` Andrew Burgess
2023-02-03 19:07         ` Simon Marchi
2023-01-30  4:45 ` [PATCH v3 8/8] gdb/testsuite: Add test to exercise multi-threaded AArch64 SVE inferiors Thiago Jung Bauermann
2023-02-01 10:10   ` Luis Machado
2023-02-06 19:11 ` [PATCH v3 0/8] gdbserver improvements for AArch64 SVE support Pedro Alves
2023-02-06 20:05   ` Simon Marchi
2023-02-06 21:06     ` Pedro Alves
2023-02-07 13:49       ` Simon Marchi

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=e6c0ec52-d8d3-decf-7363-8e7d81e1f882@simark.ca \
    --to=simark@simark.ca \
    --cc=aburgess@redhat.com \
    --cc=gdb-patches@sourceware.org \
    --cc=luis.machado@arm.com \
    --cc=thiago.bauermann@linaro.org \
    /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).