diff mbox

[asan] Handle noreturn calls with __asan_handle_no_return ()

Message ID 20121205144952.GV2315@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek Dec. 5, 2012, 2:49 p.m. UTC
Hi!

This patch fixes

-FAIL: g++.dg/asan/asan_test.C  -O2  AddressSanitizer_LongJmpTest execution test
-FAIL: g++.dg/asan/asan_test.C  -O2  AddressSanitizer_BuiltinLongJmpTest execution test
-FAIL: g++.dg/asan/asan_test.C  -O2  AddressSanitizer_SigLongJmpTest execution test

(both x86_64 and i686), and also fixes the clone-test-1.c testcase that
succeeded only because it had dg-shouldfail when it should have.

Unfortunately, it also regresses

+FAIL: g++.dg/asan/asan_test.C  -O2  AddressSanitizer_PthreadExitTest execution test

but that looks like a library problem to me:

AddressSanitizer CHECK failed: ../../../../../libsanitizer/asan/asan_rtl.cc:271 "((curr_thread)) != (0)" (0x0, 0x0)

Ok for trunk?

2012-12-05  Jakub Jelinek  <jakub@redhat.com>

	* sanitizer.def (BUILT_IN_ASAN_HANDLE_NO_RETURN): New builtin.
	* asan.c (instrument_builtin_call): Change is_gimple_builtin_call
	gcc_assert to gcc_checking_assert.
	(maybe_instrument_call): Imit __builtin___asan_handle_no_return ()
	before noreturn calls other than __builtin_trap () and
	__builtin_unreachable ().

	* c-c++-common/asan/clone-test-1.c: Remove bogus dg-shouldfail.


	Jakub

Comments

Dodji Seketeli Dec. 10, 2012, 9:44 p.m. UTC | #1
Jakub Jelinek <jakub@redhat.com> writes:

