From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id wJ2OLHe94mNqiiwAWB0awg (envelope-from ) for ; Tue, 07 Feb 2023 16:07:03 -0500 Received: by simark.ca (Postfix, from userid 112) id B18C41E15D; Tue, 7 Feb 2023 16:07:03 -0500 (EST) Authentication-Results: simark.ca; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.a=rsa-sha256 header.s=default header.b=oTMfSNpX; dkim-atps=neutral X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED, RDNS_DYNAMIC,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.6 Received: from sourceware.org (ip-8-43-85-97.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 120A41E0D3 for ; Tue, 7 Feb 2023 16:07:03 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id F3EE03858410 for ; Tue, 7 Feb 2023 21:07:01 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org F3EE03858410 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1675804022; bh=EyIYBd8xFDFFPiST9IDpzJPuomA3OX7ULplp5ChCYnI=; h=References:To:Cc:Subject:In-reply-to:Date:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=oTMfSNpXFkNe1+PpG7KICtzTak7ELz6rycRyZ4Ri7q1eY3camRrso3bGX95U3tTxv GYvgeveyFIv3EYbptMn+xanQLaHNH7dZfMWQcbsnsayiVuQ0qDj6PJ7s5dm1PnUlnA scVjBAErTY1q3CsmBRgMZC4HZnBEIEA9WP/MjCmo= 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 Received: by mail-oa1-x29.google.com with SMTP id 586e51a60fabf-15f97c478a8so20603836fac.13 for ; Tue, 07 Feb 2023 13:06:38 -0800 (PST) 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 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-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Thiago Jung Bauermann via Gdb-patches Reply-To: Thiago Jung Bauermann Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" 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