From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk1-x72f.google.com (mail-qk1-x72f.google.com [IPv6:2607:f8b0:4864:20::72f]) by sourceware.org (Postfix) with ESMTPS id 395FF3858D35 for ; Fri, 14 Aug 2020 10:03:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 395FF3858D35 Received: by mail-qk1-x72f.google.com with SMTP id j187so7800835qke.11 for ; Fri, 14 Aug 2020 03:03:58 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=++iIEIPFFd7lpGiECH8iwkVCMJwPVf4odvx5lYBwkjg=; b=o1u+PoKhLvKzmJ4f5XkYW+V2I60Au944Tm70hg566+Ejl32JptCoLRhqRczzdp93sD S/EFcBsfYv6xmPPsLFTRk2FTejBxsFc4hdEdMgTbLTu+sfr9r8/0BNKRDqxav/goS1wu vg3ZPbEgdPCBJCq2bC1jZHVEpVH8tHLX66zEmuj7740MIEtYU3gJgiSyBEugjv/j1IgU iE6C8Iz3H/eirkSW2xXEC9FfNI5mvGW552Q2pQgBIlGBp+ys7kxvPJG3vK5nmOM0yAWU QH7F4mM0u+Re28Pp6SpISVZawZMgfqEkf9sCrCIoar7zUK+09Uha2eDEwyVMxDDxp0/+ 1/OA== X-Gm-Message-State: AOAM531PXmVIHg2cDHHId+bIJr8EkEcNL6ng/8xjWOeF+BaoYOGXna5x r68/gs/9PtOL79M4KFq70OERBaxfyrWGnzCi+U8= X-Google-Smtp-Source: ABdhPJyBhSGTrvIQNBFrecgAndpceD6Te3mF/5i8JWwLH1p89Taix1yKIWbA1urOuvk0qNn7GEV+PV11Gh8lr5q3dqE= X-Received: by 2002:a05:620a:531:: with SMTP id h17mr1179883qkh.61.1597399437652; Fri, 14 Aug 2020 03:03:57 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Uros Bizjak Date: Fri, 14 Aug 2020 12:03:46 +0200 Message-ID: Subject: Re: [PATCH] [PR target/96350]Force ENDBR immediate into memory to avoid fake ENDBR opcode. To: Hongtao Liu Cc: GCC Patches , "H. J. Lu" Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Fri, 14 Aug 2020 10:03:59 -0000 On Fri, Aug 14, 2020 at 6:54 AM Hongtao Liu wrote: > > On Tue, Aug 11, 2020 at 5:56 PM Uros Bizjak wrote: > > > > On Tue, Aug 11, 2020 at 11:36 AM Hongtao Liu wrote: > > > > > > On Tue, Aug 11, 2020 at 4:38 PM Uros Bizjak wrote: > > > > > > > > On Tue, Aug 11, 2020 at 5:30 AM Hongtao Liu wrote: > > > > > > > > > > Hi: > > > > > The issue is described in the bugzilla. > > > > > Bootstrap is ok, regression test for i386/x86-64 backend is ok. > > > > > Ok for trunk? > > > > > > > > > > ChangeLog > > > > > gcc/ > > > > > PR target/96350 > > > > > * config/i386/i386.c (ix86_legitimate_constant_p): Return > > > > > false for ENDBR immediate. > > > > > (ix86_legitimate_address_p): Ditto. > > > > > * config/i386/predicated.md > > > > > (x86_64_immediate_operand): Exclude ENDBR immediate. > > > > > (x86_64_zext_immediate_operand): Ditto. > > > > > (x86_64_dwzext_immediate_operand): Ditto. > > > > > (ix86_not_endbr_immediate_operand): New predicate. > > > > > > > > > > gcc/testsuite > > > > > * gcc.target/i386/endbr_immediate.c: New test. > > > > > > > > +;; Return true if VALUE isn't an ENDBR opcode in immediate field. > > > > +(define_predicate "ix86_not_endbr_immediate_operand" > > > > + (match_test "1") > > > > > > > > Please reverse the above logic to introduce > > > > ix86_endbr_immediate_operand, that returns true for unwanted > > > > immediate. Something like: > > > > > > > > (define_predicate "ix86_endbr_immediate_operand" > > > > (match_code "const_int") > > > > ... > > > > > > > > And you will be able to use it like: > > > > > > > > if (ix86_endbr_immediate_operand (x, VOIDmode) > > > > return false; > > > > > > > > > > Changed. > > > > No, it is not. > > > > + if ((flag_cf_protection & CF_BRANCH) > > + && CONST_INT_P (op)) > > > > You don't need to check for const ints here. > > > > And please rewrite the body of the function to something like (untested): > > > > { > > unsigned HOST_WIDE_INT val = TARGET_64BIT ? 0xfa1e0ff3 : 0xfb1e0ff3; > > > > if (x == val) > > return 1; > > > > if (TARGET_64BIT) > > for (; x >= val; x >>= 8) > > if (x == val) > > return 1; > > > > return 0; > > } > > > > so it will at least *look* like some thoughts have been spent on this. > > I don't plan to review the code where it is obvious from the first > > look that it was thrown together in a hurry. Please get some internal > > company signoff first. Ping me in a week for a review. > > > > Sorry for the hurry, i know your time is precious. > > > Uros. > > > > > > > /* Otherwise we handle everything else in the move patterns. */ > > > > - return true; > > > > + return ix86_not_endbr_immediate_operand (x, VOIDmode); > > > > } > > > > > > > > Please handle this in CASE_CONST_SCALAR_INT: part. > > > > > > > > + if (disp && !ix86_not_endbr_immediate_operand (disp, VOIDmode)) > > > > + return false; > > > > > > > > And this in: > > > > > > > > /* Validate displacement. */ > > > > if (disp) > > > > { > > > > > > > > > > Changed. > > > > A better place for these new special cases is at the beginning of the > > part I referred, not at the end. > > > > Yes. > > > Uros. > > Update patch. OK with two nits below. Thanks, Uros. + if (flag_cf_protection & CF_BRANCH) + { + unsigned HOST_WIDE_INT imm = INTVAL (op); UINTVAL, just for the consistency. + unsigned HOST_WIDE_INT val = TARGET_64BIT ? 0xfa1e0ff3 : 0xfb1e0ff3; @@ -374,6 +402,8 @@ (define_predicate "x86_64_dwzext_immediate_operand" (match_code "const_int,const_wide_int") { + if (ix86_endbr_immediate_operand (op, VOIDmode)) + return false; vertical space here. switch (GET_CODE (op)) > > -- > BR, > Hongtao