Message ID | 20200331151134.483137-1-hjl.tools@gmail.com |
---|---|
State | New |
Headers | show |
Series | x86: Insert ENDBR if function will be called indirectly | expand |
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 >
On Wed, Apr 8, 2020 at 9:16 AM Jeff Law <law@redhat.com> 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)).
On Wed, 2020-04-08 at 09:23 -0700, H.J. Lu wrote: > On Wed, Apr 8, 2020 at 9:16 AM Jeff Law <law@redhat.com> 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 >
On Wed, Apr 8, 2020 at 9:41 AM Jeff Law <law@redhat.com> 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 <law@redhat.com> 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.
On Fri, 2020-04-17 at 08:18 -0700, H.J. Lu wrote: > On Wed, Apr 8, 2020 at 9:41 AM Jeff Law <law@redhat.com> 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 <law@redhat.com> 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
diff --git a/gcc/config/i386/i386-features.c b/gcc/config/i386/i386-features.c index 66b120d21a7..78fb373db6e 100644 --- a/gcc/config/i386/i386-features.c +++ b/gcc/config/i386/i386-features.c @@ -1963,7 +1963,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 00000000000..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/pr94417-2.c new file mode 100644 index 00000000000..9eb0f5b2141 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/pr94417-2.c @@ -0,0 +1,21 @@ +/* { dg-do compile } */ +/* { dg-require-effective-target lp64 } */ +/* { dg-require-effective-target fpic } */ +/* { dg-options "-O2 -fpic -mcmodel=large -fcf-protection" } */ +/* { 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-3.c b/gcc/testsuite/gcc.target/i386/pr94417-3.c new file mode 100644 index 00000000000..07c451796c2 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/pr94417-3.c @@ -0,0 +1,19 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fcf-protection -mforce-indirect-call" } */ +/* { dg-final { scan-assembler-times {\mendbr} 2 } } */ + +extern void ext (void); + +__attribute((noclone, noinline)) +static +void +foo (void) +{ + ext (); +} + +void +bar (void) +{ + foo (); +}