From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x236.google.com (mail-oi1-x236.google.com [IPv6:2607:f8b0:4864:20::236]) by sourceware.org (Postfix) with ESMTPS id 59EDE3858D1E for ; Tue, 29 Nov 2022 04:30:04 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 59EDE3858D1E 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-oi1-x236.google.com with SMTP id r76so13940094oie.13 for ; Mon, 28 Nov 2022 20:30:04 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=content-transfer-encoding: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=sMMs6JA7DpfPtk1HFM4ZFP/rgxlvDrPmWCWkADbjUCY=; b=IaOormWdu3xOdzS9tjKXPo/zfo4K1fvyc0kDX7ffWF4Yhpf751To9506zVkYNXtvX1 Rh7tgyYk3PJRKxR2oSzD2xunzzuDKd/ezclabinDR2SwPA2WOgKMzOnz/XXAF8Eln2T5 GgPjAJ/ve6glWDY2IonnHjNxPYFgDBuJErsPV2aIGS1d9rzn2VBAPZkxnKsVXV1KFcCG YsQ2uL7a6npVpN90Q2BdUxb7murL9vAUPqem/YAuEr2PgAZ2MkpEMF0KdZEkWPmfBAsA P5M/rDt/9u+alnlAm/PS4j0sCpak8YkUdwNdI1fbKbGyaF+oezQB9F+eceUXRSVgObwN yr7g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding: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=sMMs6JA7DpfPtk1HFM4ZFP/rgxlvDrPmWCWkADbjUCY=; b=5pJ+6Nsoa+tXdaKRmgcEDFBzzkInp/qlkmNyygcJTLkKB+JueXtCIJuwI4/b5JS0dC ynk+preITXvBFy1DDIIUnRHQtEcO+DeqINOeF3OH9qq7km3UnE9zX4k2rw1IJ0zAtAD7 CW/RGbc/KvsSHTK71ka3Vtzuw6GbcKv0cYDn0mqWqiodB7t0YBvYQDRBm6fwJMKzMuIq 3eQa2rVdOQn0sYfsH9iP23lvx5U1L0Lfm+tY6wpHUdeCAAjujKm23++WoSkzVhrA4q5Z texWxJJyKAHLmtOF+Xcpon3fu4i361MHvGswRGpXCJ7qWQXYEPxD3p1UQ82WtmeXMAP0 4pDA== X-Gm-Message-State: ANoB5pnIO/vqYazh6BwRADQgkeMpMmIO/CJkPgodXkdIICS8vRn9aZq+ eOv2b13aM8ia0AQJ92TNFoa61g== X-Google-Smtp-Source: AA0mqf7ETkkzk2vTeEU0HOXfDiScQCrSAfeURh5Ra20AnrZnDTkH7EJ1qElo73W0OTdE7d3kdMqMag== X-Received: by 2002:a05:6808:1293:b0:359:dc34:5b5e with SMTP id a19-20020a056808129300b00359dc345b5emr28301031oiw.259.1669696203665; Mon, 28 Nov 2022 20:30:03 -0800 (PST) Received: from localhost ([2804:14d:7e39:8470:201c:ea26:41ce:5eb]) by smtp.gmail.com with ESMTPSA id w23-20020a056830281700b0066c4092ae4csm5533335otu.10.2022.11.28.20.30.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 28 Nov 2022 20:30:03 -0800 (PST) References: <20221126020452.1686509-1-thiago.bauermann@linaro.org> <20221126020452.1686509-5-thiago.bauermann@linaro.org> <3539acb1-462d-62e3-5a70-40786942c325@simark.ca> User-agent: mu4e 1.8.11; emacs 28.2 From: Thiago Jung Bauermann To: Simon Marchi Cc: gdb-patches@sourceware.org, Luis Machado Subject: Re: [PATCH v2 4/6] gdbserver/linux-aarch64: When thread stops, update its target description In-reply-to: <3539acb1-462d-62e3-5a70-40786942c325@simark.ca> Date: Tue, 29 Nov 2022 04:30:00 +0000 Message-ID: <87bkoqgz7b.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-11.4 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: >> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-lo= w.cc >> index cab4fc0a4674..786ce4071279 100644 >> --- a/gdbserver/linux-aarch64-low.cc >> +++ b/gdbserver/linux-aarch64-low.cc >> @@ -99,6 +99,9 @@ protected: >>=20=20 >> void low_arch_setup () override; >>=20=20 >> + gdb::optional >> + arch_update_tdesc (const thread_info *thread) override; > > I'm all for using optional to be clear and explicit in general, but > unless an empty optional and an optional wrapping nullptr are both valid > and have different meanings (which doesn't seem, to be the case here), I > wouldn't recommend wrapping a pointer in an optional. > > Pointers have a dedicated value for "no value", that is well understood > by everyone. And then, it does create the possibility of returning an > optional wrapping nullptr, which typically won't be a legitimate value. > So it's just one more thing to worry about. I like optional because it forces the caller to check that the result is valid before using it, whereas with an unwrapped pointer it's easy to accidentally do a null pointer dereference. I hadn't considered the situation of an optional wrapping a nullptr though. Interesting point. For v3 I changed the function to return an unwrapped pointer and check for nullptr in the caller. >> @@ -840,8 +845,16 @@ aarch64_target::low_arch_setup () >> { >> struct aarch64_features features >> =3D aarch64_get_arch_features (current_thread); >> + const target_desc *tdesc =3D aarch64_linux_read_description (feat= ures); >>=20=20 >> - current_process ()->tdesc =3D aarch64_linux_read_description (fea= tures); >> + /* Only SVE-enabled inferiors need per-thread target descriptions= . */ >> + if (features.vq > 0) >> + { >> + current_thread->tdesc =3D tdesc; >> + current_process ()->priv->arch_private->has_sve =3D true; >> + } > > Is it not possible for a process to start with SVE disabled (vq =3D=3D 0)= and > have some threads enable it later? Thank you Luis for clarifying this point. >> @@ -853,6 +866,28 @@ aarch64_target::low_arch_setup () >> aarch64_linux_get_debug_reg_capacity (lwpid_of (current_thread)); >> } >>=20=20 >> +/* Implementation of linux target ops method "arch_update_tdesc". */ >> + >> +gdb::optional >> +aarch64_target::arch_update_tdesc (const thread_info *thread) >> +{ >> + /* Only processes using SVE need to update the thread's target descri= ption. */ >> + if (!get_thread_process (thread)->priv->arch_private->has_sve) >> + return {}; >> + >> + const struct aarch64_features features =3D aarch64_get_arch_features = (thread); >> + const struct target_desc *tdesc =3D aarch64_linux_read_description (f= eatures); >> + >> + if (tdesc =3D=3D thread->tdesc) >> + return {}; >> + >> + /* Adjust the register sets we should use for this particular set of >> + features. */ >> + aarch64_adjust_register_sets(features); >> + >> + return tdesc; > > Naming nit: I don't think we need "update" in the method name, there's > no "updating component" to it AFAICT. It's basically "get this thread's > tdesc if for some reason it differents from the process-wide we have". > So, I don't know, "get_thread_tdesc" or just "thread_tdesc". That's true. I renamed the method to =E2=80=9Cget_thread_tdesc=E2=80=9D. Th= anks for the suggestion. >> @@ -2348,6 +2354,18 @@ linux_process_target::filter_event (int lwpid, in= t wstat) >> return; >> } >> } >> + else >> + { >> + /* Give the arch code an opportunity to update the thread's target >> + description. */ >> + gdb::optional new_tdesc >> + =3D arch_update_tdesc (thread); >> + if (new_tdesc.has_value ()) >> + { >> + regcache_release (); > > Hmm, regcache_release frees the regcache for all threads. Can we free > the regcache only for this thread? Indeed. regcache_release simply calls the static function =E2=80=9Cfree_register_cache_thread=E2=80=9D on all threads, so for v3 I ma= de it public and called it just for this thread. --=20 Thiago