From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ot1-x32d.google.com (mail-ot1-x32d.google.com [IPv6:2607:f8b0:4864:20::32d]) by sourceware.org (Postfix) with ESMTPS id D67AE385801F for ; Mon, 17 Jan 2022 12:50:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D67AE385801F Received: by mail-ot1-x32d.google.com with SMTP id y11-20020a0568302a0b00b0059a54d66106so2387939otu.0 for ; Mon, 17 Jan 2022 04:50:26 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:subject:to:cc:references:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=P+jBlIaAZEs6Tb0qInjJ223/IQEV/8Ba6xV79+xoJBs=; b=ZAKKdRjsGCm4vUWTdoRpbcBrNvjF23p1wFVHq7/CHqWbf1+6ND0N1XxBiowNt0fxI6 ruQ7PrXV+4nt0qSRWEMjqxyjSCfLPb/yX8m2qmiXdoUT1ViUMCrpIR++NG9oszWZiWz8 GABlhY4MgTBvZ1xS5MDzX5gurzNm/PA7BPAg5jUi+RevuVjetKASlyVwLbTgVH28IcYA GtWOREBDWNlACP5PtsBFmx52xvkAxS5ZaYZMlri23MQkPQRZUvc5Eyda8bJwUJxTRYqv B3JHGVd0UyKNliOMJPWKQofvtsSZKREjv7P3DVfMnE0qYXB8niGD2eojM2wzOu9xyV/Y b2EQ== X-Gm-Message-State: AOAM531idcxYzhzmRrY/IErcEHj2CSBylEQCnUF6Reu2I/KXRR+bV33N DsAoWQPvyvAbHKH2B7VMhP0l7A== X-Google-Smtp-Source: ABdhPJxBT95XF4iSB/G971WeQuB68dLA+16keTC4f0UZY9d18yBx2wWoeUMbyE/Z5NlVRq6nSRCukw== X-Received: by 2002:a9d:7f0e:: with SMTP id j14mr4195075otq.305.1642423826212; Mon, 17 Jan 2022 04:50:26 -0800 (PST) Received: from ?IPv6:2804:7f0:4841:563b:30b1:6189:5c0d:a8d8? ([2804:7f0:4841:563b:30b1:6189:5c0d:a8d8]) by smtp.gmail.com with ESMTPSA id q65sm6503716oih.45.2022.01.17.04.50.24 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 17 Jan 2022 04:50:25 -0800 (PST) From: Luis Machado Subject: Re: [PATCH 2/5] gdb/arm: Define MSP and PSP registers for M-Profile To: Christophe Lyon , gdb-patches@sourceware.org Cc: torjborn.svensson@st.com References: <20220114163552.4107885-1-christophe.lyon@foss.st.com> <20220114163552.4107885-2-christophe.lyon@foss.st.com> Message-ID: <23335b01-d23d-ad3b-1691-f2c2b0e6307c@linaro.org> Date: Mon, 17 Jan 2022 09:50:23 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: <20220114163552.4107885-2-christophe.lyon@foss.st.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org 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: , X-List-Received-Date: Mon, 17 Jan 2022 12:50:28 -0000 On 1/14/22 1:35 PM, Christophe Lyon via Gdb-patches wrote: > This patch removes the hardcoded access to PSP in > arm_m_exception_cache() and relies on the definition with the XML > descriptions. > --- > gdb/arch/arm.h | 6 ++- > gdb/arm-tdep.c | 45 ++++++++------------- > gdb/features/arm/arm-m-profile-with-fpa.c | 2 + > gdb/features/arm/arm-m-profile-with-fpa.xml | 2 + > gdb/features/arm/arm-m-profile.c | 2 + > gdb/features/arm/arm-m-profile.xml | 2 + > 6 files changed, 28 insertions(+), 31 deletions(-) > > diff --git a/gdb/arch/arm.h b/gdb/arch/arm.h > index eabcb434f1f..638780011fc 100644 > --- a/gdb/arch/arm.h > +++ b/gdb/arch/arm.h > @@ -49,6 +49,8 @@ enum gdb_regnum { > ARM_D0_REGNUM, /* VFP double-precision registers. */ > ARM_D31_REGNUM = ARM_D0_REGNUM + 31, > ARM_FPSCR_REGNUM, > + ARM_MSP_REGNUM, /* Cortex-M Main Stack Pointer. */ > + ARM_PSP_REGNUM, /* Cortex-M Process Stack Pointer. */ Commit ae66a8f19ef6bf2dc7369cf26073f34ddf7c175b started moving things away from these hardcoded register numbers. So I'd rather do a runtime discovery of register numbers as opposed to having these. The above commit records some register set data in struct gdbarch_tdep: bool have_mve; /* Do we have a MVE extension? */ int mve_vpr_regnum; /* MVE VPR register number. */ int mve_pseudo_base; /* Number of the first MVE pseudo register. */ int mve_pseudo_count; /* Total number of MVE pseudo registers. */ So, for example, we'd record the numbers for both MSP and PSP. > > /* Other useful registers. */ > ARM_FP_REGNUM = 11, /* Frame register in ARM code, if used. */ > @@ -65,8 +67,8 @@ enum arm_register_counts { > ARM_NUM_ARG_REGS = 4, > /* Number of floating point argument registers. */ > ARM_NUM_FP_ARG_REGS = 4, > - /* Number of registers (old, defined as ARM_FPSCR_REGNUM + 1. */ > - ARM_NUM_REGS = ARM_FPSCR_REGNUM + 1 > + /* Number of registers (old, defined as ARM_PSP_REGNUM + 1. */ > + ARM_NUM_REGS = ARM_PSP_REGNUM + 1 > }; The above constant (ARM_NUM_REGS) is no longer representative of the full set of available registers. It is now used to initialize the number of registers before discovery and xml parsing. I'd like to see this numbering scheme replaced by a more dynamic mechanism, similar to how the MVE register set does. > > /* Enum describing the different kinds of breakpoints. */ > diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c > index 14ec0bc8f9e..b6a1deafad5 100644 > --- a/gdb/arm-tdep.c > +++ b/gdb/arm-tdep.c > @@ -3012,7 +3012,6 @@ arm_m_exception_cache (struct frame_info *this_frame) > enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); > struct arm_prologue_cache *cache; > CORE_ADDR lr; > - CORE_ADDR sp; > CORE_ADDR unwound_sp; > LONGEST xpsr; > uint32_t exc_return; > @@ -3028,7 +3027,6 @@ arm_m_exception_cache (struct frame_info *this_frame) > to the exception and if FPU is used (causing extended stack frame). */ > > lr = get_frame_register_unsigned (this_frame, ARM_LR_REGNUM); > - sp = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM); > > /* Check EXC_RETURN indicator bits. */ > exc_return = (((lr >> 28) & 0xf) == 0xf); > @@ -3037,37 +3035,13 @@ arm_m_exception_cache (struct frame_info *this_frame) > process_stack_used = ((lr & (1 << 2)) != 0); > if (exc_return && process_stack_used) > { > - /* Thread (process) stack used. > - Potentially this could be other register defined by target, but PSP > - can be considered a standard name for the "Process Stack Pointer". > - To be fully aware of system registers like MSP and PSP, these could > - be added to a separate XML arm-m-system-profile that is valid for > - ARMv6-M and ARMv7-M architectures. Also to be able to debug eg a > - corefile off-line, then these registers must be defined by GDB, > - and also be included in the corefile regsets. */ > - > - int psp_regnum = user_reg_map_name_to_regnum (gdbarch, "psp", -1); > - if (psp_regnum == -1) > - { > - /* Thread (process) stack could not be fetched, > - give warning and exit. */ > - > - warning (_("no PSP thread stack unwinding supported.")); > - > - /* Terminate any further stack unwinding by refer to self. */ > - cache->prev_sp = sp; > - return cache; > - } > - else > - { > - /* Thread (process) stack used, use PSP as SP. */ > - unwound_sp = get_frame_register_unsigned (this_frame, psp_regnum); > - } > + /* Thread (process) stack used, use PSP as SP. */ > + unwound_sp = get_frame_register_unsigned (this_frame, ARM_PSP_REGNUM); > } > else > { > /* Main stack used, use MSP as SP. */ > - unwound_sp = sp; > + unwound_sp = get_frame_register_unsigned (this_frame, ARM_MSP_REGNUM); > } > > /* The hardware saves eight 32-bit words, comprising xPSR, > @@ -9296,6 +9270,19 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) > if (!valid_p) > return NULL; > > + if (is_m) > + { > + feature = tdesc_find_feature (tdesc, > + "org.gnu.gdb.arm.m-system"); The above xml feature is not present in the xml files that were modified below. Is this feature being generated by some debugging stub/probe maybe? > + if (feature != NULL) > + { > + valid_p &= tdesc_numbered_register (feature, tdesc_data.get (), > + ARM_MSP_REGNUM, "msp"); > + valid_p &= tdesc_numbered_register (feature, tdesc_data.get (), > + ARM_PSP_REGNUM, "psp"); > + } > + } > + > feature = tdesc_find_feature (tdesc, > "org.gnu.gdb.arm.fpa"); > if (feature != NULL) > diff --git a/gdb/features/arm/arm-m-profile-with-fpa.c b/gdb/features/arm/arm-m-profile-with-fpa.c > index 2b7c78597bb..4afba856875 100644 > --- a/gdb/features/arm/arm-m-profile-with-fpa.c > +++ b/gdb/features/arm/arm-m-profile-with-fpa.c > @@ -35,5 +35,7 @@ create_feature_arm_arm_m_profile_with_fpa (struct target_desc *result, long regn > tdesc_create_reg (feature, "", regnum++, 1, NULL, 96, "arm_fpa_ext"); > tdesc_create_reg (feature, "", regnum++, 1, NULL, 32, "int"); > tdesc_create_reg (feature, "xpsr", regnum++, 1, NULL, 32, "int"); > + tdesc_create_reg (feature, "msp", regnum++, 1, NULL, 32, "data_ptr"); > + tdesc_create_reg (feature, "psp", regnum++, 1, NULL, 32, "data_ptr"); > return regnum; > } > diff --git a/gdb/features/arm/arm-m-profile-with-fpa.xml b/gdb/features/arm/arm-m-profile-with-fpa.xml > index fceeaea7177..ee796b549a2 100644 > --- a/gdb/features/arm/arm-m-profile-with-fpa.xml > +++ b/gdb/features/arm/arm-m-profile-with-fpa.xml > @@ -36,4 +36,6 @@ > > > > + > + In this xml... > > diff --git a/gdb/features/arm/arm-m-profile.c b/gdb/features/arm/arm-m-profile.c > index 027f3c15c56..db6c0b10d15 100644 > --- a/gdb/features/arm/arm-m-profile.c > +++ b/gdb/features/arm/arm-m-profile.c > @@ -27,5 +27,7 @@ create_feature_arm_arm_m_profile (struct target_desc *result, long regnum) > tdesc_create_reg (feature, "pc", regnum++, 1, NULL, 32, "code_ptr"); > regnum = 25; > tdesc_create_reg (feature, "xpsr", regnum++, 1, NULL, 32, "int"); > + tdesc_create_reg (feature, "msp", regnum++, 1, NULL, 32, "data_ptr"); > + tdesc_create_reg (feature, "psp", regnum++, 1, NULL, 32, "data_ptr"); > return regnum; > } > diff --git a/gdb/features/arm/arm-m-profile.xml b/gdb/features/arm/arm-m-profile.xml > index d56fb246342..3253d1c5339 100644 > --- a/gdb/features/arm/arm-m-profile.xml > +++ b/gdb/features/arm/arm-m-profile.xml > @@ -24,4 +24,6 @@ > > > > + > + ... and in this xml the psp and msp registers are present in the "org.gnu.gdb.arm.m-profile" feature, not "org.gnu.gdb.arm.m-system". So the code above that checks for the "org.gnu.gdb.arm.m-system" feature won't work as expected. I like the idea of having these two registers in a separate feature like "org.gnu.gdb.arm.m-system". I think it is cleaner that way, and easier to manage. > >