From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ot1-x335.google.com (mail-ot1-x335.google.com [IPv6:2607:f8b0:4864:20::335]) by sourceware.org (Postfix) with ESMTPS id 9C15B3858D3C for ; Thu, 2 Feb 2023 02:54:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 9C15B3858D3C Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org Received: by mail-ot1-x335.google.com with SMTP id k91-20020a9d19e4000000b0068bca1294aaso146592otk.8 for ; Wed, 01 Feb 2023 18:54:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:message-id:date:in-reply-to:subject:cc:to:from :user-agent:references:from:to:cc:subject:date:message-id:reply-to; bh=lzUUDDnT/1hOaZgzPLOrctcG+ejaum4PRDex5TS5Ofo=; b=BScRbUs6XpM2hQ3Rdefn3rCsEyHkwlb4J3bdG39xh1aVNKoCpa1SLDm0d8Ox1ag8NP LVohREtBnuzxLreTYhUkMcpLOwEVwMwZN8MGsgK1jUsBRl3ZJP1FPZ35PFRVmXmKlNXz CpCDNG2buxL0eECk1RL5uybvUVHZCVH1EZ/2nRUZtHVZmYxqbGsqd662Uw0fUB+ZYF7b UnXUYsM6LTtz3xwG3hREBclhNA9mIXXR81CaGe8WqKf1y7w9CCEY/S7de7kSDLe8cl4B RBeDSnUNOHmUbQYBhLVvz08RXk4ycNYBoY/4Cnd5i/F+v7tYqjSj82v0njLx1wV3OflI K7eQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:message-id:date:in-reply-to:subject:cc:to:from :user-agent:references:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=lzUUDDnT/1hOaZgzPLOrctcG+ejaum4PRDex5TS5Ofo=; b=a5dR1gSRx6QnNrbzC5uRRBoqGhLDbHJKf1pBIyQs5gKHgveAUXmoLKSdailrnN5z3Q Ig04Fb7daO17+WEHhzMyvOf2suTrPIrdVnF2EWGzruC+cNuxjCrvh6pyto8GIEX6/Ees j6jnuAMH2oRJjUXBAvq0y3ZBGXKNsC9hFwUn0q7vAbqhUfT2T99lvBvA5qfPZ/oOFWsP Ez2ey9iQuIfY1+IBUOfE3YAdQkFy38vj0ukpydwzzSu/Tp9GjXmAxRk4ju9kucLjkB+q Qq4ESTKRf8Cfwdo4JfrldHSuBOaBJLP36p2sBkTYcZQjn52TghH0Df/kv4zXPFs3gck/ tmfA== X-Gm-Message-State: AO0yUKUWPUlNnwKpll1iGpRW+FXttSWnAQLoGOy2swkeE+vqI6Xm8drW VeoFlZqwI0GBqLnQ7M8tHtxylw== X-Google-Smtp-Source: AK7set+zA6DWpsenxkK6wJowh0upVCOaRbsoc7VduYlpvqSvVQFuSH63tEqa2FlM+i4BSEQuIvbJAw== X-Received: by 2002:a05:6830:34a8:b0:670:8cc8:3a00 with SMTP id c40-20020a05683034a800b006708cc83a00mr3021216otu.19.1675306455364; Wed, 01 Feb 2023 18:54:15 -0800 (PST) Received: from localhost ([2804:14d:7e39:8470:5ff6:b7dc:1f9a:3db3]) by smtp.gmail.com with ESMTPSA id c20-20020a9d6854000000b006864ccdb053sm6288711oto.26.2023.02.01.18.54.14 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 01 Feb 2023 18:54:14 -0800 (PST) References: <20230130044518.3322695-1-thiago.bauermann@linaro.org> <20230130044518.3322695-5-thiago.bauermann@linaro.org> <87pmattzjw.fsf@redhat.com> <7970ac03-1123-d5f6-7b17-808832d43be6@simark.ca> <9a85e2fe-078a-e2ee-7e49-53fe0ceef492@arm.com> User-agent: mu4e 1.8.13; emacs 28.2 From: Thiago Jung Bauermann To: Luis Machado Cc: Simon Marchi , Andrew Burgess , Thiago Jung Bauermann via Gdb-patches Subject: Re: [PATCH v3 4/8] gdbserver/linux-aarch64: When thread stops, update its target description In-reply-to: <9a85e2fe-078a-e2ee-7e49-53fe0ceef492@arm.com> Date: Thu, 02 Feb 2023 02:54:05 +0000 Message-ID: <87y1pgaib6.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-12.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Luis Machado 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. >> 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. 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. > 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. -- Thiago