From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ot1-x32c.google.com (mail-ot1-x32c.google.com [IPv6:2607:f8b0:4864:20::32c]) by sourceware.org (Postfix) with ESMTPS id 247913858C5E for ; Mon, 24 Jul 2023 16:19:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 247913858C5E 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-ot1-x32c.google.com with SMTP id 46e09a7af769-6bb231f1817so908394a34.3 for ; Mon, 24 Jul 2023 09:19:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1690215571; x=1690820371; 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=qcLTqlecVMpcpulIqYPUcKAtzi1Jgbw2m6yYBD4kvew=; b=L+HeyAQ8DHZQlhK68v6DKE+YhnTXB/fB4Rfx+0YyOcNNPHYcecBaKMQCo7/LEB44FP IYG7OxXeYE3bNY8vpYerG7ZEAPcyaVXB6EAUJ6eV2HTOhHalJ0dhebmPSWRMmD76Cb90 slWH2Ie/uV6qVAomrcBGmCtT2nV//U1dpA5sk6pUIWX2jXCmUZoVU0EeWsqStpObNkTW mJvOQOnJeKj/5+DnxC7dUmgCfBNzeH9lNQJm40ejpOKm1PyR5tjflIYumjj3yQ2ynAk/ ywDYdYWeOyNgl54nfBQ2GVfy6sNzVxSfoyK7/dF9JfLXUSFFAuI5xa8HU3h567/lhXUA p+yw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1690215571; x=1690820371; 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=qcLTqlecVMpcpulIqYPUcKAtzi1Jgbw2m6yYBD4kvew=; b=jB9NFjzDdYDn85E9Ql+ibdFbqpGPDQ9hZStpuEJcBSQnXVM8MRaw2Q9x/6lBr007F6 Ff4A/Q2GD66B0JFB+RygamEsIDOt+HoHFLTQS3dIN7tNv+RACYupmGuIUabvTv1/ky+a 6MSp61JQw1paQllxUFUepb61KNKteNpiCaReO9cNdXxbCN2TmxTaammsBIsc3ZW4bkHB /CvySwQzD4vYkgNtAVgeyX12gMhouT2XHX+e3qKDZHQTUjzGDO/ee0yKfhB67CAp/sny AjbP8Ckw3M3GZF6Reg518EEIkL9/nVgu2W2kb3MDSeSYKJj2OWEkxiVf8n1FDxFpwODZ dtiw== X-Gm-Message-State: ABy/qLbcFEITCIxb9/UaUhtQoJelD67WXP6NabYHuRqM6zmScH3VFgLl eJzMXlWSaHAVViFQAB3L+N49jCuCmBNx36UA1k0= X-Google-Smtp-Source: APBJJlGwBCEo9TB4gD6vW/XI7URoySk5hN+bcGYH+N/Mz+bqnWiuHkDFvB06rguZyrLpaqjLmCiGdg== X-Received: by 2002:a05:6870:a113:b0:1ba:2c39:4cfd with SMTP id m19-20020a056870a11300b001ba2c394cfdmr9370350oae.19.1690215571290; Mon, 24 Jul 2023 09:19:31 -0700 (PDT) Received: from localhost ([2804:14d:7e39:8470:e1cb:a74f:236e:c03e]) by smtp.gmail.com with ESMTPSA id y12-20020a4a450c000000b005678320f1f2sm4515527ooa.13.2023.07.24.09.19.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 24 Jul 2023 09:19:30 -0700 (PDT) References: <20230630134616.1238105-1-luis.machado@arm.com> <20230630134616.1238105-4-luis.machado@arm.com> User-agent: mu4e 1.10.5; emacs 28.2 From: Thiago Jung Bauermann To: Luis Machado Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v3 03/16] [gdb/gdbserver] refactor: Simplify SVE interface to read/write registers In-reply-to: <20230630134616.1238105-4-luis.machado@arm.com> Date: Mon, 24 Jul 2023 13:19:28 -0300 Message-ID: <87bkg1jn8v.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-11.0 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,T_SCC_BODY_TEXT_LINE 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: Luis Machado via Gdb-patches writes: > This is a patch in preparation to upcoming patches enabling SME support. It > attempts to simplify the gdb/gdbserver shared interface used to read/write > SVE registers. > > Where the current code makes use of unique_ptr, allocating a new buffer by > hand and passing a buffer around, this patch makes that code use > gdb::byte_vector and passes a reference to this byte vector to the functions, > allowing the functions to have ready access to the size of the buffer. Nice! > It also shares a bit more code between gdb and gdbserver, in particular around > handling of ptrace get/set requests for SVE. Also nice! > diff --git a/gdb/nat/aarch64-scalable-linux-ptrace.c b/gdb/nat/aarch64-scalable-linux-ptrace.c > index cc43f510892..192eebcda19 100644 > --- a/gdb/nat/aarch64-scalable-linux-ptrace.c > +++ b/gdb/nat/aarch64-scalable-linux-ptrace.c > @@ -120,28 +120,44 @@ aarch64_sve_set_vq (int tid, struct reg_buffer_common *reg_buf) > > /* See nat/aarch64-scalable-linux-ptrace.h. */ > > -std::unique_ptr > -aarch64_sve_get_sveregs (int tid) > +gdb::byte_vector > +aarch64_fetch_sve_regset (int tid) > { > - struct iovec iovec; > uint64_t vq = aarch64_sve_get_vq (tid); > > if (vq == 0) > - perror_with_name (_("Unable to fetch SVE register header")); > + perror_with_name (_("Unable to fetch SVE vector length")); > > /* A ptrace call with NT_ARM_SVE will return a header followed by either a > dump of all the SVE and FP registers, or an fpsimd structure (identical to > the one returned by NT_FPREGSET) if the kernel has not yet executed any > SVE code. Make sure we allocate enough space for a full SVE dump. */ > > - iovec.iov_len = SVE_PT_SIZE (vq, SVE_PT_REGS_SVE); > - std::unique_ptr buf (new gdb_byte[iovec.iov_len]); > - iovec.iov_base = buf.get (); > + gdb::byte_vector sve_state (SVE_PT_SIZE (vq, SVE_PT_REGS_SVE), 0); > + > + struct iovec iovec; > + iovec.iov_base = sve_state.data (); > + iovec.iov_len = sve_state.size (); > > if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_SVE, &iovec) < 0) > perror_with_name (_("Unable to fetch SVE registers")); > > - return buf; > + return sve_state; > +} > + > +/* See nat/aarch64-scalable-linux-ptrace.h. */ > + > +bool This function always returns true, and its only caller doesn't use the return value. Should it return void instead? > +aarch64_store_sve_regset (int tid, const gdb::byte_vector &sve_state) > +{ > + struct iovec iovec; > + iovec.iov_base = (void *) sve_state.data (); Minor nit: is the cast necessary? The code looks cleaner without it. Also, it's not used in aarch64_fetch_sve_regset. > + else > + { > + /* Otherwise, reformat the fpsimd structure into a full SVE set, by > + expanding the V registers (working backwards so we don't splat > + registers before they are copied) and using zero for everything > + else. > + Note that enough space for a full SVE dump was originally allocated > + for base. */ > + > + header->flags |= SVE_PT_REGS_SVE; > + header->size = SVE_PT_SIZE (vq, SVE_PT_REGS_SVE); > + > + memcpy (base + SVE_PT_SVE_FPSR_OFFSET (vq), &fpsimd->fpsr, > + sizeof (uint32_t)); > + memcpy (base + SVE_PT_SVE_FPCR_OFFSET (vq), &fpsimd->fpcr, > + sizeof (uint32_t)); > + > + for (int i = AARCH64_SVE_Z_REGS_NUM; i >= 0 ; i--) Shouldn't i start from AARCH64_SVE_Z_REGS_NUM - 1? If this is correct, a comment explaining why would be helpful. -- Thiago