From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f41.google.com (mail-wm1-f41.google.com [209.85.128.41]) by sourceware.org (Postfix) with ESMTPS id 293EF3858D35 for ; Mon, 6 Feb 2023 20:26:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 293EF3858D35 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wm1-f41.google.com with SMTP id hn2-20020a05600ca38200b003dc5cb96d46so11709265wmb.4 for ; Mon, 06 Feb 2023 12:26:40 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:content-language:in-reply-to:mime-version :user-agent:date:message-id:from:references:cc:to:subject :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=l3pGZSj1i67DEhbmFz6zpMqRcKXTH7px3hCp21YupTY=; b=uYcJzuBXFyK1ccB9FMKRnlp2VgIGg+fPl5LluuBS5WVuF5W2uFqprzyzaWLULUXM/9 3YKVi80RJFVuyDOp5SWjbLqcqBE2GglxTLAHREmnGHIil2Hn1GjtBTtK9NwNfKn3cgoP JWFGLOnyRsHs3Upky4V8Ua+xPa7fHjmDN6yQg1DzFkB2NoA+pzZWcna8JgOVV0M46DlB T5fZuHFcQgDxni9SdDT2+VRxoqcpDubKoQ/qMZLncaeNktNLU9VfMYUHEw7O4D9C1gAb DB3mP7tASf/+mGkvrG2sQWHNGw5XIw7nUhNr6/kObZRiNCU4gDD6HfaRE+81hJOcjqm7 +eaA== X-Gm-Message-State: AO0yUKXoU70LUEKzj5V3GxZ/NOWJRfZ2HL5puNKSx1KULinxFVSqJiLP wjtWQcrtF6eAoYK4jKGknmQ7VsHQEDjOpw== X-Google-Smtp-Source: AK7set8HfGPYbV/6npZ19dhV51txi7JZBvT67f3Df7MYqpSl9VJStmPtCqMghCq99Y0nFqMSfnxHCw== X-Received: by 2002:a05:600c:3420:b0:3dc:4548:abe6 with SMTP id y32-20020a05600c342000b003dc4548abe6mr919966wmp.12.1675715198721; Mon, 06 Feb 2023 12:26:38 -0800 (PST) Received: from ?IPv6:2001:8a0:f92b:9e00::1fe? ([2001:8a0:f92b:9e00::1fe]) by smtp.gmail.com with ESMTPSA id w14-20020a05600c474e00b003dfe57f6f61sm12068125wmo.33.2023.02.06.12.26.37 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 06 Feb 2023 12:26:38 -0800 (PST) Subject: Re: [PATCH v3 4/8] gdbserver/linux-aarch64: When thread stops, update its target description To: Thiago Jung Bauermann , Luis Machado Cc: Simon Marchi , Andrew Burgess , Thiago Jung Bauermann via Gdb-patches 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> From: Pedro Alves Message-ID: <3f4e3603-59e3-a896-72e4-d692646c4e44@palves.net> Date: Mon, 6 Feb 2023 20:26:32 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: <87y1pgaib6.fsf@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.7 required=5.0 tests=BAYES_00,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,GIT_PATCH_0,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,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: 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. FWIW, I have the same thought as Simon while reading this. > > 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. > >