diff mbox

Fix build of jit (was Re: [PATCH, RFC] Introduce -fsanitize=use-after-scope (v3))

Message ID 1478534833.7673.11.camel@redhat.com
State New
Headers show

Commit Message

David Malcolm Nov. 7, 2016, 4:07 p.m. UTC 
On Mon, 2016-11-07 at 11:03 +0100, Martin Liška wrote:
> Hello.
> 
> After discussion with Jakub, I'm resending new version of the patch,
> where I changed following:
> 1) gimplify_ctxp->live_switch_vars is used to track variables
> introduced in switch_expr. Every time
>    a case_label_expr is seen, these are unpoisoned. It's quite
> conservative, however it covers all
>    corner cases on can come up with. Compared to clang, we are much
> more precise in switch statements
>    where a variable liveness crosses label boundary.
> 2) I found a bug where ASAN_CHECK was optimized out due to missing
> check of IFN_ASAN_MARK internal fn.
>    Test was added for that.
> 3) Multiple switch tests have been added, which is going to be sent
> in upcoming email.
> 
> Patch can bootstrap on ppc64le-redhat-linux and survives regression
> tests (+ asan bootstrap finishes
> successfully).

The patch (r241896) introduced an error in the build of the jit:

../../src/gcc/jit/jit-builtins.c:62:1: error: invalid conversion from
‘int’ to ‘gcc::jit::built_in_attribute’ [-fpermissive]
 };
 ^

which seems to be due to the "0" for ATTRS in:

sanitizer code).

Dave

Comments

Jakub Jelinek Nov. 7, 2016, 4:17 p.m. UTC | #1 
On Mon, Nov 07, 2016 at 11:07:13AM -0500, David Malcolm wrote:
> The patch (r241896) introduced an error in the build of the jit:
> 
> ../../src/gcc/jit/jit-builtins.c:62:1: error: invalid conversion from
> ‘int’ to ‘gcc::jit::built_in_attribute’ [-fpermissive]
>  };
>  ^
> 
> which seems to be due to the "0" for ATTRS in:
> 
> --- a/gcc/sanitizer.def
> +++ b/gcc/sanitizer.def
> @@ -165,6 +165,10 @@ DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_BEFORE_DYNAMIC_INIT,
>  DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_AFTER_DYNAMIC_INIT,
>  		      "__asan_after_dynamic_init",
>  		      BT_FN_VOID, ATTR_NOTHROW_LEAF_LIST)
> +DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_CLOBBER_N, "__asan_poison_stack_memory",
> +		      BT_FN_VOID_PTR_PTRMODE, 0)
> +DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_UNCLOBBER_N, "__asan_unpoison_stack_memory",
> +		      BT_FN_VOID_PTR_PTRMODE, 0)

I believe the 0 here is a bug, I'd think we should be using something like
ATTR_TMPURE_NOTHROW_LEAF_LIST that we are using __asan_load* - the functions
aren't going to throw, nor call anything in the current TU.  Not 100% sure
about the TMPURE, after all they do write/read memory (the shadow one).
So maybe ATTR_NOTHROW_LEAF_LIST instead for now?  Martin?

> Is the attached patch OK as a fix? (assuming testing passes)  Or should
> these builtins have other attrs?  (sorry, am not very familiar with the
> sanitizer code).

	Jakub
Martin Liška Nov. 8, 2016, 9:38 a.m. UTC | #2 
On 11/07/2016 05:17 PM, Jakub Jelinek wrote:
> On Mon, Nov 07, 2016 at 11:07:13AM -0500, David Malcolm wrote:
>> The patch (r241896) introduced an error in the build of the jit:
>>
>> ../../src/gcc/jit/jit-builtins.c:62:1: error: invalid conversion from
>> ‘int’ to ‘gcc::jit::built_in_attribute’ [-fpermissive]
>>  };
>>  ^
>>
>> which seems to be due to the "0" for ATTRS in:
>>
>> --- a/gcc/sanitizer.def
>> +++ b/gcc/sanitizer.def
>> @@ -165,6 +165,10 @@ DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_BEFORE_DYNAMIC_INIT,
>>  DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_AFTER_DYNAMIC_INIT,
>>  		      "__asan_after_dynamic_init",
>>  		      BT_FN_VOID, ATTR_NOTHROW_LEAF_LIST)
>> +DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_CLOBBER_N, "__asan_poison_stack_memory",
>> +		      BT_FN_VOID_PTR_PTRMODE, 0)
>> +DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_UNCLOBBER_N, "__asan_unpoison_stack_memory",
>> +		      BT_FN_VOID_PTR_PTRMODE, 0)
> 
> I believe the 0 here is a bug, I'd think we should be using something like
> ATTR_TMPURE_NOTHROW_LEAF_LIST that we are using __asan_load* - the functions
> aren't going to throw, nor call anything in the current TU.  Not 100% sure
> about the TMPURE, after all they do write/read memory (the shadow one).
> So maybe ATTR_NOTHROW_LEAF_LIST instead for now?  Martin?

