From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oa1-x2f.google.com (mail-oa1-x2f.google.com [IPv6:2001:4860:4864:20::2f]) by sourceware.org (Postfix) with ESMTPS id DC3373858C52 for ; Sat, 4 Feb 2023 06:18:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DC3373858C52 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-oa1-x2f.google.com with SMTP id 586e51a60fabf-1636eae256cso9253783fac.0 for ; Fri, 03 Feb 2023 22:18:48 -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=YGJ7QHMlRtITalF6LzkxD8n54GNBBI+BK0jID6nhHic=; b=ITyivT9RIogjxt4OlUzOCXqQ+DNu5jsdmaDRKfYTeUOl9ZmdHooYvb96JXFGhy/FeL OchBkwpgbSi7+CO39AauLaIeho7okKZE2V6Lvg21MDyLft49p/bHeQLmHSB2SwRtWgZT BqhYK8qwym6CNL7Gs5okYvmmceh/7EpdXVBpmbn3tEpLsToH/GXtHE8Vl2/dHF5fFiey 4TdpXotsdk/f0QFqT26cpgOGRFgYtER4YasbeqEamdDKHvprV45VWXDMtS/kLXLIWCU8 nBQe8g0+hRflihDtZGSiAhfuu71l9sMaMBfmpB1IWXHsCeOawwf31w+dzQ6haO8XbL+v v6DQ== 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=YGJ7QHMlRtITalF6LzkxD8n54GNBBI+BK0jID6nhHic=; b=csM+K3t2lYIQF5U7L5DU0k8ydgqmKjvCbI15cLW0t/J2/svV1mpeaC0s1MyHTgr2RL +3DOgGs5DQJbYbPd8akyRPSfBAOf6zAryAg8dzCICNgySqNSSwrDfSWxGeCCw0m4Xhow nvKPPwZavevkPFPr1W2dtfiANSK/jBOxFXCABhIaE/IchvyZITyN9kJwWwkWnQYpd6QP 3FJEbk++DOn3Fj+jcBIvQ9Uk9GQAWMExr6Kl9glxh6v0Vv5q2lohub0QaTrDeA+Yki47 s0aEJDIch6IstGNYjRZuZcA2y4JjQv2Fc7yVeW87YYzkdaZ6Q7RYGvnmn7ECM44jzVrV ESpQ== X-Gm-Message-State: AO0yUKWEF3/S4NV08oDHrJWa1b6px3N1P1CQ1DfV3pIShrXJgs4VOLXi qUgXgZz8WI63EUKWFw7hfuLA3w== X-Google-Smtp-Source: AK7set9FgifdoGd82m/6FAagzOsqihk99ig8MZU5tDH4uaKYp+l82/NVTBNlol8YT2Du+RlBnsle5A== X-Received: by 2002:a05:6870:b60b:b0:163:c089:490f with SMTP id cm11-20020a056870b60b00b00163c089490fmr5834504oab.0.1675491527989; Fri, 03 Feb 2023 22:18:47 -0800 (PST) Received: from localhost ([189.100.172.221]) by smtp.gmail.com with ESMTPSA id a4-20020a056870618400b0014fc049fc0asm1553134oah.57.2023.02.03.22.18.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 03 Feb 2023 22:18:47 -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> <87y1pgaib6.fsf@linaro.org> <7aa9cb50-7353-e9e5-6812-7615aaad6f93@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: <7aa9cb50-7353-e9e5-6812-7615aaad6f93@arm.com> Date: Sat, 04 Feb 2023 06:18:44 +0000 Message-ID: <87pmaqlzqz.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-10.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_BARRACUDACENTRAL,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/2/23 02:54, Thiago Jung Bauermann wrote: >> 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 > > No need. As Simon suggested, we can do this incrementally. I don't think we should hold > off on > this series' particular improvements so we can get the more general case sorted. Sounds good to me. I'll be glad to work on a follow-up series with these improvements after this one is done. -- Thiago