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 8E8813875B7C for ; Wed, 14 Dec 2022 17:00:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8E8813875B7C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=foss.arm.com Authentication-Results: sourceware.org; spf=none smtp.mailfrom=foss.arm.com 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 D0371FEC; Wed, 14 Dec 2022 09:01:30 -0800 (PST) Received: from [10.57.11.95] (unknown [10.57.11.95]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 59AF33F5A1; Wed, 14 Dec 2022 09:00:49 -0800 (PST) Message-ID: <7db3982c-cd42-023f-0dd1-0eb7b7dbfb20@foss.arm.com> Date: Wed, 14 Dec 2022 17:00:47 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.2 Subject: Re: [PATCH 12/15 V4] arm: implement bti injection Content-Language: en-GB To: Andrea Corallo Cc: Kyrylo Tkachov , Richard Earnshaw , nd , Andrea Corallo via Gcc-patches References: <30990d91-0c32-a3b0-7954-846bf1eddc8d@foss.arm.com> From: Richard Earnshaw In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3489.6 required=5.0 tests=BAYES_00,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,NICE_REPLY_A,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 14/12/2022 16:40, Andrea Corallo via Gcc-patches wrote: > Hi Richard, > > thanks for reviewing. > > Richard Earnshaw writes: > >> On 28/10/2022 17:40, Andrea Corallo via Gcc-patches wrote: >>> Hi all, >>> please find attached the third iteration of this patch addresing >>> review >>> comments. >>> Thanks >>> Andrea >>> >> >> @@ -23374,12 +23374,6 @@ output_probe_stack_range (rtx reg1, rtx reg2) >> return ""; >> } >> >> -static bool >> -aarch_bti_enabled () >> -{ >> - return false; >> -} >> - >> /* Generate the prologue instructions for entry into an ARM or Thumb-2 >> function. */ >> void >> @@ -32992,6 +32986,61 @@ arm_current_function_pac_enabled_p (void) >> && !crtl->is_leaf)); >> } >> >> +/* Return TRUE if Branch Target Identification Mechanism is enabled. */ >> +bool >> +aarch_bti_enabled (void) >> +{ >> + return aarch_enable_bti == 1; >> +} >> >> See comment in earlier patch about the location of this function >> moving. Can aarch_enable_bti take values other than 0 and 1? > > Yes default is 2. It shouldn't be by this point, because, hopefully you've gone through the equivalent of this hunk (from aarch64) somewhere in arm_override_options: if (aarch_enable_bti == 2) { #ifdef TARGET_ENABLE_BTI aarch_enable_bti = 1; #else aarch_enable_bti = 0; #endif } And after this point the '2' should never be seen again. We use this trick to permit the user to force a default that differs from the configuration. However, I don't see a hunk to do this in patch 3, so perhaps that needs updating to fix this. > [...] > >> + return GET_CODE (pat) == UNSPEC_VOLATILE && XINT (pat, 1) == >> UNSPEC_BTI_NOP; >> >> I'm not sure where this crept in, but UNSPEC and UNSPEC_VOLATILE have >> separate enums in the backend, so UNSPEC_BIT_NOP should really be >> VUNSPEC_BTI_NOP and defined in the enum "unspecv". > > Done > >> +aarch_pac_insn_p (rtx x) >> +{ >> + if (!x || !INSN_P (x)) >> + return false; >> + >> + rtx pat = PATTERN (x); >> + >> + if (GET_CODE (pat) == SET) >> + { >> + rtx tmp = XEXP (pat, 1); >> + if (tmp >> + && GET_CODE (tmp) == UNSPEC >> + && (XINT (tmp, 1) == UNSPEC_PAC_NOP >> + || XINT (tmp, 1) == UNSPEC_PACBTI_NOP)) >> + return true; >> + } >> + >> >> This will also need updating (see review on earlier patch) because >> PACBTI needs to be unspec_volatile, while PAC doesn't. > > Done > >> +/* The following two functions are for code compatibility with aarch64 >> + code, this even if in arm we have only one bti instruction. */ >> + >> >> I'd just write >> /* Target specific mapping for aarch_gen_bti_c and >> aarch_gen_bti_j. For Arm, both of these map to a simple BTI >> instruction. */ > > Done > >> >> @@ -162,6 +162,7 @@ (define_c_enum "unspec" [ >> UNSPEC_PAC_NOP ; Represents PAC signing LR >> UNSPEC_PACBTI_NOP ; Represents PAC signing LR + valid landing pad >> UNSPEC_AUT_NOP ; Represents PAC verifying LR >> + UNSPEC_BTI_NOP ; Represent BTI >> ]) >> >> BTI is an unspec volatile, so this should be in the "vunspec" enum and >> renamed accordingly (see above). > > Done. > > Please find attached the updated version of this patch. > > BR > > Andrea > Apart from that, this is OK. R.