Yes, 0 is bug. I'm inclining to ATTR_NOTHROW_LEAF_LIST as __asan_{un}poison_stack_memory
modifies global memory. It would be more safe. I'm also going to change it for ASAN_MARK
internal function (where ECF_TM_PURE is currently selected).

I'm testing patch for that.
Martin

> 
>> Is the attached patch OK as a fix? (assuming testing passes)  Or should
>> these builtins have other attrs?  (sorry, am not very familiar with the
>> sanitizer code).
> 
> 	Jakub
>
Jakub Jelinek Nov. 8, 2016, 9:40 a.m. UTC | #3 
On Tue, Nov 08, 2016 at 10:38:23AM +0100, Martin Liška wrote:
> > I believe the 0 here is a bug, I'd think we should be using something like
> > ATTR_TMPURE_NOTHROW_LEAF_LIST that we are using __asan_load* - the functions
> > aren't going to throw, nor call anything in the current TU.  Not 100% sure
> > about the TMPURE, after all they do write/read memory (the shadow one).
> > So maybe ATTR_NOTHROW_LEAF_LIST instead for now?  Martin?
> 
> Yes, 0 is bug. I'm inclining to ATTR_NOTHROW_LEAF_LIST as __asan_{un}poison_stack_memory
> modifies global memory. It would be more safe. I'm also going to change it for ASAN_MARK
> internal function (where ECF_TM_PURE is currently selected).

The TM stuff needs to be eventually resolved with the TM maintainers
(Richard Henderson and Torvald Riegel), the thing is that we can annotate
stuff even in TM regions, tm_pure functions etc.  I believe we have lots of
other TM issues (internal calls and the like) that haven't been addressed.

	Jakub
diff mbox

Patch

From 6db5f9e50dc95f504d33970ee553172bbf400ae7 Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalcolm@redhat.com>
Date: Mon, 7 Nov 2016 11:21:20 -0500
Subject: [PATCH] Fix build of jit

gcc/ChangeLog:
	* asan.c (ATTR_NULL): Define.
	* sanitizer.def (BUILT_IN_ASAN_CLOBBER_N): Use ATTR_NULL rather
	than 0.
	(BUILT_IN_ASAN_UNCLOBBER_N): Likewise.
---
 gcc/asan.c        | 2 ++
 gcc/sanitizer.def | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/gcc/asan.c b/gcc/asan.c
index 1e0ce8d..4a124cb 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -2463,6 +2463,8 @@  initialize_sanitizer_builtins (void)
 #define BT_FN_I16_CONST_VPTR_INT BT_FN_IX_CONST_VPTR_INT[4]
 #define BT_FN_I16_VPTR_I16_INT BT_FN_IX_VPTR_IX_INT[4]
 #define BT_FN_VOID_VPTR_I16_INT BT_FN_VOID_VPTR_IX_INT[4]
+#undef ATTR_NULL
+#define ATTR_NULL 0
 #undef ATTR_NOTHROW_LEAF_LIST
 #define ATTR_NOTHROW_LEAF_LIST ECF_NOTHROW | ECF_LEAF
 #undef ATTR_TMPURE_NOTHROW_LEAF_LIST
diff --git a/gcc/sanitizer.def b/gcc/sanitizer.def
index 1c142e9..596b8b0 100644
--- a/gcc/sanitizer.def
+++ b/gcc/sanitizer.def
@@ -166,9 +166,9 @@  DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_AFTER_DYNAMIC_INIT,
 		      "__asan_after_dynamic_init",
 		      BT_FN_VOID, ATTR_NOTHROW_LEAF_LIST)
 DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_CLOBBER_N, "__asan_poison_stack_memory",
-		      BT_FN_VOID_PTR_PTRMODE, 0)
+		      BT_FN_VOID_PTR_PTRMODE, ATTR_NULL)
 DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_UNCLOBBER_N, "__asan_unpoison_stack_memory",
-		      BT_FN_VOID_PTR_PTRMODE, 0)
+		      BT_FN_VOID_PTR_PTRMODE, ATTR_NULL)
 
 /* Thread Sanitizer */
 DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_INIT, "__tsan_init", 
-- 
1.8.5.3