Re: [PATCH][RFC] Instrument function exit with __builtin_unreachable in C++.

["Martin Liška" <mliska@suse.cz>]

[-- Attachment #1: Type: text/plain, Size: 1589 bytes --]

On 10/12/2017 10:48 AM, Jakub Jelinek wrote:
> On Thu, Oct 12, 2017 at 10:40:42AM +0200, Martin Liška wrote:
>> --- a/gcc/cp/constexpr.c
>> +++ b/gcc/cp/constexpr.c
>> @@ -1175,7 +1175,12 @@ cxx_eval_builtin_function_call (const constexpr_ctx *ctx, tree t, tree fun,
>>  	{
>>  	  new_call = build_call_array_loc (EXPR_LOCATION (t), TREE_TYPE (t),
>>  					   CALL_EXPR_FN (t), nargs, args);
>> -	  error ("%q+E is not a constant expression", new_call);
>> +
>> +	  /* Do not allow__builtin_unreachable in constexpr function.  */
>> +	  if (DECL_FUNCTION_CODE (fun) == BUILT_IN_UNREACHABLE)
> 
> As I said earlier, I think it would be better to differentiate between
> explicit __builtin_unreachable and the implicitly added one from the patch.
> So this could be done as
> if (DECL_FUNCTION_CODE (fun) == BUILT_IN_UNREACHABLE
>     && EXPR_LOCATION (t) == BUILTINS_LOCATION)
> 
>> +  location_t loc = DECL_SOURCE_LOCATION (fndecl);
>> +  if (sanitize_flags_p (SANITIZE_RETURN, fndecl))
>> +    t = ubsan_instrument_return (loc);
>> +  else
>> +    t = build_call_expr_loc (loc, builtin_decl_explicit (BUILT_IN_UNREACHABLE),
> 
> and here use BUILTINS_LOCATION instead of loc.
> The code might be more readable by doing:
>     {
>       tree fndecl = builtin_decl_explicit (BUILT_IN_UNREACHABLE);
>       t = build_call_expr_loc (BUILTINS_LOCATION, fndecl, 0);
>     }
> 
>> +			     0);
>> +
> 
> 	Jakub
> 

Hi.

I'm sending updated version of the patch that should address it.

Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.

Ready to be installed?
Martin

[-- Attachment #2: 0001-Instrument-function-exit-with-__builtin_unreachable-.patch --]
[-- Type: text/x-patch, Size: 5158 bytes --]

From 36f3f45d9fa42344261faf60bb3cfbe22ed262ac Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Thu, 12 Oct 2017 10:14:59 +0200
Subject: [PATCH 1/3] Instrument function exit with __builtin_unreachable in
 C++

gcc/c-family/ChangeLog:

2017-10-12  Martin Liska  <mliska@suse.cz>

	PR middle-end/82404
	* c-opts.c (c_common_post_options): Set -Wreturn-type for C++
	FE.
	* c.opt: Set default value of warn_return_type.

gcc/cp/ChangeLog:

2017-10-12  Martin Liska  <mliska@suse.cz>

	PR middle-end/82404
	* constexpr.c (cxx_eval_builtin_function_call): Handle
	__builtin_unreachable call.
	* cp-gimplify.c (cp_ubsan_maybe_instrument_return): Rename to
	...
	(cp_maybe_instrument_return): ... this.
	(cp_genericize): Call the function unconditionally.

gcc/fortran/ChangeLog:

2017-10-12  Martin Liska  <mliska@suse.cz>

	PR middle-end/82404
	* options.c (gfc_post_options): Set default value of
	-Wreturn-type to false.
---
 gcc/c-family/c-opts.c |  3 +++
 gcc/c-family/c.opt    |  2 +-
 gcc/cp/constexpr.c    |  8 +++++++-
 gcc/cp/cp-gimplify.c  | 20 ++++++++++++++------
 gcc/fortran/options.c |  3 +++
 5 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/gcc/c-family/c-opts.c b/gcc/c-family/c-opts.c
index 6bd535532d3..682d7a83ec5 100644
--- a/gcc/c-family/c-opts.c
+++ b/gcc/c-family/c-opts.c
@@ -978,6 +978,9 @@ c_common_post_options (const char **pfilename)
 	flag_extern_tls_init = 1;
     }
 
+  if (warn_return_type == -1)
+    warn_return_type = c_dialect_cxx () ? 1 : 0;
+
   if (num_in_fnames > 1)
     error ("too many filenames given.  Type %s --help for usage",
 	   progname);
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 13d2a59b8a5..e26fba734c0 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -960,7 +960,7 @@ C++ ObjC++ Var(warn_reorder) Warning LangEnabledBy(C++ ObjC++,Wall)
 Warn when the compiler reorders code.
 
 Wreturn-type
-C ObjC C++ ObjC++ Var(warn_return_type) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
+C ObjC C++ ObjC++ Var(warn_return_type) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) Init(-1)
 Warn whenever a function's return type defaults to \"int\" (C), or about inconsistent return types (C++).
 
 Wscalar-storage-order
diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index 59192829d71..15253ffad9d 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -1182,7 +1182,13 @@ cxx_eval_builtin_function_call (const constexpr_ctx *ctx, tree t, tree fun,
 	{
 	  new_call = build_call_array_loc (EXPR_LOCATION (t), TREE_TYPE (t),
 					   CALL_EXPR_FN (t), nargs, args);
-	  error ("%q+E is not a constant expression", new_call);
+
+	  /* Do not allow__builtin_unreachable in constexpr function.  */
+	  if (DECL_FUNCTION_CODE (fun) == BUILT_IN_UNREACHABLE
+	      && EXPR_LOCATION (t) == BUILTINS_LOCATION)
+	    error ("constexpr call flows off the end of the function");
+	  else
+	    error ("%q+E is not a constant expression", new_call);
 	}
       *non_constant_p = true;
       return t;
diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c
index 262485a5c1f..014c1ee7231 100644
--- a/gcc/cp/cp-gimplify.c
+++ b/gcc/cp/cp-gimplify.c
@@ -1556,10 +1556,11 @@ cp_genericize_tree (tree* t_p, bool handle_invisiref_parm_p)
 
 /* If a function that should end with a return in non-void
    function doesn't obviously end with return, add ubsan
-   instrumentation code to verify it at runtime.  */
+   instrumentation code to verify it at runtime.  If -fsanitize=return
+   is not enabled, instrument __builtin_unreachable.  */
 
 static void
-cp_ubsan_maybe_instrument_return (tree fndecl)
+cp_maybe_instrument_return (tree fndecl)
 {
   if (VOID_TYPE_P (TREE_TYPE (TREE_TYPE (fndecl)))
       || DECL_CONSTRUCTOR_P (fndecl)
@@ -1600,7 +1601,16 @@ cp_ubsan_maybe_instrument_return (tree fndecl)
   tree *p = &DECL_SAVED_TREE (fndecl);
   if (TREE_CODE (*p) == BIND_EXPR)
     p = &BIND_EXPR_BODY (*p);
-  t = ubsan_instrument_return (DECL_SOURCE_LOCATION (fndecl));
+
+  location_t loc = DECL_SOURCE_LOCATION (fndecl);
+  if (sanitize_flags_p (SANITIZE_RETURN, fndecl))
+    t = ubsan_instrument_return (loc);
+  else
+    {
+      tree fndecl = builtin_decl_explicit (BUILT_IN_UNREACHABLE);
+      t = build_call_expr_loc (BUILTINS_LOCATION, fndecl, 0);
+    }
+
   append_to_statement_list (t, p);
 }
 
@@ -1674,9 +1684,7 @@ cp_genericize (tree fndecl)
      walk_tree's hash functionality.  */
   cp_genericize_tree (&DECL_SAVED_TREE (fndecl), true);
 
-  if (sanitize_flags_p (SANITIZE_RETURN)
-      && current_function_decl != NULL_TREE)
-    cp_ubsan_maybe_instrument_return (fndecl);
+  cp_maybe_instrument_return (fndecl);
 
   /* Do everything else.  */
   c_genericize (fndecl);
diff --git a/gcc/fortran/options.c b/gcc/fortran/options.c
index f7bbd7f2cde..59e7f028b09 100644
--- a/gcc/fortran/options.c
+++ b/gcc/fortran/options.c
@@ -430,6 +430,9 @@ gfc_post_options (const char **pfilename)
     gfc_fatal_error ("Maximum subrecord length cannot exceed %d",
 		     MAX_SUBRECORD_LENGTH);
 
+  if (warn_return_type == -1)
+    warn_return_type = 0;
+
   gfc_cpp_post_options ();
 
   if (gfc_option.allow_std & GFC_STD_F2008)
-- 
2.14.2