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

[Thomas Schwinge]

Hi!

After r254437 "Instrument function exit with __builtin_unreachable in
C++" (assuming that I bisected that correctly), I'm seeing a number of
C++ "ifunc" test cases regress from PASS to UNSUPPORTED.  This is because
of:

ifunc_available29518.c: In function 'void (* g())()':
ifunc_available29518.c:6:15: warning: no return statement in function 
returning non-void [-Wreturn-type]

This is not a problem in C testing, where that diagnostic explicitly
needs to be enabled whereas in C++ mode it's enabled by default.

OK to fix that as follows?  If approving this, please respond with
"Reviewed-by: NAME " so that your effort will be recorded.  See
.

diff --git gcc/testsuite/lib/target-supports.exp 
gcc/testsuite/lib/target-supports.exp
index d7ef04f..63cc75b 100644
--- gcc/testsuite/lib/target-supports.exp
+++ gcc/testsuite/lib/target-supports.exp
@@ -439,8 +439,9 @@ proc check_ifunc_available { } {
#ifdef __cplusplus
extern "C" {
#endif
+   extern void f_ ();
typedef void F (void);
-   F* g (void) {}
+   F* g (void) { return _; }
void f () __attribute__ ((ifunc ("g")));
#ifdef __cplusplus
}


Grüße
 Thomas

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

[Martin Liška]

On 11/15/2017 11:04 AM, Jakub Jelinek wrote:
> On Wed, Nov 15, 2017 at 10:54:23AM +0100, Martin Liška wrote:
>> gcc/c/ChangeLog:
>>
>> 2017-11-15  Martin Liska  
>>
>>  * c-decl.c (grokdeclarator):
>>  Compare warn_return_type for greater than zero.
>>  (start_function): Likewise.
>>  (finish_function): Likewise.
>>  * c-typeck.c (c_finish_return): Likewise.
>>
>> gcc/cp/ChangeLog:
>>
>> 2017-11-15  Martin Liska  
>>
>>  * decl.c (finish_function):
>>  Compare warn_return_type for greater than zero.
>>  * semantics.c (finish_return_stmt): Likewise.
> 
> The c/cp changes aren't really needed, are they?  Because
> in that case you guarantee in the post options handling it is
> 0 or 1.

Yep, you're right!

> 
> The rest looks good (except for Ada that Eric doesn't want to change).
> 
>   Jakub
> 


Done that and I'm going to install the patch.

Martin
>From c0934d0be85d40762d4bafbf9991b167b711736e Mon Sep 17 00:00:00 2001
From: marxin 
Date: Wed, 15 Nov 2017 09:16:23 +0100
Subject: [PATCH] Disable -Wreturn-type by default in all languages other from
 C++.

gcc/ChangeLog:

2017-11-15  Martin Liska  

	* tree-cfg.c (pass_warn_function_return::execute):
	Compare warn_return_type for greater than zero.

gcc/fortran/ChangeLog:

2017-11-15  Martin Liska  

	* options.c (gfc_post_options):
	Do not set default value of warn_return_type.
	* trans-decl.c (gfc_trans_deferred_vars):
	Compare warn_return_type for greater than zero.
	(generate_local_decl): Likewise
	(gfc_generate_function_code): Likewise.
---
 gcc/fortran/options.c| 3 ---
 gcc/fortran/trans-decl.c | 8 
 gcc/tree-cfg.c   | 2 +-
 3 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/gcc/fortran/options.c b/gcc/fortran/options.c
index c584a19e559..0ee6b7808d9 100644
--- a/gcc/fortran/options.c
+++ b/gcc/fortran/options.c
@@ -435,9 +435,6 @@ 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)
diff --git a/gcc/fortran/trans-decl.c b/gcc/fortran/trans-decl.c
index 8efaae79ebc..60e7d8f79ee 100644
--- a/gcc/fortran/trans-decl.c
+++ b/gcc/fortran/trans-decl.c
@@ -4198,7 +4198,7 @@ gfc_trans_deferred_vars (gfc_symbol * proc_sym, gfc_wrapped_block * block)
 		  break;
 	}
 	  /* TODO: move to the appropriate place in resolve.c.  */
-	  if (warn_return_type && el == NULL)
+	  if (warn_return_type > 0 && el == NULL)
 	gfc_warning (OPT_Wreturn_type,
 			 "Return value of function %qs at %L not set",
 			 proc_sym->name, _sym->declared_at);
