From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oa1-x29.google.com (mail-oa1-x29.google.com [IPv6:2001:4860:4864:20::29]) by sourceware.org (Postfix) with ESMTPS id A5DE93858D35 for ; Tue, 7 Feb 2023 21:06:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A5DE93858D35 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-x29.google.com with SMTP id 586e51a60fabf-15f97c478a8so20603836fac.13 for ; Tue, 07 Feb 2023 13:06:38 -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=EyIYBd8xFDFFPiST9IDpzJPuomA3OX7ULplp5ChCYnI=; b=aAdvnGYRbgUMsrpWcXdf1j1k+V69fZumwk9C2lKYubEtZ15+4kmYHDoZZxXgnY89o3 yosEQfRsGbDRJTOHDZ7eXgBJ8cWnXUeoV+IyoYGtbERKURRJ8pufW0U5XtQP9lXaL2X1 24tSfyvm8241PC5yREaoiRjyqMpH+xBVPaXi8naVRMu3tVk4rgy7QsI3FvOMNGfD267t 7872zhcbl46UzrzTVMALTIzsBqzWJH7/5IE5BBZiGvVXVwaA5HnNIpj/W0iat4KvH01u o1AN7ARBt4QC1KQfiVTzu+gBjP7F4rPXDVuAqEpsPKvlfFTpg5XuaT32yVpoMl02slnF velg== 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=EyIYBd8xFDFFPiST9IDpzJPuomA3OX7ULplp5ChCYnI=; b=LITVdlwZMA082t7F/ilEjjcN2t8byXlGVZOQBZsEG3+01HdvYNhWIa4uYs9RKUHsWa YMKU47EDgWdZVKoEd/hC8uQ3knJCUoA5pblrdcJsQVmLs0VJW4hiDiA990TVUeDHlk8g dFW/fGAAaUultbd7BWfljw0zZMQuK89Zf7nB1ZWiqTzBipCBLn+dd7J9+S5O01ZQTtIG B3mjpTaJEZn6LpwnGKBNUwU4cmnPEsL8ssC05GEN8xp6x3YVFhSUJjq/AS5OOa98Rwqc reME+47Su6mph6Lh/LsVCOPDFx1600eOYkNVg6wXS9hR8OXkCcUl5jmDtVzO81wOxtfP yg9w== X-Gm-Message-State: AO0yUKWBaToJhkqmtllyDOMOfPRWzmzjcj2HZkZSZ4nEQdpK76Am0hcT QpyCsPu3h6rQLjVMQu+8nH4GWQ== X-Google-Smtp-Source: AK7set9P3xZ96zWF4pxdrKB0Ys0edsbxwxKlj5KRpKN9r0NxnRMF/xlOK2WTCvJVWJlq3ZNrJGwKyw== X-Received: by 2002:a05:6870:d620:b0:163:c555:24e0 with SMTP id a32-20020a056870d62000b00163c55524e0mr2628715oaq.18.1675803997254; Tue, 07 Feb 2023 13:06:37 -0800 (PST) Received: from localhost ([2804:14d:7e39:8470:56ca:1532:909:af92]) by smtp.gmail.com with ESMTPSA id do20-20020a0568300e1400b0068bce6239a3sm3033501otb.38.2023.02.07.13.06.36 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 07 Feb 2023 13:06:36 -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> <3f4e3603-59e3-a896-72e4-d692646c4e44@palves.net> User-agent: mu4e 1.8.13; emacs 28.2 From: Thiago Jung Bauermann To: Pedro Alves Cc: Luis Machado , 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: <3f4e3603-59e3-a896-72e4-d692646c4e44@palves.net> Date: Tue, 07 Feb 2023 21:06:33 +0000 Message-ID: <87v8kd9odi.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-11.8 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: Pedro Alves writes: > On 2023-02-02 2:54 a.m., Thiago Jung Bauermann via Gdb-patches 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. > > Please drop the "get_" prefix from the class method, it doesn't really > add value, and we typically don't add it. Most GDB getter/setters are > in foo() / set_foo() pair style, rather than get_foo() / set_foo(). > > A "get_" prefix is however typically used for global getter functions. Ok, I will drop the prefix. Thank you for explaining the pattern. I wasn't aware of it. > FWIW, I have the same thought as Simon while reading this. Ok, I'll change the code to always store a tdesc in thread_info even if it's the same as the process-wide one. If both you and Simon thought along the same lines it may be a more intuitive mental model than the one I was considering. That is, assuming we continue with the thread-specific tdesc approach rather than the one which expands tdescs to allow describing one register's type in terms of another one. I'll revisit my notes and think more about this. -- Thiago