From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oa1-x35.google.com (mail-oa1-x35.google.com [IPv6:2001:4860:4864:20::35]) by sourceware.org (Postfix) with ESMTPS id A73913858C5E for ; Fri, 3 Feb 2023 03:47:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A73913858C5E 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-x35.google.com with SMTP id 586e51a60fabf-15fe106c7c7so5245621fac.8 for ; Thu, 02 Feb 2023 19:47:35 -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=vEMDaWxVFSOOsfUeRLvrAawO1CqtYvqhVf7DHewj3eQ=; b=l5mvMgDdrv71LJxks3oKwLImRNfLYsRRRZKn9PmmRCRI0oAb6ow3ep2XJDE33tLwsS aEMVesoB9H1lpjh914VPrcwsMhI4VxCgG0MdjAMirXs48Ju1Gi2955C75V6S+EVP9AG4 5EjObHeD3NZhgsymFvZZ2FOrWOKvG+1IxvCCLWfgOK+Sy+eifL774zLG1UagBz7FPvag lqRYFJnAai3glAijvSB2d/gFOHJr5UKoydoPa3kNX9nua62p6uquiULiIfAQXMlCTlTF vDOGxRb+QWSxdWg5nbxSppEkSl4sX4ooPSOnxOELva70dAAbU1tgXTx2q4lSruBEmvLD 10Kw== 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=vEMDaWxVFSOOsfUeRLvrAawO1CqtYvqhVf7DHewj3eQ=; b=yT9Q4bVOyDeUJES1XYK/f06N6iDl+NvYhcoWR1Ku4CVoYfOJnrVCf6mT1Z5paRSnZV w2usFwfzdfnSs81zG6B5wlw4u2mNmegeNA2caYZI30dNLh0zpm1ZZoQbo5DcRVzhPIfL e9QIm4XNFJ5V1L3yqLfODy7XiSTAJPNv7Mo77vAIZyHoI3N4rw7Rz73IGJ/t1yoXmbOz GUVoXLlzW+6sCotVk7WyBFSvWtvv0jmSACy8PyZCw588Z27JycdWZMLqaVH/MRaJKnip ygIby1DIL3KRTch70sgHjBcnPGoBy3NGbrkLq78i4HohywCVWE2SjsONeW296A8c+Wcz Pv/A== X-Gm-Message-State: AO0yUKXJZpOepZ+OYfidLi136T8fFa+cFYgyjIOmD3Z/RMqbg1Qm31qj OTHNVpk0DDjyaSUAHDOB8d5Pm2gSHvzlfMb5 X-Google-Smtp-Source: AK7set9Ah6X4NIDzS8RLWi66gOBzETtmDfN5G7UCKWOpqzHBjyyvcwIOdFYpEpxknPFHZXjjQeowjA== X-Received: by 2002:a05:6870:6082:b0:163:b35e:40d3 with SMTP id t2-20020a056870608200b00163b35e40d3mr3904804oae.17.1675396054809; Thu, 02 Feb 2023 19:47:34 -0800 (PST) Received: from localhost ([2804:14d:7e39:8470:7132:fbe:2b2e:ae3e]) by smtp.gmail.com with ESMTPSA id h5-20020a056870538500b0015f9cc16ef7sm482519oan.46.2023.02.02.19.47.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 02 Feb 2023 19:47:34 -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> User-agent: mu4e 1.8.13; emacs 28.2 From: Thiago Jung Bauermann To: Simon Marchi Cc: Luis Machado , 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: Date: Fri, 03 Feb 2023 03:47:31 +0000 Message-ID: <871qn79zqk.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-10.6 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: Simon Marchi writes: > On 2/1/23 21: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. > > Either way works. Ok, if there's no preference I suggest leaving it as it is now (unless we decide moving away from process_info->tdesc). >>>> 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. > > Err yeah, I meant the free function that returns the process' tdesc if > the thread doesn't have one. > >> 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. > > Ok. Would it work if a new thread initially inherited the tdesc from > its process? Yes, it would. Version 1 of these patches didn't work exactly like that because I removed process_info->tdesc, but the new thread inherited from the parent thread's tdesc. This happened in linux_process_target::handle_extended_wait where the clone and fork events are handled. >>> 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. > > At least for the register description, if we decide it's now a > per-thread thing, what does it mean to have a process-wide description > anyway? I think it would make sense to get rid of it, that could help > confirm our model works (and it would remove the chance of using the > wrong tdesc by mistake for a thread that has its own tdesc). It's > probably something that can be done incrementally though. Would it be done incrementally by keeping process_info->tdesc but changing each target in turn to use thread_info->tdesc and ignore the process_info one? -- Thiago