Re: [PATCH] x86: Insert ENDBR if function will be called indirectly
On Tue, 2020-03-31 at 08:11 -0700, H.J. Lu via Gcc-patches wrote: > Since constant_call_address_operand has > > ;; Test for a pc-relative call operand > (define_predicate "constant_call_address_operand" > (match_code "symbol_ref") > { > if (ix86_cmodel == CM_LARGE || ix86_cmodel == CM_LARGE_PIC > || flag_force_indirect_call) > return false; > if (TARGET_DLLIMPORT_DECL_ATTRIBUTES && SYMBOL_REF_DLLIMPORT_P (op)) > return false; > return true; > }) > > even if cgraph_node::get (cfun->decl)->only_called_directly_p () returns > false, the fuction may still be called indirectly. Copy the logic from > constant_call_address_operand to rest_of_insert_endbranch to insert ENDBR > at function entry if function will be called indirectly. > > gcc/ > > PR target/94417 > * config/i386/i386-features.c (rest_of_insert_endbranch): Insert > ENDBR at function entry if function will be called indirectly. Can you just call constant_call_address_operand rather than copying its contents? jeff >
Re: [PATCH] x86: Insert ENDBR if function will be called indirectly
On Wed, Apr 8, 2020 at 9:16 AM Jeff Law wrote: > > On Tue, 2020-03-31 at 08:11 -0700, H.J. Lu via Gcc-patches wrote: > > Since constant_call_address_operand has > > > > ;; Test for a pc-relative call operand > > (define_predicate "constant_call_address_operand" > > (match_code "symbol_ref") > > { > > if (ix86_cmodel == CM_LARGE || ix86_cmodel == CM_LARGE_PIC > > || flag_force_indirect_call) > > return false; > > if (TARGET_DLLIMPORT_DECL_ATTRIBUTES && SYMBOL_REF_DLLIMPORT_P (op)) > > return false; > > return true; > > }) > > > > even if cgraph_node::get (cfun->decl)->only_called_directly_p () returns > > false, the fuction may still be called indirectly. Copy the logic from > > constant_call_address_operand to rest_of_insert_endbranch to insert ENDBR > > at function entry if function will be called indirectly. > > > > gcc/ > > > > PR target/94417 > > * config/i386/i386-features.c (rest_of_insert_endbranch): Insert > > ENDBR at function entry if function will be called indirectly. > Can you just call constant_call_address_operand rather than copying its > contents? I wish I could. constant_call_address_operand uses SYMBOL_REF_DLLIMPORT_P (op) But I need to use DECL_DLLIMPORT_P (cfun->decl)). -- H.J.
Re: [PATCH] x86: Insert ENDBR if function will be called indirectly
On Wed, 2020-04-08 at 09:23 -0700, H.J. Lu wrote: > On Wed, Apr 8, 2020 at 9:16 AM Jeff Law wrote: > > On Tue, 2020-03-31 at 08:11 -0700, H.J. Lu via Gcc-patches wrote: > > > Since constant_call_address_operand has > > > > > > ;; Test for a pc-relative call operand > > > (define_predicate "constant_call_address_operand" > > > (match_code "symbol_ref") > > > { > > > if (ix86_cmodel == CM_LARGE || ix86_cmodel == CM_LARGE_PIC > > > || flag_force_indirect_call) > > > return false; > > > if (TARGET_DLLIMPORT_DECL_ATTRIBUTES && SYMBOL_REF_DLLIMPORT_P (op)) > > > return false; > > > return true; > > > }) > > > > > > even if cgraph_node::get (cfun->decl)->only_called_directly_p () returns > > > false, the fuction may still be called indirectly. Copy the logic from > > > constant_call_address_operand to rest_of_insert_endbranch to insert ENDBR > > > at function entry if function will be called indirectly. > > > > > > gcc/ > > > > > > PR target/94417 > > > * config/i386/i386-features.c (rest_of_insert_endbranch): Insert > > > ENDBR at function entry if function will be called indirectly. > > Can you just call constant_call_address_operand rather than copying its > > contents? > > I wish I could. constant_call_address_operand uses SYMBOL_REF_DLLIMPORT_P > (op) > But I need to use DECL_DLLIMPORT_P (cfun->decl)). Sigh. In that case I guess the patch is OK as-is. jeff >
Re: [PATCH] x86: Insert ENDBR if function will be called indirectly
On Wed, Apr 8, 2020 at 9:41 AM Jeff Law wrote: > > On Wed, 2020-04-08 at 09:23 -0700, H.J. Lu wrote: > > On Wed, Apr 8, 2020 at 9:16 AM Jeff Law wrote: > > > On Tue, 2020-03-31 at 08:11 -0700, H.J. Lu via Gcc-patches wrote: > > > > Since constant_call_address_operand has > > > > > > > > ;; Test for a pc-relative call operand > > > > (define_predicate "constant_call_address_operand" > > > > (match_code "symbol_ref") > > > > { > > > > if (ix86_cmodel == CM_LARGE || ix86_cmodel == CM_LARGE_PIC > > > > || flag_force_indirect_call) > > > > return false; > > > > if (TARGET_DLLIMPORT_DECL_ATTRIBUTES && SYMBOL_REF_DLLIMPORT_P (op)) > > > > return false; > > > > return true; > > > > }) > > > > > > > > even if cgraph_node::get (cfun->decl)->only_called_directly_p () returns > > > > false, the fuction may still be called indirectly. Copy the logic from > > > > constant_call_address_operand to rest_of_insert_endbranch to insert > > > > ENDBR > > > > at function entry if function will be called indirectly. > > > > > > > > gcc/ > > > > > > > > PR target/94417 > > > > * config/i386/i386-features.c (rest_of_insert_endbranch): Insert > > > > ENDBR at function entry if function will be called indirectly. > > > Can you just call constant_call_address_operand rather than copying its > > > contents? > > > > I wish I could. constant_call_address_operand uses SYMBOL_REF_DLLIMPORT_P > > (op) > > But I need to use DECL_DLLIMPORT_P (cfun->decl)). > Sigh. In that case I guess the patch is OK as-is. > I'd like to backport this wrong code fix to GCC 9/8 branches. Is it OK for GCC 9/8 branches? Thanks. -- H.J. From f86e27e71fb963aaa95b1da31515f888f6e146f8 Mon Sep 17 00:00:00 2001 From: "H.J. Lu" Date: Wed, 8 Apr 2020 09:47:35 -0700 Subject: [PATCH] x86: Insert ENDBR if function will be called indirectly Since constant_call_address_operand has ;; Test for a pc-relative call operand (define_predicate "constant_call_address_operand" (match_code "symbol_ref") { if (ix86_cmodel == CM_LARGE || ix86_cmodel == CM_LARGE_PIC || flag_force_indirect_call) return false; if (TARGET_DLLIMPORT_DECL_ATTRIBUTES && SYMBOL_REF_DLLIMPORT_P (op)) return false; return true; }) even if cgraph_node::get (cfun->decl)->only_called_directly_p () returns false, the fuction may still be called indirectly. Copy the logic from constant_call_address_operand to rest_of_insert_endbranch to insert ENDBR at function entry if function will be called indirectly. NB: gcc.target/i386/pr94417-2.c is updated to expect 4 ENDBRs, instead of 2, since only GCC 10 has the fix for PR target/89355 not to insert ENDBR after NOTE_INSN_DELETED_LABEL. gcc/ Backport from master PR target/94417 * config/i386/i386.c (rest_of_insert_endbranch): Insert ENDBR at function entry if function will be called indirectly. gcc/testsuite/ PR target/94417 * gcc.target/i386/pr94417-1.c: New test. * gcc.target/i386/pr94417-2.c: Likewise. * gcc.target/i386/pr94417-3.c: Likewise. (cherry picked from commit c5f379653964a1d2c7037b2de3e947a48370a198) --- gcc/config/i386/i386.c| 7 ++- gcc/testsuite/gcc.target/i386/pr94417-1.c | 20 gcc/testsuite/gcc.target/i386/pr94417-2.c | 21 + gcc/testsuite/gcc.target/i386/pr94417-3.c | 19 +++ 4 files changed, 66 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/gcc.target/i386/pr94417-1.c create mode 100644 gcc/testsuite/gcc.target/i386/pr94417-2.c create mode 100644 gcc/testsuite/gcc.target/i386/pr94417-3.c diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c index 5d12d82db5b..3891d44ce85 100644 --- a/gcc/config/i386/i386.c +++ b/gcc/config/i386/i386.c @@ -2535,7 +2535,12 @@ rest_of_insert_endbranch (void) && (!flag_manual_endbr || lookup_attribute ("cf_check", DECL_ATTRIBUTES (cfun->decl))) - && !cgraph_node::get (cfun->decl)->only_called_directly_p ()) + && (!cgraph_node::get (cfun->decl)->only_called_directly_p () + || ix86_cmodel == CM_LARGE + || ix86_cmodel == CM_LARGE_PIC + || flag_force_indirect_call + || (TARGET_DLLIMPORT_DECL_ATTRIBUTES + && DECL_DLLIMPORT_P (cfun->decl { /* Queue ENDBR insertion to x86_function_profiler. */ if (crtl->profile && flag_fentry) diff --git a/gcc/testsuite/gcc.target/i386/pr94417-1.c b/gcc/testsuite/gcc.target/i386/pr94417-1.c new file mode 100644 index 000..5bbe057fa8f --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/pr94417-1.c @@ -0,0 +1,20 @@ +/* { dg-do compile } */ +/* { dg-require-effective-target lp64 } */ +/* { dg-options "-O2 -fcf-protection -mcmodel=large" } */ +/* { dg-final { scan-assembler-times {\mendbr} 2 } } */ + +extern void ext (void); + +__attribute((noclone, noinline)) +static +void +foo (void) +{ + ext (); +} + +void +bar (void) +{ + foo (); +} diff --git a/gcc/testsuite/gcc.target/i386/pr94417-2.c b/gcc/testsuite/gcc.target/i386/pr944
Re: [PATCH] x86: Insert ENDBR if function will be called indirectly
On Fri, 2020-04-17 at 08:18 -0700, H.J. Lu wrote: > On Wed, Apr 8, 2020 at 9:41 AM Jeff Law wrote: > > On Wed, 2020-04-08 at 09:23 -0700, H.J. Lu wrote: > > > On Wed, Apr 8, 2020 at 9:16 AM Jeff Law wrote: > > > > On Tue, 2020-03-31 at 08:11 -0700, H.J. Lu via Gcc-patches wrote: > > > > > Since constant_call_address_operand has > > > > > > > > > > ;; Test for a pc-relative call operand > > > > > (define_predicate "constant_call_address_operand" > > > > > (match_code "symbol_ref") > > > > > { > > > > > if (ix86_cmodel == CM_LARGE || ix86_cmodel == CM_LARGE_PIC > > > > > || flag_force_indirect_call) > > > > > return false; > > > > > if (TARGET_DLLIMPORT_DECL_ATTRIBUTES && SYMBOL_REF_DLLIMPORT_P (op)) > > > > > return false; > > > > > return true; > > > > > }) > > > > > > > > > > even if cgraph_node::get (cfun->decl)->only_called_directly_p () > > > > > returns > > > > > false, the fuction may still be called indirectly. Copy the logic > > > > > from > > > > > constant_call_address_operand to rest_of_insert_endbranch to insert > > > > > ENDBR > > > > > at function entry if function will be called indirectly. > > > > > > > > > > gcc/ > > > > > > > > > > PR target/94417 > > > > > * config/i386/i386-features.c (rest_of_insert_endbranch): Insert > > > > > ENDBR at function entry if function will be called indirectly. > > > > Can you just call constant_call_address_operand rather than copying its > > > > contents? > > > > > > I wish I could. constant_call_address_operand uses SYMBOL_REF_DLLIMPORT_P > > > (op) > > > But I need to use DECL_DLLIMPORT_P (cfun->decl)). > > Sigh. In that case I guess the patch is OK as-is. > > > > I'd like to backport this wrong code fix to GCC 9/8 branches. > Is it OK for GCC 9/8 branches? Sure. jeff