@@ -5619,7 +5619,7 @@ generate_local_decl (gfc_symbol * sym)
   else if (sym->attr.flavor == FL_PROCEDURE)
 {
   /* TODO: move to the appropriate place in resolve.c.  */
-  if (warn_return_type
+  if (warn_return_type > 0
 	  && sym->attr.function
 	  && sym->result
 	  && sym != sym->result
@@ -6494,11 +6494,11 @@ gfc_generate_function_code (gfc_namespace * ns)
   if (result == NULL_TREE || artificial_result_decl)
 	{
 	  /* TODO: move to the appropriate place in resolve.c.  */
-	  if (warn_return_type && sym == sym->result)
+	  if (warn_return_type > 0 && sym == sym->result)
 	gfc_warning (OPT_Wreturn_type,
 			 "Return value of function %qs at %L not set",
 			 sym->name, >declared_at);
-	  if (warn_return_type)
+	  if (warn_return_type > 0)
 	TREE_NO_WARNING(sym->backend_decl) = 1;
 	}
   if (result != NULL_TREE)
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index 9a2fa1d98ca..f08a0547f0f 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -9071,7 +9071,7 @@ pass_warn_function_return::execute (function *fun)
 
   /* If we see "return;" in some basic block, then we do reach the end
  without returning a value.  */
-  else if (warn_return_type
+  else if (warn_return_type > 0
 	   && !TREE_NO_WARNING (fun->decl)
 	   && EDGE_COUNT (EXIT_BLOCK_PTR_FOR_FN (fun)->preds) > 0
 	   && !VOID_TYPE_P (TREE_TYPE (TREE_TYPE (fun->decl
-- 
2.14.3

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

[Jakub Jelinek]

On Wed, Nov 15, 2017 at 10:54:23AM +0100, Martin Liška wrote:
> gcc/c/ChangeLog:
> 
> 2017-11-15  Martin Liska  
> 
>   * c-decl.c (grokdeclarator):
>   Compare warn_return_type for greater than zero.
>   (start_function): Likewise.
>   (finish_function): Likewise.
>   * c-typeck.c (c_finish_return): Likewise.
> 
> gcc/cp/ChangeLog:
> 
> 2017-11-15  Martin Liska  
> 
>   * decl.c (finish_function):
>   Compare warn_return_type for greater than zero.
>   * semantics.c (finish_return_stmt): Likewise.

The c/cp changes aren't really needed, are they?  Because
in that case you guarantee in the post options handling it is
0 or 1.

The rest looks good (except for Ada that Eric doesn't want to change).

Jakub

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

[Eric Botcazou]

> Following patch survives regression tests and bootstraps.

Please drop the Ada bits though, -Wreturn-type just doesn't work in Ada.

-- 
Eric Botcazou

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

[Martin Liška]

On 11/15/2017 10:42 AM, Eric Botcazou wrote:
>> But we don't.  Wonder if in addition to your patch or instead of it it
>> wouldn't be safer (especially for FEs added in the future) to:
>>
>>/* If we see "return;" in some basic block, then we do reach the end
>>   without returning a value.  */
>> -  else if (warn_return_type
>> +  else if (warn_return_type > 0
>> && !TREE_NO_WARNING (fun->decl)
>> && EDGE_COUNT (EXIT_BLOCK_PTR_FOR_FN (fun)->preds) > 0
>> && !VOID_TYPE_P (TREE_TYPE (TREE_TYPE (fun->decl
>>
>> in tree-cfg.c.  That change is preapproved if it works, and your
>> patch if you want in addition to that is ok too.
> 
> That's the first thing I tried and it indeed works.
> 

Hi.

Following patch survives regression tests and bootstraps.
There are multiple places where warn_return_type should be compared
to zero.

Ready for trunk?
Thanks,
Martin
>From 5f8daccc584c7ae749d25d59526e0173aa4334f7 Mon Sep 17 00:00:00 2001
From: marxin 
Date: Wed, 15 Nov 2017 09:16:23 +0100
Subject: [PATCH] Disable -Wreturn-type by default in all languages other from
 C++.

gcc/ChangeLog:

2017-11-15  Martin Liska  

	* tree-cfg.c (pass_warn_function_return::execute):
	Compare warn_return_type for greater than zero.

gcc/ada/ChangeLog:

2017-11-15  Martin Liska  

	* gcc-interface/misc.c (gnat_post_options):
	Do not set default value of warn_return_type.

gcc/c/ChangeLog:

2017-11-15  Martin Liska  

	* c-decl.c (grokdeclarator):
	Compare warn_return_type for greater than zero.
	(start_function): Likewise.
	(finish_function): Likewise.
	* c-typeck.c (c_finish_return): Likewise.

gcc/cp/ChangeLog:

2017-11-15  Martin Liska  

	* decl.c (finish_function):
	Compare warn_return_type for greater than zero.
	* semantics.c (finish_return_stmt): Likewise.

gcc/fortran/ChangeLog:

2017-11-15  Martin Liska  

	* options.c (gfc_post_options):
	Do not set default value of warn_return_type.
	* trans-decl.c (gfc_trans_deferred_vars):
	Compare warn_return_type for greater than zero.
	(generate_local_decl): Likewise
	(gfc_generate_function_code): Likewise.
---
 gcc/ada/gcc-interface/misc.c | 3 ---
 gcc/c/c-decl.c   | 6 +++---
 gcc/c/c-typeck.c | 2 +-
 gcc/cp/decl.c| 2 +-
 gcc/cp/semantics.c   | 2 +-
 gcc/fortran/options.c| 3 ---
 gcc/fortran/trans-decl.c | 8 
 gcc/tree-cfg.c   | 2 +-
 8 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/gcc/ada/gcc-interface/misc.c b/gcc/ada/gcc-interface/misc.c
index 9a4a48fba42..7bdb3803c13 100644
--- a/gcc/ada/gcc-interface/misc.c
+++ b/gcc/ada/gcc-interface/misc.c
@@ -262,9 +262,6 @@ gnat_post_options (const char **pfilename ATTRIBUTE_UNUSED)
   /* No psABI change warnings for Ada.  */
   warn_psabi = 0;
 
-  /* No return type warnings for Ada.  */
-  warn_return_type = 0;
-
   /* No caret by default for Ada.  */
   if (!global_options_set.x_flag_diagnostics_show_caret)
 global_dc->show_caret = false;
diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
index d95a2b6ea4f..7120420f2df 100644
--- a/gcc/c/c-decl.c
+++ b/gcc/c/c-decl.c
@@ -5689,7 +5689,7 @@ grokdeclarator (const struct c_declarator *declarator,
   /* Issue a warning if this is an ISO C 99 program or if
 	 -Wreturn-type and this is a function, or if -Wimplicit;
 	 prefer the former warning since it is more explicit.  */
-  if ((warn_implicit_int || warn_return_type || flag_isoc99)
+  if ((warn_implicit_int || warn_return_type > 0 || flag_isoc99)
 	  && funcdef_flag)
 	warn_about_return_type = 1;
   else
@@ -8655,7 +8655,7 @@ start_function (struct c_declspecs *declspecs, struct c_declarator *declarator,
 
   if (warn_about_return_type)
 warn_defaults_to (loc, flag_isoc99 ? OPT_Wimplicit_int
-			   : (warn_return_type ? OPT_Wreturn_type
+			   : (warn_return_type > 0 ? OPT_Wreturn_type
 			  : OPT_Wimplicit_int),
 		  "return type defaults to %");
 
@@ -9373,7 +9373,7 @@ finish_function (void)
   finish_fname_decls ();
 
   /* Complain if there's just no return statement.  */
-  if (warn_return_type
+  if (warn_return_type > 0
   && TREE_CODE (TREE_TYPE (TREE_TYPE (fndecl))) != VOID_TYPE
   && !current_function_returns_value && !current_function_returns_null
   /* Don't complain if we are no-return.  */
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 4bdc48a9ea3..492a245d296 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -10091,7 +10091,7 @@ c_finish_return (location_t loc, tree retval, tree origtype)
   if (!retval)
 {
   current_function_returns_null = 1;
-  if ((warn_return_type || flag_isoc99)
+  if ((warn_return_type > 0 || flag_isoc99)
 	  && valtype != NULL_TREE && TREE_CODE (valtype) != VOID_TYPE)
 	{
 	  bool warned_here;
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 041893db937..96bbff6c1f9 100644
--- a/gcc/cp/decl.c
+++ 

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

[Eric Botcazou]

> But we don't.  Wonder if in addition to your patch or instead of it it
> wouldn't be safer (especially for FEs added in the future) to:
> 
>/* If we see "return;" in some basic block, then we do reach the end
>   without returning a value.  */
> -  else if (warn_return_type
> +  else if (warn_return_type > 0
> && !TREE_NO_WARNING (fun->decl)
> && EDGE_COUNT (EXIT_BLOCK_PTR_FOR_FN (fun)->preds) > 0
> && !VOID_TYPE_P (TREE_TYPE (TREE_TYPE (fun->decl
> 
> in tree-cfg.c.  That change is preapproved if it works, and your
> patch if you want in addition to that is ok too.

That's the first thing I tried and it indeed works.

-- 
Eric Botcazou

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

[Jakub Jelinek]

On Tue, Nov 07, 2017 at 11:08:58AM +0100, Martin Liška wrote:
> > Hasn't it enabled it also for any other FEs other than C family and Fortran?
> > Say jit, brig, go, lto?, ...
> > I think better would be to remove the initialization to -1 and revert the
> > fortran/options.c change, and instead use in the C family:
> >   if (!global_options_set.x_warn_return_type)
> > warn_return_type = c_dialect_cxx ();
> > 
> > Unless it for some reason doesn't work for -Wall or -W or similar.
> > 
> 
> Hello.
> 
> Sorry for the inconvenience, however using Jakub's approach really does not 
> work properly
> with -Wall.

If -Wall had an underlying variable, then we could use:
  if (!global_options_set.x_warn_return_type
  && !global_options_set.x_warn_all)
warn_return_type = c_dialect_cxx ();

But we don't.  Wonder if in addition to your patch or instead of it it
wouldn't be safer (especially for FEs added in the future) to:
 
   /* If we see "return;" in some basic block, then we do reach the end
  without returning a value.  */
-  else if (warn_return_type
+  else if (warn_return_type > 0
&& !TREE_NO_WARNING (fun->decl)
&& EDGE_COUNT (EXIT_BLOCK_PTR_FOR_FN (fun)->preds) > 0
&& !VOID_TYPE_P (TREE_TYPE (TREE_TYPE (fun->decl

in tree-cfg.c.  That change is preapproved if it works, and your
patch if you want in addition to that is ok too.

Jakub

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

[Martin Liška]

PING^1

On 11/07/2017 11:08 AM, Martin Liška wrote:
> On 11/06/2017 06:33 PM, Jakub Jelinek wrote:
>> On Mon, Nov 06, 2017 at 06:23:11PM +0100, Eric Botcazou wrote:
 Thank you for review, done that.
>>>
>>> This has enabled -Wreturn-type for Ada, what we don't want since the 
>>> warning 
>>> is outsmarted by the language, so I have applied this.
>>>
>>>
>>> 2017-11-06  Eric Botcazou  
>>>
>>> * gcc-interface/misc.c (gnat_post_options): Clear warn_return_type.
>>
>> Hasn't it enabled it also for any other FEs other than C family and Fortran?
>> Say jit, brig, go, lto?, ...
>> I think better would be to remove the initialization to -1 and revert the
>> fortran/options.c change, and instead use in the C family:
>>   if (!global_options_set.x_warn_return_type)
>> warn_return_type = c_dialect_cxx ();
>>
>> Unless it for some reason doesn't work for -Wall or -W or similar.
>>
>>  Jakub
>>
> 
> Hello.
> 
> Sorry for the inconvenience, however using Jakub's approach really does not 
> work properly
> with -Wall.
> 
> Thus I'm suggesting following patch that will disable it in all 
> *_post_options hooks.
> 
> Ready after it survives regression tests?
> Martin
> 

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

[Martin Liška]

On 11/07/2017 06:15 PM, Andreas Schwab wrote:

This breaks g++.dg/torture/pr64669.C with -O3 on ia64:

$ gcc/xg++ -Bgcc/ ../../gcc/gcc/testsuite/g++.dg/torture/pr64669.C -nostdinc++ 
-Iia64-suse-linux/libstdc++-v3/include/ia64-suse-linux 
-Iia64-suse-linux/libstdc++-v3/include -I../libstdc++-v3/libsupc++ 
-I../libstdc++-v3/include/backward -I../libstdc++-v3/testsuite/util -O3 -S -o 
pr64669.s
../../gcc/gcc/testsuite/g++.dg/torture/pr64669.C: In member function ‘const 
char* Lex::advance_one_char(const char*, bool, unsigned int*, bool*)’:
../../gcc/gcc/testsuite/g++.dg/torture/pr64669.C:65:1: error: qsort comparator 
non-negative on sorted output: 1
  }
  ^
during RTL pass: mach
../../gcc/gcc/testsuite/g++.dg/torture/pr64669.C:65:1: internal compiler error: 
qsort checking failed
0x401cc80f qsort_chk_error
 ../../gcc/vec.c:222
0x425d20cf qsort_chk(void*, unsigned long, unsigned long, int (*)(void 
const*, void const*))
 ../../gcc/vec.c:274
0x414f680f vec<_expr*, va_heap, vl_embed>::qsort(int (*)(void const*, 
void const*))
 ../../gcc/vec.h:973
0x414f680f vec<_expr*, va_heap, vl_ptr>::qsort(int (*)(void const*, 
void const*))
 ../../gcc/vec.h:1735
0x414f680f fill_vec_av_set
 ../../gcc/sel-sched.c:3725
0x414fc59f fill_ready_list
 ../../gcc/sel-sched.c:4022
0x414fc59f find_best_expr
 ../../gcc/sel-sched.c:4382
0x414fc59f fill_insns
 ../../gcc/sel-sched.c:5539
0x414fc59f schedule_on_fences
 ../../gcc/sel-sched.c:7356
0x414fc59f sel_sched_region_2
 ../../gcc/sel-sched.c:7494
0x41503acf sel_sched_region_1
 ../../gcc/sel-sched.c:7536
0x41503acf sel_sched_region(int)
 ../../gcc/sel-sched.c:7637
0x41504e6f run_selective_scheduling()
 ../../gcc/sel-sched.c:7713
0x41df9bdf ia64_reorg
 ../../gcc/config/ia64/ia64.c:9854
0x4146d40f execute
 ../../gcc/reorg.c:3947

Andreas.



Hi Andreas.

That will be very probably dup of PR82398.

Martin

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

[Andreas Schwab]

This breaks g++.dg/torture/pr64669.C with -O3 on ia64:

$ gcc/xg++ -Bgcc/ ../../gcc/gcc/testsuite/g++.dg/torture/pr64669.C -nostdinc++ 
-Iia64-suse-linux/libstdc++-v3/include/ia64-suse-linux 
-Iia64-suse-linux/libstdc++-v3/include -I../libstdc++-v3/libsupc++ 
-I../libstdc++-v3/include/backward -I../libstdc++-v3/testsuite/util -O3 -S -o 
pr64669.s   
../../gcc/gcc/testsuite/g++.dg/torture/pr64669.C: In member function ‘const 
char* Lex::advance_one_char(const char*, bool, unsigned int*, bool*)’:
../../gcc/gcc/testsuite/g++.dg/torture/pr64669.C:65:1: error: qsort comparator 
non-negative on sorted output: 1
 }
 ^
during RTL pass: mach
../../gcc/gcc/testsuite/g++.dg/torture/pr64669.C:65:1: internal compiler error: 
qsort checking failed
0x401cc80f qsort_chk_error
../../gcc/vec.c:222
0x425d20cf qsort_chk(void*, unsigned long, unsigned long, int (*)(void 
const*, void const*))
../../gcc/vec.c:274
0x414f680f vec<_expr*, va_heap, vl_embed>::qsort(int (*)(void const*, 
void const*))
../../gcc/vec.h:973
0x414f680f vec<_expr*, va_heap, vl_ptr>::qsort(int (*)(void const*, 
void const*))
../../gcc/vec.h:1735
0x414f680f fill_vec_av_set
../../gcc/sel-sched.c:3725
0x414fc59f fill_ready_list
../../gcc/sel-sched.c:4022
0x414fc59f find_best_expr
../../gcc/sel-sched.c:4382
0x414fc59f fill_insns
../../gcc/sel-sched.c:5539
0x414fc59f schedule_on_fences
../../gcc/sel-sched.c:7356
0x414fc59f sel_sched_region_2
../../gcc/sel-sched.c:7494
0x41503acf sel_sched_region_1
../../gcc/sel-sched.c:7536
0x41503acf sel_sched_region(int)
../../gcc/sel-sched.c:7637
0x41504e6f run_selective_scheduling()
../../gcc/sel-sched.c:7713
0x41df9bdf ia64_reorg
../../gcc/config/ia64/ia64.c:9854
0x4146d40f execute
../../gcc/reorg.c:3947

Andreas.

-- 
Andreas Schwab, SUSE Labs, sch...@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

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

[Martin Liška]

On 11/06/2017 06:33 PM, Jakub Jelinek wrote:
> On Mon, Nov 06, 2017 at 06:23:11PM +0100, Eric Botcazou wrote:
>>> Thank you for review, done that.
>>
>> This has enabled -Wreturn-type for Ada, what we don't want since the warning 
>> is outsmarted by the language, so I have applied this.
>>
>>
>> 2017-11-06  Eric Botcazou  
>>
>>  * gcc-interface/misc.c (gnat_post_options): Clear warn_return_type.
> 
> Hasn't it enabled it also for any other FEs other than C family and Fortran?
> Say jit, brig, go, lto?, ...
> I think better would be to remove the initialization to -1 and revert the
> fortran/options.c change, and instead use in the C family:
>   if (!global_options_set.x_warn_return_type)
> warn_return_type = c_dialect_cxx ();
> 
> Unless it for some reason doesn't work for -Wall or -W or similar.
> 
>   Jakub
> 

Hello.

Sorry for the inconvenience, however using Jakub's approach really does not 
work properly
with -Wall.

Thus I'm suggesting following patch that will disable it in all *_post_options 
hooks.

Ready after it survives regression tests?
Martin
>From d5e41295202a09c98787dba436cc568bbf1bbf4a Mon Sep 17 00:00:00 2001
From: marxin 
Date: Tue, 7 Nov 2017 10:27:13 +0100
Subject: [PATCH] Drop -Wreturn-type by default for BRIG, GO and LTO FEs.

gcc/brig/ChangeLog:

2017-11-07  Martin Liska  

	* brig-lang.c (brig_langhook_post_options): Drop
	warn_return_type if not set.

gcc/go/ChangeLog:

2017-11-07  Martin Liska  

	* go-lang.c (go_langhook_post_options): Drop
	warn_return_type if not set.

gcc/lto/ChangeLog:

2017-11-07  Martin Liska  

	* lto-lang.c (lto_post_options): Drop
	warn_return_type if not set.
---
 gcc/brig/brig-lang.c | 3 +++
 gcc/go/go-lang.c | 3 +++
 gcc/lto/lto-lang.c   | 3 +++
 3 files changed, 9 insertions(+)

diff --git a/gcc/brig/brig-lang.c b/gcc/brig/brig-lang.c
index f34d9587632..1fd558cc6df 100644
--- a/gcc/brig/brig-lang.c
+++ b/gcc/brig/brig-lang.c
@@ -171,6 +171,9 @@ brig_langhook_post_options (const char **pfilename ATTRIBUTE_UNUSED)
  broken code if not force disabling it.  */
   flag_strict_aliasing = 0;
 
+  if (warn_return_type == -1)
+warn_return_type = 0;
+
   /* Returning false means that the backend should be used.  */
   return false;
 }
diff --git a/gcc/go/go-lang.c b/gcc/go/go-lang.c
index 81eeb5c9cdc..0baebe32349 100644
--- a/gcc/go/go-lang.c
+++ b/gcc/go/go-lang.c
@@ -313,6 +313,9 @@ go_langhook_post_options (const char **pfilename ATTRIBUTE_UNUSED)
   && !global_options_set.x_flag_reorder_blocks_and_partition)
 global_options.x_flag_reorder_blocks_and_partition = 0;
 
+  if (warn_return_type == -1)
+warn_return_type = 0;
+
   /* Returning false means that the backend should be used.  */
   return false;
 }
diff --git a/gcc/lto/lto-lang.c b/gcc/lto/lto-lang.c
index 88f29705e65..892d4767f76 100644
--- a/gcc/lto/lto-lang.c
+++ b/gcc/lto/lto-lang.c
@@ -877,6 +877,9 @@ lto_post_options (const char **pfilename ATTRIBUTE_UNUSED)
   if (!flag_merge_constants)
 flag_merge_constants = 1;
 
+  if (warn_return_type == -1)
+warn_return_type = 0;
+
   /* Initialize the compiler back end.  */
   return false;
 }
-- 
2.14.3

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

[Eric Botcazou]

> Hasn't it enabled it also for any other FEs other than C family and Fortran?
> Say jit, brig, go, lto?, ...

Very likely, yes.

> I think better would be to remove the initialization to -1 and revert the
> fortran/options.c change, and instead use in the C family:
>   if (!global_options_set.x_warn_return_type)
> warn_return_type = c_dialect_cxx ();

No disagreement.

-- 
Eric Botcazou

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

[Martin Sebor]

Sorry for being late with my comment.  I just spotted this minor
formatting issue.  Even though GCC isn't (yet) consistent about
it the keyword "constexpr" should be quoted in the error message
below (and, eventually, in all diagnostic messages).  Since the
patch has been committed by now this is just a reminder for us
to try to keep this in mind in the future.

Martin

--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -1180,9 +1180,18 @@ cxx_eval_builtin_function_call (const 
constexpr_ctx *ctx, tree t, tree fun,

 {
   if (!*non_constant_p && !ctx->quiet)
{
- 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.
+The __builtin_unreachable call with BUILTINS_LOCATION
+comes from cp_maybe_instrument_return.  */
+ if (DECL_FUNCTION_CODE (fun) == BUILT_IN_UNREACHABLE
+ && EXPR_LOCATION (t) == BUILTINS_LOCATION)
+   error ("constexpr call flows off the end of the function");

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

[Jakub Jelinek]

On Mon, Nov 06, 2017 at 06:23:11PM +0100, Eric Botcazou wrote:
> > Thank you for review, done that.
> 
> This has enabled -Wreturn-type for Ada, what we don't want since the warning 
> is outsmarted by the language, so I have applied this.
> 
> 
> 2017-11-06  Eric Botcazou  
> 
>   * gcc-interface/misc.c (gnat_post_options): Clear warn_return_type.

Hasn't it enabled it also for any other FEs other than C family and Fortran?
Say jit, brig, go, lto?, ...
I think better would be to remove the initialization to -1 and revert the
fortran/options.c change, and instead use in the C family:
  if (!global_options_set.x_warn_return_type)
warn_return_type = c_dialect_cxx ();

Unless it for some reason doesn't work for -Wall or -W or similar.

Jakub

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

[Eric Botcazou]

> Thank you for review, done that.

This has enabled -Wreturn-type for Ada, what we don't want since the warning 
is outsmarted by the language, so I have applied this.


2017-11-06  Eric Botcazou  

* gcc-interface/misc.c (gnat_post_options): Clear warn_return_type.

-- 
Eric BotcazouIndex: gcc-interface/misc.c
===
--- gcc-interface/misc.c	(revision 254449)
+++ gcc-interface/misc.c	(working copy)
@@ -262,6 +262,9 @@ gnat_post_options (const char **pfilenam
   /* No psABI change warnings for Ada.  */
   warn_psabi = 0;
 
+  /* No return type warnings for Ada.  */
+  warn_return_type = 0;
+
   /* No caret by default for Ada.  */
   if (!global_options_set.x_flag_diagnostics_show_caret)
 global_dc->show_caret = false;

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

[Martin Liška]

On 10/24/2017 04:19 PM, Jason Merrill wrote:
> On 10/18/2017 09:07 AM, Martin Liška wrote:
>> @@ -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);
> 
> You don't need to build new_call in the new case, since you don't use it.
> 
> Also, please adjust the comment to say that a __builtin_unreachable call with 
> BUILTINS_LOCATION comes from cp_maybe_instrument_return.
> 
> OK with those changes.
> 
> Jason

Hi.

Thank you for review, done that.
Can you please take a look at the single problematic test-case that blocks 
acceptance of the patch to trunk?

Martin
>From 0c4fc1acba49d2d5ca2e6c475286a14e465b6f6c Mon Sep 17 00:00:00 2001
From: marxin 
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  

	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  

	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  

	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| 15 ---
 gcc/cp/cp-gimplify.c  | 20 ++--
 gcc/fortran/options.c |  3 +++
 5 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/gcc/c-family/c-opts.c b/gcc/c-family/c-opts.c
index 32120e636c2..cead15e7a63 100644
--- a/gcc/c-family/c-opts.c
+++ b/gcc/c-family/c-opts.c
@@ -989,6 +989,9 @@ c_common_post_options (const char **pfilename)
 	flag_extern_tls_init = 1;
 }
 
+  if (warn_return_type == -1)
+warn_return_type = c_dialect_cxx ();
+
   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 dae124ac1c2..9ab31f0e153 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 483f731a49a..7c2185851e0 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -1180,9 +1180,18 @@ cxx_eval_builtin_function_call (const constexpr_ctx *ctx, tree t, tree fun,
 {
   if (!*non_constant_p && !ctx->quiet)
 	{
-	  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.
+	 The __builtin_unreachable call with BUILTINS_LOCATION
+	 comes from cp_maybe_instrument_return.  */
+	  if (DECL_FUNCTION_CODE (fun) == BUILT_IN_UNREACHABLE
+	  && EXPR_LOCATION (t) == BUILTINS_LOCATION)
+	error ("constexpr call flows off the end of the function");
+	  else
+	{
+	  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);
+	}
 	}
   *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 

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

[Jason Merrill]

On 10/18/2017 09:07 AM, Martin Liška wrote:

@@ -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);


You don't need to build new_call in the new case, since you don't use it.

Also, please adjust the comment to say that a __builtin_unreachable call 
with BUILTINS_LOCATION comes from cp_maybe_instrument_return.


OK with those changes.

Jason

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

[Martin Liška]

On 10/18/2017 02:52 PM, Marek Polacek wrote:
> On Wed, Oct 18, 2017 at 02:46:23PM +0200, Martin Liška wrote:
>> 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
> 
>> From 36f3f45d9fa42344261faf60bb3cfbe22ed262ac Mon Sep 17 00:00:00 2001
>> From: marxin 
>> 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  
>>
>>  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  
>>
>>  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  
>>
>>  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;
> 
> Here you can simply
> 
>   warn_return_type = c_dialect_cxx ();
> 
> no?
> 
>   Marek
> 

Yes, thanks for the nit.

Martin
>From 8bbb75392d13430ce43cc1c0572ec5506d8a4353 Mon Sep 17 00:00:00 2001
From: marxin 
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  

	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  

	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  

	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..2b94128e941 100644
--- a/gcc/c-family/c-opts.c
+++ 

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

[Marek Polacek]

On Wed, Oct 18, 2017 at 02:46:23PM +0200, Martin Liška wrote:
> 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

> From 36f3f45d9fa42344261faf60bb3cfbe22ed262ac Mon Sep 17 00:00:00 2001
> From: marxin 
> 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  
> 
>   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  
> 
>   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  
> 
>   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;

Here you can simply

  warn_return_type = c_dialect_cxx ();

no?

Marek

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

[Martin Liška]

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
>From 36f3f45d9fa42344261faf60bb3cfbe22ed262ac Mon Sep 17 00:00:00 2001
From: marxin 
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  

	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  

	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  

	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 

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

[Jason Merrill]

On Thu, Oct 12, 2017 at 4:40 AM, Martin Liška  wrote:
> On 10/11/2017 04:59 PM, Jason Merrill wrote:
>> On Thu, Oct 5, 2017 at 12:53 PM, Martin Liška  wrote:
>>> On 10/05/2017 05:07 PM, Jason Merrill wrote:
 On Thu, Oct 5, 2017 at 6:31 AM, Martin Liška  wrote:
> As discussed 2 days ago on IRC with Jakub and Jonathan, C++ standard says
> that having a non-return
> function with missing return statement is undefined behavior. We've got
> -fsanitize=return check for
> that and we can in such case instrument __builtin_unreachable(). This
> patch does that.


 Great.

> And there's still some fallout:
>
> FAIL: g++.dg/cpp0x/constexpr-diag3.C  -std=c++11  (test for errors, line
> 7)
> FAIL: g++.dg/cpp0x/constexpr-neg3.C  -std=c++11  (test for errors, line
> 12)
> FAIL: g++.dg/cpp1y/constexpr-return2.C  -std=c++14  (test for errors,
> line 7)
> FAIL: g++.dg/cpp1y/constexpr-return2.C  -std=c++14 (test for excess
> errors)
> FAIL: g++.dg/cpp1y/pr63996.C  -std=c++14  (test for errors, line 9)
> FAIL: g++.dg/cpp1y/pr63996.C  -std=c++14 (test for excess errors)
>
> 1) there are causing:
>
> ./xg++ -B.
> /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/cpp1y/pr63996.C
> /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/cpp1y/pr63996.C:9:23:
> in constexpr expansion of ‘foo(1)’
> /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/cpp1y/pr63996.C:4:1:
> error: ‘__builtin_unreachable()’ is not a constant expression
>   foo (int i)
>   ^~~
>
> Where we probably should not emit the built-in call. Am I right?


 Or constexpr.c could give a more friendly diagnostic for
 __builtin_unreachable.  It's correct to give a diagnostic here for
 this testcase.
>>>
>>>
>>> Hi.
>>>
>>> That's good idea, any suggestion different from:
>>>
>>> ./xg++ -B.
>>> /home/marxin/Programming/gcc2/gcc/testsuite/g++.dg/cpp1y/pr63996.C
>>> /home/marxin/Programming/gcc2/gcc/testsuite/g++.dg/cpp1y/pr63996.C:9:23:
>>> in constexpr expansion of ‘foo(1)’
>>> : error: constexpr can't contain call of a non-return function
>>> ‘__builtin_unreachable’
>>>
>>> which is probably misleading as it points to a function call that is
>>> actually missing in source code.
>>> Should we distinguish between implicit and explicit __builtin_unreachable?
>>
>> Probably without your change the constexpr code already diagnoses the
>> missing return as "constexpr call flows off the end of the function";
>> that same message seems appropriate.
>
> Hello.
>
> Ok, I've done that. Sending patch candidate.
>
>>
>>> So turning on the warning by default for c++, we get about 500 failing 
>>> test-cases. Uf :/
>>
>> Yes, we've been sloppy about this in the testsuite.  :(
>
> I'll fix it. For being sure, I'm sending first part where I demonstrate how I 
> plan to change
> tests, with following preference:
>
> 1) change function to void type if possible
> 2) return a default value for functions not returning a value (for complex 
> return value of a type A:
>I do { static A a; return a; } is it appropriate?

How about "return A();" ?

Jason

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

[Jakub Jelinek]

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

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

[Martin Liška]

On 10/11/2017 04:59 PM, Jason Merrill wrote:
> On Thu, Oct 5, 2017 at 12:53 PM, Martin Liška  wrote:
>> On 10/05/2017 05:07 PM, Jason Merrill wrote:
>>> On Thu, Oct 5, 2017 at 6:31 AM, Martin Liška  wrote:
 As discussed 2 days ago on IRC with Jakub and Jonathan, C++ standard says
 that having a non-return
 function with missing return statement is undefined behavior. We've got
 -fsanitize=return check for
 that and we can in such case instrument __builtin_unreachable(). This
 patch does that.
>>>
>>>
>>> Great.
>>>
 And there's still some fallout:

 FAIL: g++.dg/cpp0x/constexpr-diag3.C  -std=c++11  (test for errors, line
 7)
 FAIL: g++.dg/cpp0x/constexpr-neg3.C  -std=c++11  (test for errors, line
 12)
 FAIL: g++.dg/cpp1y/constexpr-return2.C  -std=c++14  (test for errors,
 line 7)
 FAIL: g++.dg/cpp1y/constexpr-return2.C  -std=c++14 (test for excess
 errors)
 FAIL: g++.dg/cpp1y/pr63996.C  -std=c++14  (test for errors, line 9)
 FAIL: g++.dg/cpp1y/pr63996.C  -std=c++14 (test for excess errors)

 1) there are causing:

 ./xg++ -B.
 /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/cpp1y/pr63996.C
 /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/cpp1y/pr63996.C:9:23:
 in constexpr expansion of ‘foo(1)’
 /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/cpp1y/pr63996.C:4:1:
 error: ‘__builtin_unreachable()’ is not a constant expression
   foo (int i)
   ^~~

 Where we probably should not emit the built-in call. Am I right?
>>>
>>>
>>> Or constexpr.c could give a more friendly diagnostic for
>>> __builtin_unreachable.  It's correct to give a diagnostic here for
>>> this testcase.
>>
>>
>> Hi.
>>
>> That's good idea, any suggestion different from:
>>
>> ./xg++ -B.
>> /home/marxin/Programming/gcc2/gcc/testsuite/g++.dg/cpp1y/pr63996.C
>> /home/marxin/Programming/gcc2/gcc/testsuite/g++.dg/cpp1y/pr63996.C:9:23:
>> in constexpr expansion of ‘foo(1)’
>> : error: constexpr can't contain call of a non-return function
>> ‘__builtin_unreachable’
>>
>> which is probably misleading as it points to a function call that is
>> actually missing in source code.
>> Should we distinguish between implicit and explicit __builtin_unreachable?
> 
> Probably without your change the constexpr code already diagnoses the
> missing return as "constexpr call flows off the end of the function";
> that same message seems appropriate.

Hello.

Ok, I've done that. Sending patch candidate.

> 
>> So turning on the warning by default for c++, we get about 500 failing 
>> test-cases. Uf :/
> 
> Yes, we've been sloppy about this in the testsuite.  :(

I'll fix it. For being sure, I'm sending first part where I demonstrate how I 
plan to change
tests, with following preference:

1) change function to void type if possible
2) return a default value for functions not returning a value (for complex 
return value of a type A:
   I do { static A a; return a; } is it appropriate?

Thanks for feedback. If it's fine, then I'll carry on.


Martin

> 
> Jason
> 

>From 56c6520d86cba32d04cdbd8873243a7ab801975f Mon Sep 17 00:00:00 2001
From: marxin 
Date: Thu, 12 Oct 2017 10:14:59 +0200
Subject: [PATCH] Instrument function exit with __builtin_unreachable in C++

gcc/c-family/ChangeLog:

2017-10-12  Martin Liska  

	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  

	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  

	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|  7 ++-
 gcc/cp/cp-gimplify.c  | 18 --
 gcc/fortran/options.c |  3 +++
 5 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/gcc/c-family/c-opts.c b/gcc/c-family/c-opts.c
index 3662aa37be6..afea6a3dca3 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.
 
 

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

[Jason Merrill]

On Thu, Oct 5, 2017 at 12:53 PM, Martin Liška  wrote:
> On 10/05/2017 05:07 PM, Jason Merrill wrote:
>> On Thu, Oct 5, 2017 at 6:31 AM, Martin Liška  wrote:
>>> As discussed 2 days ago on IRC with Jakub and Jonathan, C++ standard says
>>> that having a non-return
>>> function with missing return statement is undefined behavior. We've got
>>> -fsanitize=return check for
>>> that and we can in such case instrument __builtin_unreachable(). This
>>> patch does that.
>>
>>
>> Great.
>>
>>> And there's still some fallout:
>>>
>>> FAIL: g++.dg/cpp0x/constexpr-diag3.C  -std=c++11  (test for errors, line
>>> 7)
>>> FAIL: g++.dg/cpp0x/constexpr-neg3.C  -std=c++11  (test for errors, line
>>> 12)
>>> FAIL: g++.dg/cpp1y/constexpr-return2.C  -std=c++14  (test for errors,
>>> line 7)
>>> FAIL: g++.dg/cpp1y/constexpr-return2.C  -std=c++14 (test for excess
>>> errors)
>>> FAIL: g++.dg/cpp1y/pr63996.C  -std=c++14  (test for errors, line 9)
>>> FAIL: g++.dg/cpp1y/pr63996.C  -std=c++14 (test for excess errors)
>>>
>>> 1) there are causing:
>>>
>>> ./xg++ -B.
>>> /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/cpp1y/pr63996.C
>>> /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/cpp1y/pr63996.C:9:23:
>>> in constexpr expansion of ‘foo(1)’
>>> /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/cpp1y/pr63996.C:4:1:
>>> error: ‘__builtin_unreachable()’ is not a constant expression
>>>   foo (int i)
>>>   ^~~
>>>
>>> Where we probably should not emit the built-in call. Am I right?
>>
>>
>> Or constexpr.c could give a more friendly diagnostic for
>> __builtin_unreachable.  It's correct to give a diagnostic here for
>> this testcase.
>
>
> Hi.
>
> That's good idea, any suggestion different from:
>
> ./xg++ -B.
> /home/marxin/Programming/gcc2/gcc/testsuite/g++.dg/cpp1y/pr63996.C
> /home/marxin/Programming/gcc2/gcc/testsuite/g++.dg/cpp1y/pr63996.C:9:23:
> in constexpr expansion of ‘foo(1)’
> : error: constexpr can't contain call of a non-return function
> ‘__builtin_unreachable’
>
> which is probably misleading as it points to a function call that is
> actually missing in source code.
> Should we distinguish between implicit and explicit __builtin_unreachable?

Probably without your change the constexpr code already diagnoses the
missing return as "constexpr call flows off the end of the function";
that same message seems appropriate.

> So turning on the warning by default for c++, we get about 500 failing 
> test-cases. Uf :/

Yes, we've been sloppy about this in the testsuite.  :(

Jason

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

[Martin Liška]

On 10/05/2017 06:53 PM, Martin Liška wrote:
>>  We probably want to provide a way to turn off this behavior.
>>
>> If we're going to enable this by default, we probably also want
>> -Wreturn-type on by default.
> 
> Agree.

Hi.

So turning on the warning by default for c++, we get about 500 failing 
test-cases. Uf :/

Martin
c-c++-common/asan/pr63638.c
c-c++-common/goacc/parallel-1.c
c-c++-common/gomp/sink-1.c
c-c++-common/missing-symbol.c
c-c++-common/pr36513-2.c
c-c++-common/pr36513.c
c-c++-common/pr49706-2.c
c-c++-common/pr65120.c
c-c++-common/tm/volatile-1.c
c-c++-common/tsan/race_on_mutex.c
c-c++-common/ubsan/pr71512-1.c
c-c++-common/vector-1.c
c-c++-common/vector-2.c
c-c++-common/Wimplicit-fallthrough-8.c
g++.dg/abi/abi-tag14.C
g++.dg/abi/abi-tag18a.C
g++.dg/abi/abi-tag18.C
g++.dg/abi/covariant2.C
g++.dg/abi/covariant3.C
g++.dg/abi/mangle7.C
g++.dg/asan/pr81340.C
g++.dg/concepts/fn8.C
g++.dg/concepts/pr65575.C
g++.dg/concepts/template-parm11.C
g++.dg/conversion/op6.C
g++.dg/cpp0x/access01.C
g++.dg/cpp0x/alignas3.C
g++.dg/cpp0x/auto2.C
g++.dg/cpp0x/constexpr-array17.C
g++.dg/cpp0x/constexpr-defarg2.C
g++.dg/cpp0x/constexpr-diag3.C
g++.dg/cpp0x/constexpr-memfn1.C
g++.dg/cpp0x/constexpr-neg3.C
g++.dg/cpp0x/dc1.C
g++.dg/cpp0x/dc3.C
g++.dg/cpp0x/decltype12.C
g++.dg/cpp0x/decltype17.C
g++.dg/cpp0x/decltype3.C
g++.dg/cpp0x/decltype41.C
g++.dg/cpp0x/defaulted28.C
g++.dg/cpp0x/enum_base3.C
g++.dg/cpp0x/gen-attrs-4.C
g++.dg/cpp0x/initlist96.C
g++.dg/cpp0x/lambda/lambda-58566.C
g++.dg/cpp0x/lambda/lambda-conv10.C
g++.dg/cpp0x/lambda/lambda-conv12.C
g++.dg/cpp0x/lambda/lambda-defarg3.C
g++.dg/cpp0x/lambda/lambda-ice3.C
g++.dg/cpp0x/lambda/lambda-ice5.C
g++.dg/cpp0x/lambda/lambda-nested2.C
g++.dg/cpp0x/lambda/lambda-template12.C
g++.dg/cpp0x/lambda/lambda-template2.C
g++.dg/cpp0x/lambda/lambda-this12.C
g++.dg/cpp0x/nolinkage1.C
g++.dg/cpp0x/nsdmi-template5.C
g++.dg/cpp0x/parse1.C
g++.dg/cpp0x/pr34054.C
g++.dg/cpp0x/pr47416.C
g++.dg/cpp0x/pr58781.C
g++.dg/cpp0x/pr70538.C
g++.dg/cpp0x/pr81325.C
g++.dg/cpp0x/range-for13.C
g++.dg/cpp0x/range-for14.C
g++.dg/cpp0x/rv2n.C
g++.dg/cpp0x/rv3n.C
g++.dg/cpp0x/static_assert10.C
g++.dg/cpp0x/static_assert11.C
g++.dg/cpp0x/static_assert12.C
g++.dg/cpp0x/static_assert13.C
g++.dg/cpp0x/trailing1.C
g++.dg/cpp0x/trailing5.C
g++.dg/cpp0x/variadic114.C
g++.dg/cpp0x/variadic57.C
g++.dg/cpp0x/variadic65.C
g++.dg/cpp0x/variadic66.C
g++.dg/cpp0x/variadic97.C
g++.dg/cpp0x/variadic98.C
g++.dg/cpp0x/Wunused-variable-1.C
g++.dg/cpp1y/auto-fn11.C
g++.dg/cpp1y/auto-fn29.C
g++.dg/cpp1y/auto-fn38.C
g++.dg/cpp1y/constexpr-return2.C
g++.dg/cpp1y/lambda-init7.C
g++.dg/cpp1y/pr63996.C
g++.dg/cpp1y/pr65202.C
g++.dg/cpp1y/pr66443-cxx14.C
g++.dg/cpp1y/pr79253.C
g++.dg/cpp1y/static_assert1.C
g++.dg/cpp1y/static_assert2.C
g++.dg/cpp1y/var-templ44.C
g++.dg/cpp1z/fold6.C
g++.dg/cpp1z/inline-var2.C
g++.dg/cpp1z/lambda-this1.C
g++.dg/cpp1z/static_assert-nomsg.C
g++.dg/debug/dwarf2/dwarf4-typedef.C
g++.dg/debug/dwarf2/icf.C
g++.dg/debug/dwarf2/pr61433.C
g++.dg/debug/dwarf-eh-personality-1.C
g++.dg/debug/nullptr01.C
g++.dg/debug/pr16792.C
g++.dg/debug/pr46241.C
g++.dg/debug/pr46338.C
g++.dg/debug/pr47106.C
g++.dg/debug/pr71057.C
g++.dg/debug/pr71432.C
g++.dg/debug/pr80461.C
g++.dg/dfp/44473-1.C
g++.dg/dfp/44473-2.C
g++.dg/eh/builtin1.C
g++.dg/eh/builtin2.C
g++.dg/eh/builtin3.C
g++.dg/eh/pr45569.C
g++.dg/eh/unwind2.C
g++.dg/expr/bitfield11.C
g++.dg/expr/static_cast7.C
g++.dg/ext/altivec-14.C
g++.dg/ext/asm13.C
g++.dg/ext/builtin-object-size3.C
g++.dg/ext/has_nothrow_assign_odr.C
g++.dg/ext/label7.C
g++.dg/ext/label8.C
g++.dg/ext/tmplattr7.C
g++.dg/ext/vector8.C
g++.dg/ext/visibility/anon1.C
g++.dg/ext/visibility/anon2.C
g++.dg/ext/visibility/namespace1.C
g++.dg/ext/vla16.C
g++.dg/goacc/reference.C
g++.dg/gomp/pr37189.C
g++.dg/gomp/pr39495-1.C
g++.dg/gomp/pr39495-2.C
g++.dg/gomp/pr41429.C
g++.dg/gomp/pr82054.C
g++.dg/inherit/covariant10.C
g++.dg/inherit/covariant11.C
g++.dg/inherit/protected1.C
g++.dg/init/inline1.C
g++.dg/init/new18.C
g++.dg/init/reference2.C
g++.dg/init/reference3.C
g++.dg/init/switch1.C
g++.dg/ipa/devirt-10.C
g++.dg/ipa/devirt-13.C
g++.dg/ipa/devirt-14.C
g++.dg/ipa/devirt-15.C
g++.dg/ipa/devirt-16.C
g++.dg/ipa/devirt-17.C
g++.dg/ipa/devirt-18.C
g++.dg/ipa/devirt-19.C
g++.dg/ipa/devirt-21.C
g++.dg/ipa/devirt-23.C
g++.dg/ipa/devirt-38.C
g++.dg/ipa/devirt-40.C
g++.dg/ipa/devirt-41.C
g++.dg/ipa/devirt-42.C
g++.dg/ipa/devirt-44.C
g++.dg/ipa/devirt-45.C
g++.dg/ipa/devirt-48.C
g++.dg/ipa/devirt-52.C
g++.dg/ipa/nothrow-1.C
g++.dg/ipa/pr43812.C
g++.dg/ipa/pr44372.C
g++.dg/ipa/pr45572-1.C
g++.dg/ipa/pr58371.C
g++.dg/ipa/pr59176.C
g++.dg/ipa/pr60640-1.C
g++.dg/ipa/pr61540.C
g++.dg/ipa/pr63470.C
g++.dg/ipa/pr63587-1.C
g++.dg/ipa/pr63587-2.C
g++.dg/ipa/pr63838.C
g++.dg/ipa/pr63894.C
g++.dg/ipa/pr64068.C
g++.dg/ipa/pr64896.C
g++.dg/ipa/pr65002.C
g++.dg/ipa/pr65008.C
g++.dg/ipa/pr65465.C
g++.dg/ipa/pr66896.C
g++.dg/ipa/pr68851.C
g++.dg/ipa/pr78211.C
g++.dg/ipa/pr79931.C
g++.dg/ipa/pure-const-1.C

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

[Martin Liška]

On 10/05/2017 05:07 PM, Jason Merrill wrote:

On Thu, Oct 5, 2017 at 6:31 AM, Martin Liška  wrote:

As discussed 2 days ago on IRC with Jakub and Jonathan, C++ standard says that 
having a non-return
function with missing return statement is undefined behavior. We've got 
-fsanitize=return check for
that and we can in such case instrument __builtin_unreachable(). This patch 
does that.


Great.


And there's still some fallout:

FAIL: g++.dg/cpp0x/constexpr-diag3.C  -std=c++11  (test for errors, line 7)
FAIL: g++.dg/cpp0x/constexpr-neg3.C  -std=c++11  (test for errors, line 12)
FAIL: g++.dg/cpp1y/constexpr-return2.C  -std=c++14  (test for errors, line 7)
FAIL: g++.dg/cpp1y/constexpr-return2.C  -std=c++14 (test for excess errors)
FAIL: g++.dg/cpp1y/pr63996.C  -std=c++14  (test for errors, line 9)
FAIL: g++.dg/cpp1y/pr63996.C  -std=c++14 (test for excess errors)

1) there are causing:

./xg++ -B. /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/cpp1y/pr63996.C
/home/marxin/Programming/gcc/gcc/testsuite/g++.dg/cpp1y/pr63996.C:9:23:   in 
constexpr expansion of ‘foo(1)’
/home/marxin/Programming/gcc/gcc/testsuite/g++.dg/cpp1y/pr63996.C:4:1: error: 
‘__builtin_unreachable()’ is not a constant expression
  foo (int i)
  ^~~

Where we probably should not emit the built-in call. Am I right?


Or constexpr.c could give a more friendly diagnostic for
__builtin_unreachable.  It's correct to give a diagnostic here for
this testcase.


Hi.

That's good idea, any suggestion different from:

./xg++ -B. /home/marxin/Programming/gcc2/gcc/testsuite/g++.dg/cpp1y/pr63996.C
/home/marxin/Programming/gcc2/gcc/testsuite/g++.dg/cpp1y/pr63996.C:9:23:   in 
constexpr expansion of ‘foo(1)’
: error: constexpr can't contain call of a non-return function 
‘__builtin_unreachable’

which is probably misleading as it points to a function call that is actually 
missing in source code.
Should we distinguish between implicit and explicit __builtin_unreachable?




FAIL: g++.dg/torture/pr34850.C   -O1   (test for warnings, line 15)
FAIL: g++.dg/torture/pr34850.C   -O2   (test for warnings, line 15)
FAIL: g++.dg/torture/pr34850.C   -O2 -flto -fno-use-linker-plugin 
-flto-partition=none   (test for warnings, line 15)
FAIL: g++.dg/torture/pr34850.C   -O2 -flto -fuse-linker-plugin 
-fno-fat-lto-objects   (test for warnings, line 15)
FAIL: g++.dg/torture/pr34850.C   -O3 -g   (test for warnings, line 15)
FAIL: g++.dg/torture/pr34850.C   -Os   (test for warnings, line 15)

2) this test is somehow hard to fix as it requires some unreachability.


Can't we add a return statement to memset?


Does not help :)



Also, this testcase seems to indicate a danger from this patch, like
we've seen before with bounds checking: if we have a checking path
that complains about invalid arguments and then has undefined
behavior, we optimize away the diagnostic.  Can we warn in such cases?


Good question, can you please provide more info how that happens? Do we have an 
example for that?


  We probably want to provide a way to turn off this behavior.

If we're going to enable this by default, we probably also want
-Wreturn-type on by default.


Agree.

Thanks,
Martin



Jason

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

[Jason Merrill]

On Thu, Oct 5, 2017 at 6:31 AM, Martin Liška  wrote:
> As discussed 2 days ago on IRC with Jakub and Jonathan, C++ standard says 
> that having a non-return
> function with missing return statement is undefined behavior. We've got 
> -fsanitize=return check for
> that and we can in such case instrument __builtin_unreachable(). This patch 
> does that.

Great.

> And there's still some fallout:
>
> FAIL: g++.dg/cpp0x/constexpr-diag3.C  -std=c++11  (test for errors, line 7)
> FAIL: g++.dg/cpp0x/constexpr-neg3.C  -std=c++11  (test for errors, line 12)
> FAIL: g++.dg/cpp1y/constexpr-return2.C  -std=c++14  (test for errors, line 7)
> FAIL: g++.dg/cpp1y/constexpr-return2.C  -std=c++14 (test for excess errors)
> FAIL: g++.dg/cpp1y/pr63996.C  -std=c++14  (test for errors, line 9)
> FAIL: g++.dg/cpp1y/pr63996.C  -std=c++14 (test for excess errors)
>
> 1) there are causing:
>
> ./xg++ -B. /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/cpp1y/pr63996.C
> /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/cpp1y/pr63996.C:9:23:   in 
> constexpr expansion of ‘foo(1)’
> /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/cpp1y/pr63996.C:4:1: error: 
> ‘__builtin_unreachable()’ is not a constant expression
>  foo (int i)
>  ^~~
>
> Where we probably should not emit the built-in call. Am I right?

Or constexpr.c could give a more friendly diagnostic for
__builtin_unreachable.  It's correct to give a diagnostic here for
this testcase.

> FAIL: g++.dg/torture/pr34850.C   -O1   (test for warnings, line 15)
> FAIL: g++.dg/torture/pr34850.C   -O2   (test for warnings, line 15)
> FAIL: g++.dg/torture/pr34850.C   -O2 -flto -fno-use-linker-plugin 
> -flto-partition=none   (test for warnings, line 15)
> FAIL: g++.dg/torture/pr34850.C   -O2 -flto -fuse-linker-plugin 
> -fno-fat-lto-objects   (test for warnings, line 15)
> FAIL: g++.dg/torture/pr34850.C   -O3 -g   (test for warnings, line 15)
> FAIL: g++.dg/torture/pr34850.C   -Os   (test for warnings, line 15)
>
> 2) this test is somehow hard to fix as it requires some unreachability.

Can't we add a return statement to memset?

Also, this testcase seems to indicate a danger from this patch, like
we've seen before with bounds checking: if we have a checking path
that complains about invalid arguments and then has undefined
behavior, we optimize away the diagnostic.  Can we warn in such cases?
 We probably want to provide a way to turn off this behavior.

If we're going to enable this by default, we probably also want
-Wreturn-type on by default.

Jason

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

[Jakub Jelinek]

On Thu, Oct 05, 2017 at 12:31:23PM +0200, Martin Liška wrote:
> As discussed 2 days ago on IRC with Jakub and Jonathan, C++ standard says 
> that having a non-return
> function with missing return statement is undefined behavior. We've got 
> -fsanitize=return check for
> that and we can in such case instrument __builtin_unreachable(). This patch 
> does that. It produces quite
> some fallout in test-suite.
> 
> And there's still some fallout:
> 
> FAIL: g++.dg/cpp0x/constexpr-diag3.C  -std=c++11  (test for errors, line 7)
> FAIL: g++.dg/cpp0x/constexpr-neg3.C  -std=c++11  (test for errors, line 12)
> FAIL: g++.dg/cpp1y/constexpr-return2.C  -std=c++14  (test for errors, line 7)
> FAIL: g++.dg/cpp1y/constexpr-return2.C  -std=c++14 (test for excess errors)
> FAIL: g++.dg/cpp1y/pr63996.C  -std=c++14  (test for errors, line 9)
> FAIL: g++.dg/cpp1y/pr63996.C  -std=c++14 (test for excess errors)
> 
> 1) there are causing:
> 
> ./xg++ -B. /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/cpp1y/pr63996.C
> /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/cpp1y/pr63996.C:9:23:   in 
> constexpr expansion of ‘foo(1)’
> /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/cpp1y/pr63996.C:4:1: error: 
> ‘__builtin_unreachable()’ is not a constant expression
>  foo (int i)
>  ^~~
> 
> Where we probably should not emit the built-in call. Am I right?

The problem is that I think we save the constexpr function bodies after
folding and genericization.  We shouldn't be doing that, if we save it
before, in addition to fixing quite a few bugs it would fix this as well.

If looking for a hack instead of proper fix here, allowing
__builtin_unreachable () in constant expressions would be probably
undesirable, because then people adding it themselves in their source code
would get it accepted.  So, either we need some way how to differentiate
between user written __builtin_unreachable and one we've added ourselves
(doesn't -fsanitize=return fail the similar way though?), either as
internal-fn instead, or just by using UNKNOWN_LOCATION or BUILTINS_LOCATION
or something similar to find out it is artificial; and of course if we
reach it during constexpr evaluation error some way.

Jakub

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

[Martin Liška]

Hello.

As discussed 2 days ago on IRC with Jakub and Jonathan, C++ standard says that 
having a non-return
function with missing return statement is undefined behavior. We've got 
-fsanitize=return check for
that and we can in such case instrument __builtin_unreachable(). This patch 
does that. It produces quite
some fallout in test-suite.

And there's still some fallout:

FAIL: g++.dg/cpp0x/constexpr-diag3.C  -std=c++11  (test for errors, line 7)
FAIL: g++.dg/cpp0x/constexpr-neg3.C  -std=c++11  (test for errors, line 12)
FAIL: g++.dg/cpp1y/constexpr-return2.C  -std=c++14  (test for errors, line 7)
FAIL: g++.dg/cpp1y/constexpr-return2.C  -std=c++14 (test for excess errors)
FAIL: g++.dg/cpp1y/pr63996.C  -std=c++14  (test for errors, line 9)
FAIL: g++.dg/cpp1y/pr63996.C  -std=c++14 (test for excess errors)

1) there are causing:

./xg++ -B. /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/cpp1y/pr63996.C
/home/marxin/Programming/gcc/gcc/testsuite/g++.dg/cpp1y/pr63996.C:9:23:   in 
constexpr expansion of ‘foo(1)’
/home/marxin/Programming/gcc/gcc/testsuite/g++.dg/cpp1y/pr63996.C:4:1: error: 
‘__builtin_unreachable()’ is not a constant expression
 foo (int i)
 ^~~

Where we probably should not emit the built-in call. Am I right?

FAIL: g++.dg/torture/pr34850.C   -O1   (test for warnings, line 15)
FAIL: g++.dg/torture/pr34850.C   -O2   (test for warnings, line 15)
FAIL: g++.dg/torture/pr34850.C   -O2 -flto -fno-use-linker-plugin 
-flto-partition=none   (test for warnings, line 15)
FAIL: g++.dg/torture/pr34850.C   -O2 -flto -fuse-linker-plugin 
-fno-fat-lto-objects   (test for warnings, line 15)
FAIL: g++.dg/torture/pr34850.C   -O3 -g   (test for warnings, line 15)
FAIL: g++.dg/torture/pr34850.C   -Os   (test for warnings, line 15)

2) this test is somehow hard to fix as it requires some unreachability.

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

Thoughts?
Martin

gcc/cp/ChangeLog:

2017-10-05  Martin Liska  

PR middle-end/82404
* cp-gimplify.c (cp_ubsan_maybe_instrument_return): Remove as
renamed to ...
(cp_maybe_instrument_return): ... this.
(cp_genericize): Update call of the function.

gcc/testsuite/ChangeLog:

2017-10-05  Martin Liska  

PR middle-end/82404
* c-c++-common/dfp/call-by-value.c (foo32): Change return value
to void.
(foo64): Likewise.
(foo128): Likewise.
* g++.dg/bprob/g++-bprob-1.C: Likewise.
* g++.dg/cpp0x/lambda/lambda-template.C (f): Likewise.
* g++.dg/cpp0x/range-for6.C (foo): Return a value.
* g++.dg/cpp0x/udlit-template.C: Change return value
to void.
* g++.dg/cpp1z/eval-order3.C (struct A): Return a value.
(operator>>): Likewise.
* g++.dg/expr/cond12.C (struct X): Return a value.
(X::operator=): Likewise.
* g++.dg/gcov/gcov-1.C: Change return value
to void.
* g++.dg/gcov/gcov-threads-1.C (ContentionNoDeadlock_thread):
Return a value.
* g++.dg/ipa/devirt-21.C: Likewise.
* g++.dg/ipa/devirt-23.C: Likewise.
* g++.dg/ipa/devirt-34.C (t): Likewise.
* g++.dg/opt/20050511-1.C (bar): Likewise.
* g++.dg/opt/const3.C (A::foo1): Likewise.
(A::foo2): Likewise.
* g++.dg/opt/pr23299.C (E::c): Likewise.
* g++.dg/other/copy2.C (A::operator=): Likewise.
* g++.dg/overload/addr1.C: Likewise.
* g++.dg/pr48484.C: Likewise.
* g++.dg/tls/thread_local3.C (thread_main): Likewise.
* g++.dg/tls/thread_local3g.C (thread_main): Likewise.
* g++.dg/tls/thread_local5.C (thread_main): Likewise.
* g++.dg/tls/thread_local5g.C (thread_main): Likewise.
* g++.dg/tls/thread_local6.C (thread_main): Likewise.
* g++.dg/tls/thread_local6g.C (thread_main): Likewise.
* g++.dg/torture/pr34850.C (OctetString::operator^=): Likewise.
* g++.dg/tree-prof/pr79259.C (fn2): Likewise.
* g++.dg/tree-ssa/pr33604.C (struct Value): Likewise.
* g++.dg/tree-ssa/pr81408.C (struct p): Likewise.
(av): Likewise.
* g++.dg/warn/string1.C (test): Likewise.
---
 gcc/cp/cp-gimplify.c| 18 --
 gcc/testsuite/c-c++-common/dfp/call-by-value.c  |  6 +++---
 gcc/testsuite/g++.dg/bprob/g++-bprob-1.C|  2 +-
 gcc/testsuite/g++.dg/cpp0x/lambda/lambda-template.C |  2 +-
 gcc/testsuite/g++.dg/cpp0x/range-for6.C |  2 ++
 gcc/testsuite/g++.dg/cpp0x/udlit-template.C |  2 +-
 gcc/testsuite/g++.dg/cpp1z/eval-order3.C|  4 ++--
 gcc/testsuite/g++.dg/expr/cond12.C  |  8 +++-
 gcc/testsuite/g++.dg/gcov/gcov-1.C  |  2 +-
 gcc/testsuite/g++.dg/gcov/gcov-threads-1.C  |  2 ++
 gcc/testsuite/g++.dg/ipa/devirt-21.C|  2 +-
 gcc/testsuite/g++.dg/ipa/devirt-23.C|  2 +-