| Message ID | 20121205113905.GN2315@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Dec 5, 2012 at 3:39 PM, Jakub Jelinek <jakub@redhat.com> wrote: > Hi! > > Another problem from the compiler side when working on the asan testsuite > is that at higher -O* levels the __asan_report_* noreturn calls are > cross-jumped, but the library relies on their locus to print accurrate > locations when symbolized. Without it, asan might report an error > completely elsewhere in a function. In LLVM we solve this problem by inserting an empty volatile asm blob after the __asan_report_* calls. Hacky, but works remarkably well. --kcc > > This patch disables cross-jumping of such builtins. > Alternatively, I could introduce some attribute (no_cross_jump?), > ECF_NO_CROSS_JUMP, add those attributes to those builtins (and let users > add those too to functions they wish), and look at ECF_NO_CROSS_JUMP instead > during cross-jumping. > > 2012-12-05 Jakub Jelinek <jakub@redhat.com> > > * sanitizer.def: Add comment about importance of ordering of > BUILT_IN_ASAN_REPORT* builtins. > * cfgcleanup.c (old_insns_match_p): Don't cross-jump __asan_report_* > builtins. > > --- gcc/sanitizer.def.jj 2012-12-04 14:19:36.000000000 +0100 > +++ gcc/sanitizer.def 2012-12-05 09:38:01.205958515 +0100 > @@ -29,6 +29,8 @@ along with GCC; see the file COPYING3. > /* Address Sanitizer */ > DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_INIT, "__asan_init", > BT_FN_VOID, ATTR_NOTHROW_LEAF_LIST) > +/* Do not reorder the BUILT_IN_ASAN_REPORT* builtins, e.g. cfgcleanup.c > + relies on this order. */ > DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_LOAD1, "__asan_report_load1", > BT_FN_VOID_PTR, ATTR_NORETURN_NOTHROW_LEAF_LIST) > DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_LOAD2, "__asan_report_load2", > --- gcc/cfgcleanup.c.jj 2012-11-20 09:37:47.000000000 +0100 > +++ gcc/cfgcleanup.c 2012-12-05 09:37:23.389181984 +0100 > @@ -1138,6 +1138,28 @@ old_insns_match_p (int mode ATTRIBUTE_UN > CALL_INSN_FUNCTION_USAGE (i2)) > || SIBLING_CALL_P (i1) != SIBLING_CALL_P (i2)) > return dir_none; > + > + /* For address sanitizer, never crossjump __asan_report_* builtins, > + otherwise errors might be reported on incorrect lines. */ > + if (flag_asan) > + { > + rtx call = get_call_rtx_from (i1); > + if (call && GET_CODE (XEXP (XEXP (call, 0), 0)) == SYMBOL_REF) > + { > + rtx symbol = XEXP (XEXP (call, 0), 0); > + if (SYMBOL_REF_DECL (symbol) > + && TREE_CODE (SYMBOL_REF_DECL (symbol)) == FUNCTION_DECL) > + { > + if ((DECL_BUILT_IN_CLASS (SYMBOL_REF_DECL (symbol)) > + == BUILT_IN_NORMAL) > + && DECL_FUNCTION_CODE (SYMBOL_REF_DECL (symbol)) > + >= BUILT_IN_ASAN_REPORT_LOAD1 > + && DECL_FUNCTION_CODE (SYMBOL_REF_DECL (symbol)) > + <= BUILT_IN_ASAN_REPORT_STORE16) > + return dir_none; > + } > + } > + } > } > > #ifdef STACK_REGS > > Jakub
Can the report builtins be marked with certain attribute such as 'no-return' etc? David On Wed, Dec 5, 2012 at 3:48 AM, Konstantin Serebryany <konstantin.s.serebryany@gmail.com> wrote: > On Wed, Dec 5, 2012 at 3:39 PM, Jakub Jelinek <jakub@redhat.com> wrote: >> Hi! >> >> Another problem from the compiler side when working on the asan testsuite >> is that at higher -O* levels the __asan_report_* noreturn calls are >> cross-jumped, but the library relies on their locus to print accurrate >> locations when symbolized. Without it, asan might report an error >> completely elsewhere in a function. > > In LLVM we solve this problem by inserting an empty volatile asm blob > after the __asan_report_* calls. > Hacky, but works remarkably well. > > --kcc > >> >> This patch disables cross-jumping of such builtins. >> Alternatively, I could introduce some attribute (no_cross_jump?), >> ECF_NO_CROSS_JUMP, add those attributes to those builtins (and let users >> add those too to functions they wish), and look at ECF_NO_CROSS_JUMP instead >> during cross-jumping. >> >> 2012-12-05 Jakub Jelinek <jakub@redhat.com> >> >> * sanitizer.def: Add comment about importance of ordering of >> BUILT_IN_ASAN_REPORT* builtins. >> * cfgcleanup.c (old_insns_match_p): Don't cross-jump __asan_report_* >> builtins. >> >> --- gcc/sanitizer.def.jj 2012-12-04 14:19:36.000000000 +0100 >> +++ gcc/sanitizer.def 2012-12-05 09:38:01.205958515 +0100 >> @@ -29,6 +29,8 @@ along with GCC; see the file COPYING3. >> /* Address Sanitizer */ >> DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_INIT, "__asan_init", >> BT_FN_VOID, ATTR_NOTHROW_LEAF_LIST) >> +/* Do not reorder the BUILT_IN_ASAN_REPORT* builtins, e.g. cfgcleanup.c >> + relies on this order. */ >> DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_LOAD1, "__asan_report_load1", >> BT_FN_VOID_PTR, ATTR_NORETURN_NOTHROW_LEAF_LIST) >> DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_LOAD2, "__asan_report_load2", >> --- gcc/cfgcleanup.c.jj 2012-11-20 09:37:47.000000000 +0100 >> +++ gcc/cfgcleanup.c 2012-12-05 09:37:23.389181984 +0100 >> @@ -1138,6 +1138,28 @@ old_insns_match_p (int mode ATTRIBUTE_UN >> CALL_INSN_FUNCTION_USAGE (i2)) >> || SIBLING_CALL_P (i1) != SIBLING_CALL_P (i2)) >> return dir_none; >> + >> + /* For address sanitizer, never crossjump __asan_report_* builtins, >> + otherwise errors might be reported on incorrect lines. */ >> + if (flag_asan) >> + { >> + rtx call = get_call_rtx_from (i1); >> + if (call && GET_CODE (XEXP (XEXP (call, 0), 0)) == SYMBOL_REF) >> + { >> + rtx symbol = XEXP (XEXP (call, 0), 0); >> + if (SYMBOL_REF_DECL (symbol) >> + && TREE_CODE (SYMBOL_REF_DECL (symbol)) == FUNCTION_DECL) >> + { >> + if ((DECL_BUILT_IN_CLASS (SYMBOL_REF_DECL (symbol)) >> + == BUILT_IN_NORMAL) >> + && DECL_FUNCTION_CODE (SYMBOL_REF_DECL (symbol)) >> + >= BUILT_IN_ASAN_REPORT_LOAD1 >> + && DECL_FUNCTION_CODE (SYMBOL_REF_DECL (symbol)) >> + <= BUILT_IN_ASAN_REPORT_STORE16) >> + return dir_none; >> + } >> + } >> + } >> } >> >> #ifdef STACK_REGS >> >> Jakub
On Wed, Dec 05, 2012 at 08:24:59AM -0800, Xinliang David Li wrote: > Can the report builtins be marked with certain attribute such as > 'no-return' etc? They are marked as noreturn obviously (and leaf, nothrow etc.). But noreturn isn't attribute that prevents cross-jumping. It is fine to cross-jump noreturn calls. On IRC Richard agreed that for 4.8 we can do this cfgcleanup.c hack (perhaps also in tree-ssa-tail-merge.c if we ever manage to get it to do tail merging of these), and for 4.9 go with a new attribute. Jakub
Ok. Tail/head merging optimization also happens after PRE. Though it happens before asan instrumentation, in theory it can trigger similar bogus loc problem, but less likely. David On Wed, Dec 5, 2012 at 8:35 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Wed, Dec 05, 2012 at 08:24:59AM -0800, Xinliang David Li wrote: >> Can the report builtins be marked with certain attribute such as >> 'no-return' etc? > > They are marked as noreturn obviously (and leaf, nothrow etc.). > But noreturn isn't attribute that prevents cross-jumping. It is fine > to cross-jump noreturn calls. > On IRC Richard agreed that for 4.8 we can do this cfgcleanup.c hack > (perhaps also in tree-ssa-tail-merge.c if we ever manage to get it to > do tail merging of these), and for 4.9 go with a new attribute. > > Jakub
--- gcc/sanitizer.def.jj 2012-12-04 14:19:36.000000000 +0100 +++ gcc/sanitizer.def 2012-12-05 09:38:01.205958515 +0100 @@ -29,6 +29,8 @@ along with GCC; see the file COPYING3. /* Address Sanitizer */ DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_INIT, "__asan_init", BT_FN_VOID, ATTR_NOTHROW_LEAF_LIST) +/* Do not reorder the BUILT_IN_ASAN_REPORT* builtins, e.g. cfgcleanup.c + relies on this order. */ DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_LOAD1, "__asan_report_load1", BT_FN_VOID_PTR, ATTR_NORETURN_NOTHROW_LEAF_LIST) DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REPORT_LOAD2, "__asan_report_load2", --- gcc/cfgcleanup.c.jj 2012-11-20 09:37:47.000000000 +0100 +++ gcc/cfgcleanup.c 2012-12-05 09:37:23.389181984 +0100 @@ -1138,6 +1138,28 @@ old_insns_match_p (int mode ATTRIBUTE_UN CALL_INSN_FUNCTION_USAGE (i2)) || SIBLING_CALL_P (i1) != SIBLING_CALL_P (i2)) return dir_none; + + /* For address sanitizer, never crossjump __asan_report_* builtins, + otherwise errors might be reported on incorrect lines. */ + if (flag_asan) + { + rtx call = get_call_rtx_from (i1); + if (call && GET_CODE (XEXP (XEXP (call, 0), 0)) == SYMBOL_REF) + { + rtx symbol = XEXP (XEXP (call, 0), 0); + if (SYMBOL_REF_DECL (symbol) + && TREE_CODE (SYMBOL_REF_DECL (symbol)) == FUNCTION_DECL) + { + if ((DECL_BUILT_IN_CLASS (SYMBOL_REF_DECL (symbol)) + == BUILT_IN_NORMAL) + && DECL_FUNCTION_CODE (SYMBOL_REF_DECL (symbol)) + >= BUILT_IN_ASAN_REPORT_LOAD1 + && DECL_FUNCTION_CODE (SYMBOL_REF_DECL (symbol)) + <= BUILT_IN_ASAN_REPORT_STORE16) + return dir_none; + } + } + } } #ifdef STACK_REGS