> +++ gcc/asan.c	2012-12-05 15:30:56.069890542 +0100
> @@ -1031,7 +1031,7 @@ instrument_builtin_call (gimple_stmt_ite
>  {
>    gimple call = gsi_stmt (*iter);
>  
> -  gcc_assert (is_gimple_builtin_call (call));
> +  gcc_checking_assert (is_gimple_builtin_call (call));

Why is this change necessary?

The patch looks OK to me otherwise.
Jakub Jelinek Dec. 10, 2012, 9:53 p.m. UTC | #2
On Mon, Dec 10, 2012 at 10:44:49PM +0100, Dodji Seketeli wrote:
> Jakub Jelinek <jakub@redhat.com> writes:
> 
> > +++ gcc/asan.c	2012-12-05 15:30:56.069890542 +0100
> > @@ -1031,7 +1031,7 @@ instrument_builtin_call (gimple_stmt_ite
> >  {
> >    gimple call = gsi_stmt (*iter);
> >  
> > -  gcc_assert (is_gimple_builtin_call (call));
> > +  gcc_checking_assert (is_gimple_builtin_call (call));
> 
> Why is this change necessary?

It is not necessary, just it isn't as low cost as it would be good for
an assertion, it does:
  if (is_gimple_call (stmt)
      && (callee = gimple_call_fndecl (stmt))
      && is_builtin_fn (callee)
      && DECL_BUILT_IN_CLASS (callee) == BUILT_IN_NORMAL)
    return true;
and the caller calls that exact routine already:
  if (is_gimple_builtin_call (gsi_stmt (*iter)))
    return instrument_builtin_call (iter);
and this is the second statement in instrument_builtin_call.

	Jakub
Dodji Seketeli Dec. 11, 2012, 8:26 a.m. UTC | #3
Jakub Jelinek <jakub@redhat.com> writes:

> On Mon, Dec 10, 2012 at 10:44:49PM +0100, Dodji Seketeli wrote:
>> Jakub Jelinek <jakub@redhat.com> writes:
>> 
>> > +++ gcc/asan.c	2012-12-05 15:30:56.069890542 +0100
>> > @@ -1031,7 +1031,7 @@ instrument_builtin_call (gimple_stmt_ite
>> >  {
>> >    gimple call = gsi_stmt (*iter);
>> >  
>> > -  gcc_assert (is_gimple_builtin_call (call));
>> > +  gcc_checking_assert (is_gimple_builtin_call (call));
>> 
>> Why is this change necessary?
>
> It is not necessary, just it isn't as low cost as it would be good for
> an assertion, it does:
>   if (is_gimple_call (stmt)
>       && (callee = gimple_call_fndecl (stmt))
>       && is_builtin_fn (callee)
>       && DECL_BUILT_IN_CLASS (callee) == BUILT_IN_NORMAL)
>     return true;
> and the caller calls that exact routine already:
>   if (is_gimple_builtin_call (gsi_stmt (*iter)))
>     return instrument_builtin_call (iter);
> and this is the second statement in instrument_builtin_call.

Ah, okay.  Thanks.
diff mbox

Patch

--- gcc/sanitizer.def.jj	2012-12-05 09:38:01.000000000 +0100
+++ gcc/sanitizer.def	2012-12-05 15:26:38.361427060 +0100
@@ -57,6 +57,9 @@  DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_REGI
 DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_UNREGISTER_GLOBALS,
 		      "__asan_unregister_globals",
 		      BT_FN_VOID_PTR_PTRMODE, ATTR_NOTHROW_LEAF_LIST)
+DEF_SANITIZER_BUILTIN(BUILT_IN_ASAN_HANDLE_NO_RETURN,
+		      "__asan_handle_no_return",
+		      BT_FN_VOID, ATTR_NOTHROW_LEAF_LIST)
 
 /* Thread Sanitizer */
 DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_INIT, "__tsan_init", 
--- gcc/asan.c.jj	2012-12-05 08:25:50.000000000 +0100
+++ gcc/asan.c	2012-12-05 15:30:56.069890542 +0100
@@ -1031,7 +1031,7 @@  instrument_builtin_call (gimple_stmt_ite
 {
   gimple call = gsi_stmt (*iter);
 
-  gcc_assert (is_gimple_builtin_call (call));
+  gcc_checking_assert (is_gimple_builtin_call (call));
 
   tree callee = gimple_call_fndecl (call);
   location_t loc = gimple_location (call);
@@ -1351,8 +1351,29 @@  instrument_assignment (gimple_stmt_itera
 static bool
 maybe_instrument_call (gimple_stmt_iterator *iter)
 {
-  if (is_gimple_builtin_call (gsi_stmt (*iter)))
-    return instrument_builtin_call (iter);
+  gimple stmt = gsi_stmt (*iter);
+  bool is_builtin = is_gimple_builtin_call (stmt);
+  if (is_builtin
+      && instrument_builtin_call (iter))
+    return true;
+  if (gimple_call_noreturn_p (stmt))
+    {
+      if (is_builtin)
+	{
+	  tree callee = gimple_call_fndecl (stmt);
+	  switch (DECL_FUNCTION_CODE (callee))
+	    {
+	    case BUILT_IN_UNREACHABLE:
+	    case BUILT_IN_TRAP:
+	      /* Don't instrument these.  */
+	      return false;
+	    }
+	}
+      tree decl = builtin_decl_implicit (BUILT_IN_ASAN_HANDLE_NO_RETURN);
+      gimple g = gimple_build_call (decl, 0);
+      gimple_set_location (g, gimple_location (stmt));
+      gsi_insert_before (iter, g, GSI_SAME_STMT);
+    }
   return false;
 }
 
--- gcc/testsuite/c-c++-common/asan/clone-test-1.c.jj	2012-12-05 11:04:12.000000000 +0100
+++ gcc/testsuite/c-c++-common/asan/clone-test-1.c	2012-12-05 15:41:51.386230638 +0100
@@ -4,7 +4,6 @@ 
 /* { dg-do run { target { *-*-linux* } } } */
 /* { dg-require-effective-target clone } */
 /* { dg-options "-D_GNU_SOURCE" } */
-/* { dg-shouldfail "asan" } */
 
 #include <stdio.h>
 #include <stdlib.h>