Patchwork [asan] Disallow crossjumping of __asan_report_* builtins

login
register
mail settings
Submitter Jakub Jelinek
Date Dec. 5, 2012, 11:39 a.m.
Message ID <20121205113905.GN2315@tucnak.redhat.com>
Download mbox | patch
Permalink /patch/203857/
State New
Headers show

Comments

Jakub Jelinek - Dec. 5, 2012, 11:39 a.m.
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.

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.


	Jakub
Konstantin Serebryany - Dec. 5, 2012, 11:48 a.m.
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
Xinliang David Li - Dec. 5, 2012, 4:24 p.m.
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
Jakub Jelinek - Dec. 5, 2012, 4:35 p.m.
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
Xinliang David Li - Dec. 5, 2012, 5:32 p.m.
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

Patch

--- 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