From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 68577 invoked by alias); 22 Nov 2017 15:07:06 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 68562 invoked by uid 89); 22 Nov 2017 15:07:06 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.7 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,KB_WAM_FROM_NAME_SINGLEWORD,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: foss.arm.com Received: from usa-sjc-mx-foss1.foss.arm.com (HELO foss.arm.com) (217.140.101.70) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 22 Nov 2017 15:07:04 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 7B5891596; Wed, 22 Nov 2017 07:07:02 -0800 (PST) Received: from [10.2.206.52] (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id B06AC3F236; Wed, 22 Nov 2017 07:07:01 -0800 (PST) Subject: Re: [PATCH, GCC/ARM] Factor out CMSE register clearing code To: Kyrill Tkachov , Ramana Radhakrishnan , Richard Earnshaw , "gcc-patches@gcc.gnu.org" References: <5A158D9E.9060506@foss.arm.com> From: Thomas Preudhomme Message-ID: <2dbbe466-2884-7486-b365-b84ac607c539@foss.arm.com> Date: Wed, 22 Nov 2017 15:19:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <5A158D9E.9060506@foss.arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2017-11/txt/msg02045.txt.bz2 On 22/11/17 14:45, Kyrill Tkachov wrote: > Hi Thomas, > > On 15/11/17 17:12, Thomas Preudhomme wrote: >> Hi, >> >> Functions cmse_nonsecure_call_clear_caller_saved and >> cmse_nonsecure_entry_clear_before_return both contain very similar code >> to clear registers. What's worse, they differ slightly at times so if a >> bug is found in one careful thoughts is needed to decide whether the >> other function needs fixing too. >> >> This commit addresses the situation by factoring the two pieces of code >> into a new function. In doing so the code generated to clear VFP >> registers in cmse_nonsecure_call now uses the same sequence as >> cmse_nonsecure_entry functions. Tests expectation are thus updated >> accordingly. >> >> ChangeLog entry are as follow: >> >> *** gcc/ChangeLog *** >> >> 2017-10-24  Thomas Preud'homme >> >>         * config/arm/arm.c (cmse_clear_registers): New function. >>         (cmse_nonsecure_call_clear_caller_saved): Replace register clearing >>         code by call to cmse_clear_registers. >>         (cmse_nonsecure_entry_clear_before_return): Likewise. >> >> *** gcc/ChangeLog *** >> >> 2017-10-24  Thomas Preud'homme >> >>         * gcc.target/arm/cmse/mainline/hard-sp/cmse-13.c: Adapt expectations >>         to vmov instructions now generated. >>         * gcc.target/arm/cmse/mainline/hard-sp/cmse-7.c: Likewise. >>         * gcc.target/arm/cmse/mainline/hard-sp/cmse-8.c: Likewise. >>         * gcc.target/arm/cmse/mainline/hard/cmse-13.c: Likewise. >>         * gcc.target/arm/cmse/mainline/hard/cmse-7.c: Likewise. >>         * gcc.target/arm/cmse/mainline/hard/cmse-8.c: Likewise. >> >> Testing: bootstrapped on arm-linux-gnueabihf and no regression in the >> testsuite. >> >> Is this ok for trunk? >> > > This looks mostly ok, but I have a concern from reading the code that I'd like > some help with... > > diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c > index > 9b494e9529a4470c18192a4561e03d2f80e90797..22c9add0722974902b2a89b2b0a75759ff8ba37c > 100644 > --- a/gcc/config/arm/arm.c > +++ b/gcc/config/arm/arm.c > @@ -16991,6 +16991,128 @@ compute_not_to_clear_mask (tree arg_type, rtx arg_rtx, > int regno, >    return not_to_clear_mask; >  } > > +/* Clear registers secret before doing a cmse_nonsecure_call or returning from > +   a cmse_nonsecure_entry function.  TO_CLEAR_BITMAP indicates which registers > +   are to be fully cleared, using the value in register CLEARING_REG if more > +   efficient.  The PADDING_BITS_LEN entries array PADDING_BITS_TO_CLEAR gives > +   the bits that needs to be cleared in caller-saved core registers, with > +   SCRATCH_REG used as a scratch register for that clearing. > + > +   NOTE: one of three following assertions must hold: > +   - SCRATCH_REG is a low register > +   - CLEARING_REG is in the set of registers fully cleared (ie. its bit is set > +     in TO_CLEAR_BITMAP) > +   - CLEARING_REG is a low register.  */ > + > +static void > +cmse_clear_registers (sbitmap to_clear_bitmap, uint32_t *padding_bits_to_clear, > +              int padding_bits_len, rtx scratch_reg, rtx clearing_reg) > +{ > +  bool saved_clearing = false; > +  rtx saved_clearing_reg = NULL_RTX; > +  int i, regno, clearing_regno, minregno = R0_REGNUM, maxregno = minregno - 1; > + > > Here minregno becomes 0 and maxregno becomes -1... > > +  gcc_assert (arm_arch_cmse); > + > +  if (!bitmap_empty_p (to_clear_bitmap)) > +    { > +      minregno = bitmap_first_set_bit (to_clear_bitmap); > +      maxregno = bitmap_last_set_bit (to_clear_bitmap); > +    } > > > ...and here is a path on maxregno may not be set to a proper register number... > If bitmap is empty yes, ie. if no bit is set and no register should be cleared. > > + > +  for (regno = minregno; regno <= maxregno; regno++) > +    { > +      if (!bitmap_bit_p (to_clear_bitmap, regno)) > +    continue; > + > > ...and here we iterate from minregno (potentially 0) to maxregno (potentially > -1) which will lead to trouble. > Are there any guarantees that this case will not occur? It absolutely does occur and that's on purpose. If maxregno is -1 it means there is no bit to clear and so it is fine to do nothing. Best regards, Thomas