From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 0F3BE38356A7 for ; Fri, 22 Jul 2022 15:25:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0F3BE38356A7 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 4EA1C1063; Fri, 22 Jul 2022 08:25:35 -0700 (PDT) Received: from [10.57.10.55] (unknown [10.57.10.55]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 276453F766; Fri, 22 Jul 2022 08:25:34 -0700 (PDT) Message-ID: <10b41ec9-673e-bcf8-8321-6f6399938b43@foss.arm.com> Date: Fri, 22 Jul 2022 16:25:32 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.1 Subject: Re: [PATCH][Binutils][GAS] arm: Fix DWARF pseudo-register numbering convention Content-Language: en-GB To: Victor Do Nascimento , binutils@sourceware.org Cc: richard.earnshaw@arm.com References: <20220722121249.22131-1-victor.donascimento@arm.com> From: Richard Earnshaw In-Reply-To: <20220722121249.22131-1-victor.donascimento@arm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3496.7 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, KAM_LAZY_DOMAIN_SECURITY, NICE_REPLY_A, SPF_HELO_NONE, SPF_NONE, 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 X-BeenThere: binutils@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Binutils mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 22 Jul 2022 15:25:37 -0000 On 22/07/2022 13:12, Victor Do Nascimento via Binutils wrote: > Hi all, > > This patch restores the assembler internal numbering of DWARF pseudo- > registers to the preferred modulo 134 convention (see "4.1 DWARF register > names" in [1]) and amends the unwinder .save directive processing code to > handle this. > > Issue: > 1. DWARF pseudo-registers to be assigned their own reg_entry numbering > system based on modulo 134. Pseudo-reg no. 143 thus gets assigned > to (143 % 134) = 9. > Not orthogonal to core-register numbering. > 2. We must have a way to to map pseudo-register numbers to > corresponding physical register numbers and vice-versa. More > concretely, issues first arise when invoking s_arm_unwind_save_mixed. > > Consider parsing the hypothetical directive: > > .save{r4-r7, r10, ra_auth_core, lr} > > As `.save`-ing ra_auth_code requires a special DWARF opcode, this > directive must be split into several sub-statements, i.e. > > .save{r4-r7, r10} > .save{ra_auth_code} > .save{lr} > .save directives have a side effect of adjusting the stack calculations, so I think we need to emit these in the opposite order, with the items at higher addresses first (stack grows downwards). Ergo: .save{lr} .save{ra_auth_code} .save{r4-r7, r10} > we pass s_arm_unwind_save_mixed 2 bitmaps - one of physical register > numbers and one of pseudo-register numbers. > > core-register bitmap: 000011110010001 > pseudo-register bitmap: 000000000100000 > > Under proposed modulo 134 numbering system, this would erroneously > produce: > > .save{r4-r7} > .save{ra_auth_code} > .save{r10, lr} > > the pseudo-reg bitmap must be cast to physical register space to > know how to split .save directives correctly. So something like: > > core-register bitmap: 000011110010001 > recast pseudo-register bitmap: 000000000000100 > > would give us the correct split of the .save statements. > > 3. so our bitmap of pseudo-registers cast in physical register space > tells us that bit 12 came from a pseudo-register, but with no > guarantee (or indeed any reason to expect) that there is > preservation of register ordering in mapping between DWARF register > space and core-register space, in the presence of multiple > pseudo-registers, there is no immediate way of inferring which bit > in the DWARF bitmap mapped onto bit 12. This therefore creates the > need for a function to convert DWARF bitmaps into core-register > bitmaps for splitting the register list and then another for > translating core-register numbers back to DWARF register numbers to > correctly handle the pseudo-registers. > This is the least invasive implementation for handling the proposed > DWARF modulo 134 numbering system, but also seems to involve a great > deal of mapping to and fro numbering conventions. > > I've not yet thought of an alternative that is better and doesn't > require too many invasive alterations to the existing codebase (though > I'm sure it may well be out there). > Your problem comes from trying to mash together two bitmaps describing distinct elements and assuming that the ra_code will always be copied to IP (it probably will be, but it's dangerous to assume that). Don't do that. Instead, I think you need to build the component .save objects as you break down the input string; but since you parse the string left-to-right and want the push operations to describe right-to-left, you'll probably want some form of recursion. As a rough outline, I'd suggest your parser does something like parse_dot_save(char **str_p) { core_regs = 0; while ((reg = reg_parse (CORE, str_p) != INVALID) core_regs |= reg; if (core_regs) { // recurse for remainder of input parse_dot_save (str_p); // Now emit the regs we parsed. emit_dot_save (core_regs); return; } // Pseudo regs are always emitted singly. regno = reg_parse (PSEUDOS, str_p); if (regno != INVALID) { // recurse for remainder of input parse_dot_save (str_p); emit_dot_save_pseudo (regno); return; } // error case } error, comma and end-of-list handling will complicate the above slightly, but that should be the general gist of it. If that doesn't work, you'll need to build a table of actions during the parse and then emit the actions in reverse order once you've finished. Try not to assume there will be exactly one pseudo in the list (we might add more at a later date) or that there will be exactly two sublists of core registers (there might be zero, one, two, or perhaps even many one day). Furthermore, I'd suggest you don't try to convert the pseudos into a bitmask, there's no real value, and thus you might as well give the register its natural dwarf number internally. R. > Therefore, any feedback/advice/suggestions would be most welcome. > > Proposed changes: > 1. Implement a function to cast a bitmap of pseudo-registers to > corresponding physical register numbers: tc_dw2range_to_arm_range. > 2. When emitting an unwinder save instruction for a pseudo-register > have a function that maps back physical register ranges back to the > correct pseudo-register numbers: tc_arm_range_to_dw2range > > [1] > > Many thanks, > Victor > > gas/Changelog: > > * config/tc-arm.c: > (reg_names): Renumber RA_AUTH_CODE from 12 to 9. > (parse_reg_list): Ensure correct range-handling. > (s_arm_unwind_save_pseudo): Fix register number. > (s_arm_unwind_save_mixed): Add number system interconversion. > (tc_dw2range_to_arm_range): New. > (tc_arm_range_to_dw2range): New. > * config/tc-arm.h: Add new function prototypes. > --- > gas/config/tc-arm.c | 64 ++++++++++++++++++++++++++++++++++++++++----- > gas/config/tc-arm.h | 2 ++ > 2 files changed, 60 insertions(+), 6 deletions(-) > > diff --git a/gas/config/tc-arm.c b/gas/config/tc-arm.c > index 2e6d175482e..f66d2acca0e 100644 > --- a/gas/config/tc-arm.c > +++ b/gas/config/tc-arm.c > @@ -1948,13 +1948,13 @@ parse_reg_list (char ** strp, enum reg_list_els etype) > if (reg == FAIL && rt == REG_TYPE_PSEUDO > && ((reg = arm_reg_parse (&str, REG_TYPE_RN)) != FAIL)) > { > - cur_reg = reg; > + in_range = 0; > continue; > } > else if (reg == FAIL && rt == REG_TYPE_RN > && ((reg = arm_reg_parse (&str, REG_TYPE_PSEUDO)) != FAIL)) > { > - cur_reg = reg; > + in_range = 0; > continue; > } > > @@ -4301,7 +4301,7 @@ s_arm_unwind_save_pseudo (long range) > { > valueT op; > > - if (range & (1 << 12)) > + if (range & (1 << 9)) > { > /* Opcode for restoring RA_AUTH_CODE. */ > op = 0xb4; > @@ -4724,15 +4724,19 @@ s_arm_unwind_save_mmxwcg (void) > static void > s_arm_unwind_save_mixed (long range, long mask_range) > { > + > + mask_range = tc_dw2range_to_arm_range (mask_range); > + > while (mask_range) > { > long mask_bit = mask_range & -mask_range; > + long pseudo_reg = tc_arm_range_to_dw2range (mask_bit); > long subrange = range & (mask_bit - 1); > > if (subrange) > - s_arm_unwind_save_core (subrange); > + s_arm_unwind_save_core (subrange); > > - s_arm_unwind_save_pseudo (mask_bit); > + s_arm_unwind_save_pseudo (pseudo_reg); > range &= ~subrange; > mask_range &= ~mask_bit; > } > @@ -23998,7 +24002,7 @@ static const struct reg_entry reg_names[] = > for tc_arm_regname_to_dw2regnum to translate to DWARF reg number using > 134 + reg_number should the range 134 to 142 be used for more pseudo regs > in the future. This also helps fit RA_AUTH_CODE into a bitmask. */ > - REGDEF(ra_auth_code,12,PSEUDO), > + REGDEF(ra_auth_code,9,PSEUDO), > }; > #undef REGDEF > #undef REGNUM > @@ -27936,6 +27940,54 @@ tc_arm_regname_to_dw2regnum (char *regname) > return FAIL; > } > > +/* Cast a bitmap representation of a register set from DWARF-2 modulo 134 > + pseudo-register numbering to the corresponding physical reg. numbers. */ > + > +long > +tc_dw2range_to_arm_range (long pseudo_range) > +{ > + long phys_range = 0; > + long reg = 0; > + while (pseudo_range) > + { > + reg = pseudo_range & -pseudo_range; > + switch (reg) > + { > + case (1 << 9): > + phys_range |= (1 << 12); > + break; > + default: > + break; > + } > + pseudo_range &= ~reg; > + } > + return phys_range; > +} > + > + > +/* from DWARF-2 modulo 134 pseudo-register numbering retrieve physical regs. */ > + > +long > +tc_arm_range_to_dw2range (long phys_range) > +{ > + long pseudo_range = 0; > + long reg = 0; > + while (phys_range) > + { > + reg = phys_range & -phys_range; > + switch (reg) > + { > + case (1 << 12): > + pseudo_range |= (1 << 9); > + break; > + default: > + break; > + } > + phys_range &= ~reg; > + } > + return pseudo_range; > +} > + > #ifdef TE_PE > void > tc_pe_dwarf2_emit_offset (symbolS *symbol, unsigned int size) > diff --git a/gas/config/tc-arm.h b/gas/config/tc-arm.h > index d2176e03c98..8a8cb15d702 100644 > --- a/gas/config/tc-arm.h > +++ b/gas/config/tc-arm.h > @@ -359,6 +359,8 @@ extern void arm_handle_align (struct frag *); > extern bool arm_fix_adjustable (struct fix *); > extern int arm_elf_section_type (const char *, size_t); > extern int tc_arm_regname_to_dw2regnum (char *regname); > +extern long tc_dw2range_to_arm_range (long); > +extern long tc_arm_range_to_dw2range (long); > extern void tc_arm_frame_initial_instructions (void); > > #ifdef TE_PE