From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk1-x72e.google.com (mail-qk1-x72e.google.com [IPv6:2607:f8b0:4864:20::72e]) by sourceware.org (Postfix) with ESMTPS id 5648C3839406 for ; Thu, 31 Mar 2022 20:03:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5648C3839406 Received: by mail-qk1-x72e.google.com with SMTP id k125so507707qkf.0 for ; Thu, 31 Mar 2022 13:03:33 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=IaRB1qTMErjlcM80rKo2qAXINpkMfPYNNkAdno9nSdE=; b=1seXpg2vQo3HILeMkzZpqzhhxwvANx2jSQTPOTIU12biW6WV1cl/3Kn/YpBgD0rzqS mulN7qCvrSXWJ92q0dCFgToMUg2qy3g4NVci9aPTPnvK3TYkonmPNrnMkrIbfGBAU03Z UQIKGVwHPKti1Rj5fgW11PWxORkfRd1Ruf8v8yex61QY7kyoVT/z4XY6J3U/xHXWQDff 0bL5fQGyckPk77VnrL6ZtB0qUU7ywB28kgxpNUhZF317oIlwoFziDeiTxnQQSn+p421p 8PQjHdknohd83HoVfDqZiT42JArCnUKI/GaI+TsKbmfhibNoJwYZ2teQlY1YLyWJsmDF pDMQ== X-Gm-Message-State: AOAM533mNp2YFQ5nf2A+MnW9c0ofWFPLqnmBTtkvCfwId12U91VhYoo2 dPIePnaxJjHCKxTdNbZbryTzSJzPKMxc0drlGAk= X-Google-Smtp-Source: ABdhPJx++ccVCG5Vd9s9wba4fv9csC/941jebTI7Oxnoc6iC6Nd4TCNyK54TUY/DX1K4NT8bUq6EujXO6F3zdpCH958= X-Received: by 2002:a05:620a:3725:b0:67d:693e:7e with SMTP id de37-20020a05620a372500b0067d693e007emr4597176qkb.674.1648757012570; Thu, 31 Mar 2022 13:03:32 -0700 (PDT) MIME-Version: 1.0 References: <01D26A82-EED3-48AA-BF56-F6300AC2C7CC@oracle.com> In-Reply-To: <01D26A82-EED3-48AA-BF56-F6300AC2C7CC@oracle.com> From: Uros Bizjak Date: Thu, 31 Mar 2022 22:03:21 +0200 Message-ID: Subject: Re: [patch]update the documentation for TARGET_ZERO_CALL_USED_REGS hook and add an assertion To: Qing Zhao Cc: richard Sandiford , gcc-patches Paul A Clarke via Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-8.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE 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: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 31 Mar 2022 20:03:35 -0000 On Mon, Mar 21, 2022 at 10:28 PM Qing Zhao via Gcc-patches wrote: > > Hi, > > Per our discussion on: https://gcc.gnu.org/pipermail/gcc-patches/2022-Mar= ch/592002.html > > I come up with the following patch to: > > 1. Update the documentation for TARGET_ZERO_CALL_USED_REGS hook; > 2. Add an assertion in function.cc to make sure the actually zeroed_regs = is a subset of all call used regs; > (The reason I didn=E2=80=99t add a new parameter to TARGET_ZERO_CALL_U= SED_REGS is, I think adding the > assertion in the common place function.cc is simpler to be implemente= d). > 3. This new assertion identified a bug in i386 implementation. Fix this b= ug in i386. > > This patch is bootstrapped on both x86 and aarch64, no regression. > > Okay for commit? > > thanks. > > Qing > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > From 2e5bc1b25a707c6a17afbf03da2a8bec5b03454d Mon Sep 17 00:00:00 2001 > From: Qing Zhao > Date: Fri, 18 Mar 2022 20:49:56 +0000 > Subject: [PATCH] Add an assertion: the zeroed_hardregs set is a subset of= all > call used regs. > > We should make sure that the hard register set that is actually cleared b= y > the target hook zero_call_used_regs should be a subset of all call used > registers. > > At the same time, update documentation for the target hook > TARGET_ZERO_CALL_USED_REGS. > > This new assertion identified a bug in the i386 implemenation, which > incorrectly set the zeroed_hardregs for stack registers. Fixed this bug > in i386 implementation. > > gcc/ChangeLog: > > 2022-03-21 Qing Zhao > > * config/i386/i386.cc (zero_all_st_registers): Return the value o= f > num_of_st. > (ix86_zero_call_used_regs): Update zeroed_hardregs set according = to > the return value of zero_all_st_registers. > * doc/tm.texi: Update the documentation of TARGET_ZERO_CALL_USED_= REGS. > * function.cc (gen_call_used_regs_seq): Add an assertion. > * target.def: Update the documentation of TARGET_ZERO_CALL_USED_R= EGS. OK for the i386 part. Thanks, Uros. > --- > gcc/config/i386/i386.cc | 27 ++++++++++++++++++--------- > gcc/doc/tm.texi | 7 +++++++ > gcc/function.cc | 22 ++++++++++++++++++---- > gcc/target.def | 7 +++++++ > 4 files changed, 50 insertions(+), 13 deletions(-) > > diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc > index 5a561966eb44..d84047a4bc1b 100644 > --- a/gcc/config/i386/i386.cc > +++ b/gcc/config/i386/i386.cc > @@ -3753,16 +3753,17 @@ zero_all_vector_registers (HARD_REG_SET need_zero= ed_hardregs) > needs to be cleared, the whole stack should be cleared. However, > x87 stack registers that hold the return value should be excluded. > x87 returns in the top (two for complex values) register, so > - num_of_st should be 7/6 when x87 returns, otherwise it will be 8. */ > + num_of_st should be 7/6 when x87 returns, otherwise it will be 8. > + return the value of num_of_st. */ > > > -static bool > +static int > zero_all_st_registers (HARD_REG_SET need_zeroed_hardregs) > { > > /* If the FPU is disabled, no need to zero all st registers. */ > if (! (TARGET_80387 || TARGET_FLOAT_RETURNS_IN_80387)) > - return false; > + return 0; > > unsigned int num_of_st =3D 0; > for (unsigned int regno =3D 0; regno < FIRST_PSEUDO_REGISTER; regno++) > @@ -3774,7 +3775,7 @@ zero_all_st_registers (HARD_REG_SET need_zeroed_har= dregs) > } > > if (num_of_st =3D=3D 0) > - return false; > + return 0; > > bool return_with_x87 =3D false; > return_with_x87 =3D (crtl->return_rtx > @@ -3802,7 +3803,7 @@ zero_all_st_registers (HARD_REG_SET need_zeroed_har= dregs) > insn =3D emit_insn (gen_rtx_SET (st_reg, st_reg)); > add_reg_note (insn, REG_DEAD, st_reg); > } > - return true; > + return num_of_st; > } > > > @@ -3851,7 +3852,7 @@ ix86_zero_call_used_regs (HARD_REG_SET need_zeroed_= hardregs) > { > HARD_REG_SET zeroed_hardregs; > bool all_sse_zeroed =3D false; > - bool all_st_zeroed =3D false; > + int all_st_zeroed_num =3D 0; > bool all_mm_zeroed =3D false; > > CLEAR_HARD_REG_SET (zeroed_hardregs); > @@ -3881,9 +3882,17 @@ ix86_zero_call_used_regs (HARD_REG_SET need_zeroed= _hardregs) > if (!exit_with_mmx_mode) > /* x87 exit mode, we should zero all st registers together. */ > { > - all_st_zeroed =3D zero_all_st_registers (need_zeroed_hardregs); > - if (all_st_zeroed) > - SET_HARD_REG_BIT (zeroed_hardregs, FIRST_STACK_REG); > + all_st_zeroed_num =3D zero_all_st_registers (need_zeroed_hardregs)= ; > + > + if (all_st_zeroed_num > 0) > + for (unsigned int regno =3D FIRST_STACK_REG; regno <=3D LAST_STAC= K_REG; regno++) > + /* x87 stack registers that hold the return value should be exc= luded. > + x87 returns in the top (two for complex values) register. *= / > + if (all_st_zeroed_num =3D=3D 8 > + || !((all_st_zeroed_num >=3D 6 && regno =3D=3D REGNO (crtl-= >return_rtx)) > + || (all_st_zeroed_num =3D=3D 6 > + && (regno =3D=3D (REGNO (crtl->return_rtx) + 1))))= ) > + SET_HARD_REG_BIT (zeroed_hardregs, regno); > } > else > /* MMX exit mode, check whether we can zero all mm registers. */ > diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi > index 2f92d37da8c0..c5006afc00d2 100644 > --- a/gcc/doc/tm.texi > +++ b/gcc/doc/tm.texi > @@ -12330,6 +12330,13 @@ This target hook emits instructions to zero the = subset of @var{selected_regs} > that could conceivably contain values that are useful to an attacker. > Return the set of registers that were actually cleared. > > +For most targets, the returned set of registers is a subset of > +@var{selected_regs}, however, for some of the targets (for example MIPS)= , > +clearing some registers that are in the @var{selected_regs} requires > +clearing other call used registers that are not in the @var{selected_reg= s}, > +under such situation, the returned set of registers must be a subset of = all > +call used registers. > + > The default implementation uses normal move instructions to zero > all the registers in @var{selected_regs}. Define this hook if the > target has more efficient ways of zeroing certain registers, > diff --git a/gcc/function.cc b/gcc/function.cc > index d5ed51a6a663..ad0096a43eff 100644 > --- a/gcc/function.cc > +++ b/gcc/function.cc > @@ -5892,7 +5892,9 @@ gen_call_used_regs_seq (rtx_insn *ret, unsigned int= zero_regs_type) > df_simulate_one_insn_backwards (bb, ret, live_out); > > HARD_REG_SET selected_hardregs; > + HARD_REG_SET all_call_used_regs; > CLEAR_HARD_REG_SET (selected_hardregs); > + CLEAR_HARD_REG_SET (all_call_used_regs); > for (unsigned int regno =3D 0; regno < FIRST_PSEUDO_REGISTER; regno++) > { > if (!crtl->abi->clobbers_full_reg_p (regno)) > @@ -5901,6 +5903,13 @@ gen_call_used_regs_seq (rtx_insn *ret, unsigned in= t zero_regs_type) > continue; > if (REGNO_REG_SET_P (live_out, regno)) > continue; > +#ifdef LEAF_REG_REMAP > + if (crtl->uses_only_leaf_regs && LEAF_REG_REMAP (regno) < 0) > + continue; > +#endif > + /* This is a call used register that is dead at return. */ > + SET_HARD_REG_BIT (all_call_used_regs, regno); > + > if (only_gpr > && !TEST_HARD_REG_BIT (reg_class_contents[GENERAL_REGS], regno)= ) > continue; > @@ -5908,10 +5917,6 @@ gen_call_used_regs_seq (rtx_insn *ret, unsigned in= t zero_regs_type) > continue; > if (only_arg && !FUNCTION_ARG_REGNO_P (regno)) > continue; > -#ifdef LEAF_REG_REMAP > - if (crtl->uses_only_leaf_regs && LEAF_REG_REMAP (regno) < 0) > - continue; > -#endif > > /* Now this is a register that we might want to zero. */ > SET_HARD_REG_BIT (selected_hardregs, regno); > @@ -5925,6 +5930,15 @@ gen_call_used_regs_seq (rtx_insn *ret, unsigned in= t zero_regs_type) > HARD_REG_SET zeroed_hardregs; > start_sequence (); > zeroed_hardregs =3D targetm.calls.zero_call_used_regs (selected_hardre= gs); > + > + /* For most targets, the returned set of registers is a subset of > + selected_hardregs, however, for some of the targets (for example MI= PS), > + clearing some registers that are in selected_hardregs requires clea= ring > + other call used registers that are not in the selected_hardregs, un= der > + such situation, the returned set of registers must be a subset of > + all call used registers. */ > + gcc_assert (hard_reg_set_subset_p (zeroed_hardregs, all_call_used_regs= )); > + > rtx_insn *seq =3D get_insns (); > end_sequence (); > if (seq) > diff --git a/gcc/target.def b/gcc/target.def > index 72c2e1ef756c..d85adf36a391 100644 > --- a/gcc/target.def > +++ b/gcc/target.def > @@ -5122,6 +5122,13 @@ DEFHOOK > that could conceivably contain values that are useful to an attacker.\n\ > Return the set of registers that were actually cleared.\n\ > \n\ > +For most targets, the returned set of registers is a subset of\n\ > +@var{selected_regs}, however, for some of the targets (for example MIPS)= ,\n\ > +clearing some registers that are in the @var{selected_regs} requires\n\ > +clearing other call used registers that are not in the @var{selected_reg= s},\n\ > +under such situation, the returned set of registers must be a subset of = all\n\ > +call used registers.\n\ > +\n\ > The default implementation uses normal move instructions to zero\n\ > all the registers in @var{selected_regs}. Define this hook if the\n\ > target has more efficient ways of zeroing certain registers,\n\ > -- > 2.27.0 >