Message ID | PR3P194MB17148AA59B7C7FAA650D4B8CABF22@PR3P194MB1714.EURP194.PROD.OUTLOOK.COM |
---|---|
State | New |
Headers | show |
Series | c-family: Introduce the -Winvalid-noreturn flag from clang with extra tuneability | expand |
On 5/29/24 09:58, Julian Waters wrote: > Currently, gcc warns about noreturn marked functions that return both explicitly and implicitly, with no way to turn this warning off. clang does have an option for these classes of warnings, -Winvalid-noreturn. However, we can do better. Instead of just having 1 option that switches the warnings for both on and off, we can define an extra layer of granularity, and have a separate options for implicit returns and explicit returns, as in -Winvalid-return=explicit and -Winvalid-noreturn=implicit. This patch adds both to gcc, for compatibility with clang. Thanks! > Do note that I am relatively new to gcc's codebase, and as such couldn't figure out how to cleanly define a general -Winvalid-noreturn warning that switch both on and off, for better compatibility with clang. If someone should point out how to do so, I'll happily rewrite my patch. See -fstrong-eval-order for an example of an option that can be used with or without =arg. > I also do not have write access to gcc, and will need help pushing this patch once the green light is given Good to know, I can take care of that. > best regards, > Julian > > gcc/c-family/ChangeLog: > > * c.opt: Introduce -Winvalid-noreturn=explicit and -Winvalid-noreturn=implicit > > gcc/ChangeLog: > > * tree-cfg.cc (pass_warn_function_return::execute): Use it > > gcc/c/ChangeLog: > > * c-typeck.cc (c_finish_return): Use it > * gimple-parser.cc (c_finish_gimple_return): Use it > > gcc/config/mingw/ChangeLog: > > * mingw32.h (EXTRA_OS_CPP_BUILTINS): Fix semicolons > > gcc/cp/ChangeLog: > > * coroutines.cc (finish_co_return_stmt): Use it > * typeck.cc (check_return_expr): Use it > > gcc/doc/ChangeLog: > > * invoke.texi: Document new options > > From 4daf884f8bbc1e318ba93121a6fdf4139da80b64 Mon Sep 17 00:00:00 2001 > From: TheShermanTanker <tanksherman27@gmail.com> > Date: Wed, 29 May 2024 21:32:08 +0800 > Subject: [PATCH] Introduce the -Winvalid-noreturn flag from clang with extra > tuneability The rationale and ChangeLog entries should be part of the commit message (and so the git format-patch output). > > Signed-off-by: TheShermanTanker <tanksherman27@gmail.com> A DCO sign-off can't use a pseudonym, sorry; please either sign off using your real name or file a copyright assignment for the pseudonym with the FSF. See https://gcc.gnu.org/contribute.html#legal for more detail. > --- > gcc/c-family/c.opt | 8 ++++++++ > gcc/c/c-typeck.cc | 2 +- > gcc/c/gimple-parser.cc | 2 +- > gcc/config/mingw/mingw32.h | 6 +++--- > gcc/cp/coroutines.cc | 2 +- > gcc/cp/typeck.cc | 2 +- > gcc/doc/invoke.texi | 13 +++++++++++++ > gcc/tree-cfg.cc | 2 +- > 8 files changed, 29 insertions(+), 8 deletions(-) > > diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt > index fb34c3b7031..32a2859fdcc 100644 > --- a/gcc/c-family/c.opt > +++ b/gcc/c-family/c.opt > @@ -886,6 +886,14 @@ Winvalid-constexpr > C++ ObjC++ Var(warn_invalid_constexpr) Init(-1) Warning > Warn when a function never produces a constant expression. > > +Winvalid-noreturn=explicit > +C ObjC C++ ObjC++ Warning > +Warn when a function marked noreturn returns explicitly. > + > +Winvalid-noreturn=implicit > +C ObjC C++ ObjC++ Warning > +Warn when a function marked noreturn returns implicitly. > + > Winvalid-offsetof > C++ ObjC++ Var(warn_invalid_offsetof) Init(1) Warning > Warn about invalid uses of the \"offsetof\" macro. > diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc > index ad4c7add562..1941fbc44cb 100644 > --- a/gcc/c/c-typeck.cc > +++ b/gcc/c/c-typeck.cc > @@ -11468,7 +11468,7 @@ c_finish_return (location_t loc, tree retval, tree origtype) > location_t xloc = expansion_point_location_if_in_system_header (loc); > > if (TREE_THIS_VOLATILE (current_function_decl)) > - warning_at (xloc, 0, > + warning_at (xloc, OPT_Winvalid_noreturn_explicit, > "function declared %<noreturn%> has a %<return%> statement"); > > if (retval) > diff --git a/gcc/c/gimple-parser.cc b/gcc/c/gimple-parser.cc > index d156d83cd37..1acaf75f844 100644 > --- a/gcc/c/gimple-parser.cc > +++ b/gcc/c/gimple-parser.cc > @@ -2593,7 +2593,7 @@ c_finish_gimple_return (location_t loc, tree retval) > location_t xloc = expansion_point_location_if_in_system_header (loc); > > if (TREE_THIS_VOLATILE (current_function_decl)) > - warning_at (xloc, 0, > + warning_at (xloc, OPT_Winvalid_noreturn_explicit, > "function declared %<noreturn%> has a %<return%> statement"); > > if (! retval) > diff --git a/gcc/config/mingw/mingw32.h b/gcc/config/mingw/mingw32.h > index fa6e307476c..a69926133b1 100644 > --- a/gcc/config/mingw/mingw32.h > +++ b/gcc/config/mingw/mingw32.h > @@ -35,9 +35,9 @@ along with GCC; see the file COPYING3. If not see > | MASK_MS_BITFIELD_LAYOUT) > > #ifdef TARGET_USING_MCFGTHREAD > -#define DEFINE_THREAD_MODEL builtin_define ("__USING_MCFGTHREAD__"); > +#define DEFINE_THREAD_MODEL builtin_define ("__USING_MCFGTHREAD__") > #elif defined(TARGET_USE_PTHREAD_BY_DEFAULT) > -#define DEFINE_THREAD_MODEL builtin_define ("__USING_POSIXTHREAD__"); > +#define DEFINE_THREAD_MODEL builtin_define ("__USING_POSIXTHREAD__") > #else > #define DEFINE_THREAD_MODEL > #endif > @@ -60,7 +60,7 @@ along with GCC; see the file COPYING3. If not see > builtin_define_std ("WIN64"); \ > builtin_define ("_WIN64"); \ > } \ > - DEFINE_THREAD_MODEL \ > + DEFINE_THREAD_MODEL; \ > } \ > while (0) > > diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc > index 97bc211ff67..53e56bd4959 100644 > --- a/gcc/cp/coroutines.cc > +++ b/gcc/cp/coroutines.cc > @@ -1375,7 +1375,7 @@ finish_co_return_stmt (location_t kw, tree expr) > > /* Makes no sense for a co-routine really. */ > if (TREE_THIS_VOLATILE (current_function_decl)) > - warning_at (kw, 0, > + warning_at (kw, OPT_Winvalid_noreturn_explicit, > "function declared %<noreturn%> has a" > " %<co_return%> statement"); > > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc > index 1b7a31d32f3..74cc59bfb87 100644 > --- a/gcc/cp/typeck.cc > +++ b/gcc/cp/typeck.cc > @@ -11062,7 +11062,7 @@ check_return_expr (tree retval, bool *no_warning, bool *dangling) > (This is a G++ extension, used to get better code for functions > that call the `volatile' function.) */ > if (TREE_THIS_VOLATILE (current_function_decl)) > - warning (0, "function declared %<noreturn%> has a %<return%> statement"); > + warning (OPT_Winvalid_noreturn_explicit, "function declared %<noreturn%> has a %<return%> statement"); > > /* Check for various simple errors. */ > if (DECL_DESTRUCTOR_P (current_function_decl)) > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > index 2cba380718b..27d880fd4f0 100644 > --- a/gcc/doc/invoke.texi > +++ b/gcc/doc/invoke.texi > @@ -376,6 +376,7 @@ Objective-C and Objective-C++ Dialects}. > -Winfinite-recursion > -Winit-self -Winline -Wno-int-conversion -Wint-in-bool-context > -Wno-int-to-pointer-cast -Wno-invalid-memory-model > +-Winvalid-noreturn=explicit -Winvalid-noreturn=implicit > -Winvalid-pch -Winvalid-utf8 -Wno-unicode -Wjump-misses-init > -Wlarger-than=@var{byte-size} -Wlogical-not-parentheses -Wlogical-op > -Wlong-long -Wno-lto-type-mismatch -Wmain -Wmaybe-uninitialized > @@ -10482,6 +10483,18 @@ an error. @option{Wint-to-pointer-cast} is enabled by default. > Suppress warnings from casts from a pointer to an integer type of a > different size. > > +@opindex Winvalid-noreturn=explicit > +@opindex Wno-invalid-noreturn=explicit > +@item -Winvalid-noreturn=explicit > +Warn if a function marked noreturn returns explicitly, that is, it > +contains a return statement. > + > +@opindex Winvalid-noreturn=implicit > +@opindex Wno-invalid-noreturn=implicit > +@item -Winvalid-noreturn=implicit > +Warn if a function marked noreturn returns implicitly, that is, it > +has a path in its control flow that can reach the end of its body. > + > @opindex Winvalid-pch > @opindex Wno-invalid-pch > @item -Winvalid-pch > diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc > index 7fb7b92966b..cfe91038dd1 100644 > --- a/gcc/tree-cfg.cc > +++ b/gcc/tree-cfg.cc > @@ -9817,7 +9817,7 @@ pass_warn_function_return::execute (function *fun) > } > if (location == UNKNOWN_LOCATION) > location = cfun->function_end_locus; > - warning_at (location, 0, "%<noreturn%> function does return"); > + warning_at (location, OPT_Winvalid_noreturn_implicit, "%<noreturn%> function does return"); > } > > /* If we see "return;" in some basic block, then we do reach the end
Hi Jason, Thanks for the reply! I'll address your comments soon. I have a question, if there is an option defined in c.opt as an Enum, like fstrong-eval-order, and the -no variant of the option is passed, would the Var somehow reflect the negated option? Eg Winvalid-noreturn= C ObjC C++ ObjC++ Var(warn_invalid_noreturn) Joined Enum(invalid_noreturn) Warning Enum Name(invalid_noreturn) Type(int) EnumValue Enum(invalid_noreturn) String(explicit) Value(0) Would warn_invalid_noreturn then != 0 if -Wno-invalid-noreturn=explicit is passed? Or is there a way to make a warning call depend on 2 different OPT_ entries? best regards, Julian On Sat, Jun 1, 2024 at 4:57 AM Jason Merrill <jason@redhat.com> wrote: > > On 5/29/24 09:58, Julian Waters wrote: > > Currently, gcc warns about noreturn marked functions that return both explicitly and implicitly, with no way to turn this warning off. clang does have an option for these classes of warnings, -Winvalid-noreturn. However, we can do better. Instead of just having 1 option that switches the warnings for both on and off, we can define an extra layer of granularity, and have a separate options for implicit returns and explicit returns, as in -Winvalid-return=explicit and -Winvalid-noreturn=implicit. This patch adds both to gcc, for compatibility with clang. > > Thanks! > > > Do note that I am relatively new to gcc's codebase, and as such couldn't figure out how to cleanly define a general -Winvalid-noreturn warning that switch both on and off, for better compatibility with clang. If someone should point out how to do so, I'll happily rewrite my patch. > > See -fstrong-eval-order for an example of an option that can be used > with or without =arg. > > > I also do not have write access to gcc, and will need help pushing this patch once the green light is given > > Good to know, I can take care of that. > > > best regards, > > Julian > > > > gcc/c-family/ChangeLog: > > > > * c.opt: Introduce -Winvalid-noreturn=explicit and -Winvalid-noreturn=implicit > > > > gcc/ChangeLog: > > > > * tree-cfg.cc (pass_warn_function_return::execute): Use it > > > > gcc/c/ChangeLog: > > > > * c-typeck.cc (c_finish_return): Use it > > * gimple-parser.cc (c_finish_gimple_return): Use it > > > > gcc/config/mingw/ChangeLog: > > > > * mingw32.h (EXTRA_OS_CPP_BUILTINS): Fix semicolons > > > > gcc/cp/ChangeLog: > > > > * coroutines.cc (finish_co_return_stmt): Use it > > * typeck.cc (check_return_expr): Use it > > > > gcc/doc/ChangeLog: > > > > * invoke.texi: Document new options > > > > From 4daf884f8bbc1e318ba93121a6fdf4139da80b64 Mon Sep 17 00:00:00 2001 > > From: TheShermanTanker <tanksherman27@gmail.com> > > Date: Wed, 29 May 2024 21:32:08 +0800 > > Subject: [PATCH] Introduce the -Winvalid-noreturn flag from clang with extra > > tuneability > > The rationale and ChangeLog entries should be part of the commit message > (and so the git format-patch output). > > > > > Signed-off-by: TheShermanTanker <tanksherman27@gmail.com> > > A DCO sign-off can't use a pseudonym, sorry; please either sign off > using your real name or file a copyright assignment for the pseudonym > with the FSF. > > See https://gcc.gnu.org/contribute.html#legal for more detail. > > > --- > > gcc/c-family/c.opt | 8 ++++++++ > > gcc/c/c-typeck.cc | 2 +- > > gcc/c/gimple-parser.cc | 2 +- > > gcc/config/mingw/mingw32.h | 6 +++--- > > gcc/cp/coroutines.cc | 2 +- > > gcc/cp/typeck.cc | 2 +- > > gcc/doc/invoke.texi | 13 +++++++++++++ > > gcc/tree-cfg.cc | 2 +- > > 8 files changed, 29 insertions(+), 8 deletions(-) > > > > diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt > > index fb34c3b7031..32a2859fdcc 100644 > > --- a/gcc/c-family/c.opt > > +++ b/gcc/c-family/c.opt > > @@ -886,6 +886,14 @@ Winvalid-constexpr > > C++ ObjC++ Var(warn_invalid_constexpr) Init(-1) Warning > > Warn when a function never produces a constant expression. > > > > +Winvalid-noreturn=explicit > > +C ObjC C++ ObjC++ Warning > > +Warn when a function marked noreturn returns explicitly. > > + > > +Winvalid-noreturn=implicit > > +C ObjC C++ ObjC++ Warning > > +Warn when a function marked noreturn returns implicitly. > > + > > Winvalid-offsetof > > C++ ObjC++ Var(warn_invalid_offsetof) Init(1) Warning > > Warn about invalid uses of the \"offsetof\" macro. > > diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc > > index ad4c7add562..1941fbc44cb 100644 > > --- a/gcc/c/c-typeck.cc > > +++ b/gcc/c/c-typeck.cc > > @@ -11468,7 +11468,7 @@ c_finish_return (location_t loc, tree retval, tree origtype) > > location_t xloc = expansion_point_location_if_in_system_header (loc); > > > > if (TREE_THIS_VOLATILE (current_function_decl)) > > - warning_at (xloc, 0, > > + warning_at (xloc, OPT_Winvalid_noreturn_explicit, > > "function declared %<noreturn%> has a %<return%> statement"); > > > > if (retval) > > diff --git a/gcc/c/gimple-parser.cc b/gcc/c/gimple-parser.cc > > index d156d83cd37..1acaf75f844 100644 > > --- a/gcc/c/gimple-parser.cc > > +++ b/gcc/c/gimple-parser.cc > > @@ -2593,7 +2593,7 @@ c_finish_gimple_return (location_t loc, tree retval) > > location_t xloc = expansion_point_location_if_in_system_header (loc); > > > > if (TREE_THIS_VOLATILE (current_function_decl)) > > - warning_at (xloc, 0, > > + warning_at (xloc, OPT_Winvalid_noreturn_explicit, > > "function declared %<noreturn%> has a %<return%> statement"); > > > > if (! retval) > > diff --git a/gcc/config/mingw/mingw32.h b/gcc/config/mingw/mingw32.h > > index fa6e307476c..a69926133b1 100644 > > --- a/gcc/config/mingw/mingw32.h > > +++ b/gcc/config/mingw/mingw32.h > > @@ -35,9 +35,9 @@ along with GCC; see the file COPYING3. If not see > > | MASK_MS_BITFIELD_LAYOUT) > > > > #ifdef TARGET_USING_MCFGTHREAD > > -#define DEFINE_THREAD_MODEL builtin_define ("__USING_MCFGTHREAD__"); > > +#define DEFINE_THREAD_MODEL builtin_define ("__USING_MCFGTHREAD__") > > #elif defined(TARGET_USE_PTHREAD_BY_DEFAULT) > > -#define DEFINE_THREAD_MODEL builtin_define ("__USING_POSIXTHREAD__"); > > +#define DEFINE_THREAD_MODEL builtin_define ("__USING_POSIXTHREAD__") > > #else > > #define DEFINE_THREAD_MODEL > > #endif > > @@ -60,7 +60,7 @@ along with GCC; see the file COPYING3. If not see > > builtin_define_std ("WIN64"); \ > > builtin_define ("_WIN64"); \ > > } \ > > - DEFINE_THREAD_MODEL \ > > + DEFINE_THREAD_MODEL; \ > > } \ > > while (0) > > > > diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc > > index 97bc211ff67..53e56bd4959 100644 > > --- a/gcc/cp/coroutines.cc > > +++ b/gcc/cp/coroutines.cc > > @@ -1375,7 +1375,7 @@ finish_co_return_stmt (location_t kw, tree expr) > > > > /* Makes no sense for a co-routine really. */ > > if (TREE_THIS_VOLATILE (current_function_decl)) > > - warning_at (kw, 0, > > + warning_at (kw, OPT_Winvalid_noreturn_explicit, > > "function declared %<noreturn%> has a" > > " %<co_return%> statement"); > > > > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc > > index 1b7a31d32f3..74cc59bfb87 100644 > > --- a/gcc/cp/typeck.cc > > +++ b/gcc/cp/typeck.cc > > @@ -11062,7 +11062,7 @@ check_return_expr (tree retval, bool *no_warning, bool *dangling) > > (This is a G++ extension, used to get better code for functions > > that call the `volatile' function.) */ > > if (TREE_THIS_VOLATILE (current_function_decl)) > > - warning (0, "function declared %<noreturn%> has a %<return%> statement"); > > + warning (OPT_Winvalid_noreturn_explicit, "function declared %<noreturn%> has a %<return%> statement"); > > > > /* Check for various simple errors. */ > > if (DECL_DESTRUCTOR_P (current_function_decl)) > > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > > index 2cba380718b..27d880fd4f0 100644 > > --- a/gcc/doc/invoke.texi > > +++ b/gcc/doc/invoke.texi > > @@ -376,6 +376,7 @@ Objective-C and Objective-C++ Dialects}. > > -Winfinite-recursion > > -Winit-self -Winline -Wno-int-conversion -Wint-in-bool-context > > -Wno-int-to-pointer-cast -Wno-invalid-memory-model > > +-Winvalid-noreturn=explicit -Winvalid-noreturn=implicit > > -Winvalid-pch -Winvalid-utf8 -Wno-unicode -Wjump-misses-init > > -Wlarger-than=@var{byte-size} -Wlogical-not-parentheses -Wlogical-op > > -Wlong-long -Wno-lto-type-mismatch -Wmain -Wmaybe-uninitialized > > @@ -10482,6 +10483,18 @@ an error. @option{Wint-to-pointer-cast} is enabled by default. > > Suppress warnings from casts from a pointer to an integer type of a > > different size. > > > > +@opindex Winvalid-noreturn=explicit > > +@opindex Wno-invalid-noreturn=explicit > > +@item -Winvalid-noreturn=explicit > > +Warn if a function marked noreturn returns explicitly, that is, it > > +contains a return statement. > > + > > +@opindex Winvalid-noreturn=implicit > > +@opindex Wno-invalid-noreturn=implicit > > +@item -Winvalid-noreturn=implicit > > +Warn if a function marked noreturn returns implicitly, that is, it > > +has a path in its control flow that can reach the end of its body. > > + > > @opindex Winvalid-pch > > @opindex Wno-invalid-pch > > @item -Winvalid-pch > > diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc > > index 7fb7b92966b..cfe91038dd1 100644 > > --- a/gcc/tree-cfg.cc > > +++ b/gcc/tree-cfg.cc > > @@ -9817,7 +9817,7 @@ pass_warn_function_return::execute (function *fun) > > } > > if (location == UNKNOWN_LOCATION) > > location = cfun->function_end_locus; > > - warning_at (location, 0, "%<noreturn%> function does return"); > > + warning_at (location, OPT_Winvalid_noreturn_implicit, "%<noreturn%> function does return"); > > } > > > > /* If we see "return;" in some basic block, then we do reach the end >
On Sat, Jun 1, 2024 at 11:32 AM Julian Waters <tanksherman27@gmail.com> wrote: > > Hi Jason, > > Thanks for the reply! I'll address your comments soon. I have a > question, if there is an option defined in c.opt as an Enum, like > fstrong-eval-order, and the -no variant of the option is passed, would > the Var somehow reflect the negated option? Eg > > Winvalid-noreturn= > C ObjC C++ ObjC++ Var(warn_invalid_noreturn) Joined > Enum(invalid_noreturn) Warning > > Enum > Name(invalid_noreturn) Type(int) > > EnumValue > Enum(invalid_noreturn) String(explicit) Value(0) > > Would warn_invalid_noreturn then != 0 if > -Wno-invalid-noreturn=explicit is passed? Or is there a way to make a > warning call depend on 2 different OPT_ entries? > > best regards, > Julian This kind of issue is one of the reasons why I think it's better to just avoid the "=" character in warning option names, and to just make completely separate options using the "-" character instead... (i.e., -Winvalid-noreturn-implicit and -Winvalid-noreturn-explicit) > > On Sat, Jun 1, 2024 at 4:57 AM Jason Merrill <jason@redhat.com> wrote: > > > > On 5/29/24 09:58, Julian Waters wrote: > > > Currently, gcc warns about noreturn marked functions that return both explicitly and implicitly, with no way to turn this warning off. clang does have an option for these classes of warnings, -Winvalid-noreturn. However, we can do better. Instead of just having 1 option that switches the warnings for both on and off, we can define an extra layer of granularity, and have a separate options for implicit returns and explicit returns, as in -Winvalid-return=explicit and -Winvalid-noreturn=implicit. This patch adds both to gcc, for compatibility with clang. > > > > Thanks! > > > > > Do note that I am relatively new to gcc's codebase, and as such couldn't figure out how to cleanly define a general -Winvalid-noreturn warning that switch both on and off, for better compatibility with clang. If someone should point out how to do so, I'll happily rewrite my patch. > > > > See -fstrong-eval-order for an example of an option that can be used > > with or without =arg. > > > > > I also do not have write access to gcc, and will need help pushing this patch once the green light is given > > > > Good to know, I can take care of that. > > > > > best regards, > > > Julian > > > > > > gcc/c-family/ChangeLog: > > > > > > * c.opt: Introduce -Winvalid-noreturn=explicit and -Winvalid-noreturn=implicit > > > > > > gcc/ChangeLog: > > > > > > * tree-cfg.cc (pass_warn_function_return::execute): Use it > > > > > > gcc/c/ChangeLog: > > > > > > * c-typeck.cc (c_finish_return): Use it > > > * gimple-parser.cc (c_finish_gimple_return): Use it > > > > > > gcc/config/mingw/ChangeLog: > > > > > > * mingw32.h (EXTRA_OS_CPP_BUILTINS): Fix semicolons > > > > > > gcc/cp/ChangeLog: > > > > > > * coroutines.cc (finish_co_return_stmt): Use it > > > * typeck.cc (check_return_expr): Use it > > > > > > gcc/doc/ChangeLog: > > > > > > * invoke.texi: Document new options > > > > > > From 4daf884f8bbc1e318ba93121a6fdf4139da80b64 Mon Sep 17 00:00:00 2001 > > > From: TheShermanTanker <tanksherman27@gmail.com> > > > Date: Wed, 29 May 2024 21:32:08 +0800 > > > Subject: [PATCH] Introduce the -Winvalid-noreturn flag from clang with extra > > > tuneability > > > > The rationale and ChangeLog entries should be part of the commit message > > (and so the git format-patch output). > > > > > > > > Signed-off-by: TheShermanTanker <tanksherman27@gmail.com> > > > > A DCO sign-off can't use a pseudonym, sorry; please either sign off > > using your real name or file a copyright assignment for the pseudonym > > with the FSF. > > > > See https://gcc.gnu.org/contribute.html#legal for more detail. > > > > > --- > > > gcc/c-family/c.opt | 8 ++++++++ > > > gcc/c/c-typeck.cc | 2 +- > > > gcc/c/gimple-parser.cc | 2 +- > > > gcc/config/mingw/mingw32.h | 6 +++--- > > > gcc/cp/coroutines.cc | 2 +- > > > gcc/cp/typeck.cc | 2 +- > > > gcc/doc/invoke.texi | 13 +++++++++++++ > > > gcc/tree-cfg.cc | 2 +- > > > 8 files changed, 29 insertions(+), 8 deletions(-) > > > > > > diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt > > > index fb34c3b7031..32a2859fdcc 100644 > > > --- a/gcc/c-family/c.opt > > > +++ b/gcc/c-family/c.opt > > > @@ -886,6 +886,14 @@ Winvalid-constexpr > > > C++ ObjC++ Var(warn_invalid_constexpr) Init(-1) Warning > > > Warn when a function never produces a constant expression. > > > > > > +Winvalid-noreturn=explicit > > > +C ObjC C++ ObjC++ Warning > > > +Warn when a function marked noreturn returns explicitly. > > > + > > > +Winvalid-noreturn=implicit > > > +C ObjC C++ ObjC++ Warning > > > +Warn when a function marked noreturn returns implicitly. > > > + > > > Winvalid-offsetof > > > C++ ObjC++ Var(warn_invalid_offsetof) Init(1) Warning > > > Warn about invalid uses of the \"offsetof\" macro. > > > diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc > > > index ad4c7add562..1941fbc44cb 100644 > > > --- a/gcc/c/c-typeck.cc > > > +++ b/gcc/c/c-typeck.cc > > > @@ -11468,7 +11468,7 @@ c_finish_return (location_t loc, tree retval, tree origtype) > > > location_t xloc = expansion_point_location_if_in_system_header (loc); > > > > > > if (TREE_THIS_VOLATILE (current_function_decl)) > > > - warning_at (xloc, 0, > > > + warning_at (xloc, OPT_Winvalid_noreturn_explicit, > > > "function declared %<noreturn%> has a %<return%> statement"); > > > > > > if (retval) > > > diff --git a/gcc/c/gimple-parser.cc b/gcc/c/gimple-parser.cc > > > index d156d83cd37..1acaf75f844 100644 > > > --- a/gcc/c/gimple-parser.cc > > > +++ b/gcc/c/gimple-parser.cc > > > @@ -2593,7 +2593,7 @@ c_finish_gimple_return (location_t loc, tree retval) > > > location_t xloc = expansion_point_location_if_in_system_header (loc); > > > > > > if (TREE_THIS_VOLATILE (current_function_decl)) > > > - warning_at (xloc, 0, > > > + warning_at (xloc, OPT_Winvalid_noreturn_explicit, > > > "function declared %<noreturn%> has a %<return%> statement"); > > > > > > if (! retval) > > > diff --git a/gcc/config/mingw/mingw32.h b/gcc/config/mingw/mingw32.h > > > index fa6e307476c..a69926133b1 100644 > > > --- a/gcc/config/mingw/mingw32.h > > > +++ b/gcc/config/mingw/mingw32.h > > > @@ -35,9 +35,9 @@ along with GCC; see the file COPYING3. If not see > > > | MASK_MS_BITFIELD_LAYOUT) > > > > > > #ifdef TARGET_USING_MCFGTHREAD > > > -#define DEFINE_THREAD_MODEL builtin_define ("__USING_MCFGTHREAD__"); > > > +#define DEFINE_THREAD_MODEL builtin_define ("__USING_MCFGTHREAD__") > > > #elif defined(TARGET_USE_PTHREAD_BY_DEFAULT) > > > -#define DEFINE_THREAD_MODEL builtin_define ("__USING_POSIXTHREAD__"); > > > +#define DEFINE_THREAD_MODEL builtin_define ("__USING_POSIXTHREAD__") > > > #else > > > #define DEFINE_THREAD_MODEL > > > #endif > > > @@ -60,7 +60,7 @@ along with GCC; see the file COPYING3. If not see > > > builtin_define_std ("WIN64"); \ > > > builtin_define ("_WIN64"); \ > > > } \ > > > - DEFINE_THREAD_MODEL \ > > > + DEFINE_THREAD_MODEL; \ > > > } \ > > > while (0) > > > > > > diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc > > > index 97bc211ff67..53e56bd4959 100644 > > > --- a/gcc/cp/coroutines.cc > > > +++ b/gcc/cp/coroutines.cc > > > @@ -1375,7 +1375,7 @@ finish_co_return_stmt (location_t kw, tree expr) > > > > > > /* Makes no sense for a co-routine really. */ > > > if (TREE_THIS_VOLATILE (current_function_decl)) > > > - warning_at (kw, 0, > > > + warning_at (kw, OPT_Winvalid_noreturn_explicit, > > > "function declared %<noreturn%> has a" > > > " %<co_return%> statement"); > > > > > > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc > > > index 1b7a31d32f3..74cc59bfb87 100644 > > > --- a/gcc/cp/typeck.cc > > > +++ b/gcc/cp/typeck.cc > > > @@ -11062,7 +11062,7 @@ check_return_expr (tree retval, bool *no_warning, bool *dangling) > > > (This is a G++ extension, used to get better code for functions > > > that call the `volatile' function.) */ > > > if (TREE_THIS_VOLATILE (current_function_decl)) > > > - warning (0, "function declared %<noreturn%> has a %<return%> statement"); > > > + warning (OPT_Winvalid_noreturn_explicit, "function declared %<noreturn%> has a %<return%> statement"); > > > > > > /* Check for various simple errors. */ > > > if (DECL_DESTRUCTOR_P (current_function_decl)) > > > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > > > index 2cba380718b..27d880fd4f0 100644 > > > --- a/gcc/doc/invoke.texi > > > +++ b/gcc/doc/invoke.texi > > > @@ -376,6 +376,7 @@ Objective-C and Objective-C++ Dialects}. > > > -Winfinite-recursion > > > -Winit-self -Winline -Wno-int-conversion -Wint-in-bool-context > > > -Wno-int-to-pointer-cast -Wno-invalid-memory-model > > > +-Winvalid-noreturn=explicit -Winvalid-noreturn=implicit > > > -Winvalid-pch -Winvalid-utf8 -Wno-unicode -Wjump-misses-init > > > -Wlarger-than=@var{byte-size} -Wlogical-not-parentheses -Wlogical-op > > > -Wlong-long -Wno-lto-type-mismatch -Wmain -Wmaybe-uninitialized > > > @@ -10482,6 +10483,18 @@ an error. @option{Wint-to-pointer-cast} is enabled by default. > > > Suppress warnings from casts from a pointer to an integer type of a > > > different size. > > > > > > +@opindex Winvalid-noreturn=explicit > > > +@opindex Wno-invalid-noreturn=explicit > > > +@item -Winvalid-noreturn=explicit > > > +Warn if a function marked noreturn returns explicitly, that is, it > > > +contains a return statement. > > > + > > > +@opindex Winvalid-noreturn=implicit > > > +@opindex Wno-invalid-noreturn=implicit > > > +@item -Winvalid-noreturn=implicit > > > +Warn if a function marked noreturn returns implicitly, that is, it > > > +has a path in its control flow that can reach the end of its body. > > > + > > > @opindex Winvalid-pch > > > @opindex Wno-invalid-pch > > > @item -Winvalid-pch > > > diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc > > > index 7fb7b92966b..cfe91038dd1 100644 > > > --- a/gcc/tree-cfg.cc > > > +++ b/gcc/tree-cfg.cc > > > @@ -9817,7 +9817,7 @@ pass_warn_function_return::execute (function *fun) > > > } > > > if (location == UNKNOWN_LOCATION) > > > location = cfun->function_end_locus; > > > - warning_at (location, 0, "%<noreturn%> function does return"); > > > + warning_at (location, OPT_Winvalid_noreturn_implicit, "%<noreturn%> function does return"); > > > } > > > > > > /* If we see "return;" in some basic block, then we do reach the end > >
On 6/1/24 11:31, Julian Waters wrote: > Hi Jason, > > Thanks for the reply! I'll address your comments soon. I have a > question, if there is an option defined in c.opt as an Enum, like > fstrong-eval-order, and the -no variant of the option is passed, would > the Var somehow reflect the negated option? Eg > > Winvalid-noreturn= > C ObjC C++ ObjC++ Var(warn_invalid_noreturn) Joined > Enum(invalid_noreturn) Warning > > Enum > Name(invalid_noreturn) Type(int) > > EnumValue > Enum(invalid_noreturn) String(explicit) Value(0) -fstrong-eval-order has fstrong-eval-order C++ ObjC++ Common Alias(fstrong-eval-order=, all, none) to represent that plain -fstrong-eval-order is equivalent to -fstrong-eval-order=all, and -fno-strong-eval-order is equivalent to =none. > Would warn_invalid_noreturn then != 0 if > -Wno-invalid-noreturn=explicit is passed? Or is there a way to make a > warning call depend on 2 different OPT_ entries? Typically = options will specify RejectNegative so the driver will reject e.g. -Wno-invalid-noreturn=explicit. Jason > best regards, > Julian > > On Sat, Jun 1, 2024 at 4:57 AM Jason Merrill <jason@redhat.com> wrote: >> >> On 5/29/24 09:58, Julian Waters wrote: >>> Currently, gcc warns about noreturn marked functions that return both explicitly and implicitly, with no way to turn this warning off. clang does have an option for these classes of warnings, -Winvalid-noreturn. However, we can do better. Instead of just having 1 option that switches the warnings for both on and off, we can define an extra layer of granularity, and have a separate options for implicit returns and explicit returns, as in -Winvalid-return=explicit and -Winvalid-noreturn=implicit. This patch adds both to gcc, for compatibility with clang. >> >> Thanks! >> >>> Do note that I am relatively new to gcc's codebase, and as such couldn't figure out how to cleanly define a general -Winvalid-noreturn warning that switch both on and off, for better compatibility with clang. If someone should point out how to do so, I'll happily rewrite my patch. >> >> See -fstrong-eval-order for an example of an option that can be used >> with or without =arg. >> >>> I also do not have write access to gcc, and will need help pushing this patch once the green light is given >> >> Good to know, I can take care of that. >> >>> best regards, >>> Julian >>> >>> gcc/c-family/ChangeLog: >>> >>> * c.opt: Introduce -Winvalid-noreturn=explicit and -Winvalid-noreturn=implicit >>> >>> gcc/ChangeLog: >>> >>> * tree-cfg.cc (pass_warn_function_return::execute): Use it >>> >>> gcc/c/ChangeLog: >>> >>> * c-typeck.cc (c_finish_return): Use it >>> * gimple-parser.cc (c_finish_gimple_return): Use it >>> >>> gcc/config/mingw/ChangeLog: >>> >>> * mingw32.h (EXTRA_OS_CPP_BUILTINS): Fix semicolons >>> >>> gcc/cp/ChangeLog: >>> >>> * coroutines.cc (finish_co_return_stmt): Use it >>> * typeck.cc (check_return_expr): Use it >>> >>> gcc/doc/ChangeLog: >>> >>> * invoke.texi: Document new options >>> >>> From 4daf884f8bbc1e318ba93121a6fdf4139da80b64 Mon Sep 17 00:00:00 2001 >>> From: TheShermanTanker <tanksherman27@gmail.com> >>> Date: Wed, 29 May 2024 21:32:08 +0800 >>> Subject: [PATCH] Introduce the -Winvalid-noreturn flag from clang with extra >>> tuneability >> >> The rationale and ChangeLog entries should be part of the commit message >> (and so the git format-patch output). >> >>> >>> Signed-off-by: TheShermanTanker <tanksherman27@gmail.com> >> >> A DCO sign-off can't use a pseudonym, sorry; please either sign off >> using your real name or file a copyright assignment for the pseudonym >> with the FSF. >> >> See https://gcc.gnu.org/contribute.html#legal for more detail. >> >>> --- >>> gcc/c-family/c.opt | 8 ++++++++ >>> gcc/c/c-typeck.cc | 2 +- >>> gcc/c/gimple-parser.cc | 2 +- >>> gcc/config/mingw/mingw32.h | 6 +++--- >>> gcc/cp/coroutines.cc | 2 +- >>> gcc/cp/typeck.cc | 2 +- >>> gcc/doc/invoke.texi | 13 +++++++++++++ >>> gcc/tree-cfg.cc | 2 +- >>> 8 files changed, 29 insertions(+), 8 deletions(-) >>> >>> diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt >>> index fb34c3b7031..32a2859fdcc 100644 >>> --- a/gcc/c-family/c.opt >>> +++ b/gcc/c-family/c.opt >>> @@ -886,6 +886,14 @@ Winvalid-constexpr >>> C++ ObjC++ Var(warn_invalid_constexpr) Init(-1) Warning >>> Warn when a function never produces a constant expression. >>> >>> +Winvalid-noreturn=explicit >>> +C ObjC C++ ObjC++ Warning >>> +Warn when a function marked noreturn returns explicitly. >>> + >>> +Winvalid-noreturn=implicit >>> +C ObjC C++ ObjC++ Warning >>> +Warn when a function marked noreturn returns implicitly. >>> + >>> Winvalid-offsetof >>> C++ ObjC++ Var(warn_invalid_offsetof) Init(1) Warning >>> Warn about invalid uses of the \"offsetof\" macro. >>> diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc >>> index ad4c7add562..1941fbc44cb 100644 >>> --- a/gcc/c/c-typeck.cc >>> +++ b/gcc/c/c-typeck.cc >>> @@ -11468,7 +11468,7 @@ c_finish_return (location_t loc, tree retval, tree origtype) >>> location_t xloc = expansion_point_location_if_in_system_header (loc); >>> >>> if (TREE_THIS_VOLATILE (current_function_decl)) >>> - warning_at (xloc, 0, >>> + warning_at (xloc, OPT_Winvalid_noreturn_explicit, >>> "function declared %<noreturn%> has a %<return%> statement"); >>> >>> if (retval) >>> diff --git a/gcc/c/gimple-parser.cc b/gcc/c/gimple-parser.cc >>> index d156d83cd37..1acaf75f844 100644 >>> --- a/gcc/c/gimple-parser.cc >>> +++ b/gcc/c/gimple-parser.cc >>> @@ -2593,7 +2593,7 @@ c_finish_gimple_return (location_t loc, tree retval) >>> location_t xloc = expansion_point_location_if_in_system_header (loc); >>> >>> if (TREE_THIS_VOLATILE (current_function_decl)) >>> - warning_at (xloc, 0, >>> + warning_at (xloc, OPT_Winvalid_noreturn_explicit, >>> "function declared %<noreturn%> has a %<return%> statement"); >>> >>> if (! retval) >>> diff --git a/gcc/config/mingw/mingw32.h b/gcc/config/mingw/mingw32.h >>> index fa6e307476c..a69926133b1 100644 >>> --- a/gcc/config/mingw/mingw32.h >>> +++ b/gcc/config/mingw/mingw32.h >>> @@ -35,9 +35,9 @@ along with GCC; see the file COPYING3. If not see >>> | MASK_MS_BITFIELD_LAYOUT) >>> >>> #ifdef TARGET_USING_MCFGTHREAD >>> -#define DEFINE_THREAD_MODEL builtin_define ("__USING_MCFGTHREAD__"); >>> +#define DEFINE_THREAD_MODEL builtin_define ("__USING_MCFGTHREAD__") >>> #elif defined(TARGET_USE_PTHREAD_BY_DEFAULT) >>> -#define DEFINE_THREAD_MODEL builtin_define ("__USING_POSIXTHREAD__"); >>> +#define DEFINE_THREAD_MODEL builtin_define ("__USING_POSIXTHREAD__") >>> #else >>> #define DEFINE_THREAD_MODEL >>> #endif >>> @@ -60,7 +60,7 @@ along with GCC; see the file COPYING3. If not see >>> builtin_define_std ("WIN64"); \ >>> builtin_define ("_WIN64"); \ >>> } \ >>> - DEFINE_THREAD_MODEL \ >>> + DEFINE_THREAD_MODEL; \ >>> } \ >>> while (0) >>> >>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc >>> index 97bc211ff67..53e56bd4959 100644 >>> --- a/gcc/cp/coroutines.cc >>> +++ b/gcc/cp/coroutines.cc >>> @@ -1375,7 +1375,7 @@ finish_co_return_stmt (location_t kw, tree expr) >>> >>> /* Makes no sense for a co-routine really. */ >>> if (TREE_THIS_VOLATILE (current_function_decl)) >>> - warning_at (kw, 0, >>> + warning_at (kw, OPT_Winvalid_noreturn_explicit, >>> "function declared %<noreturn%> has a" >>> " %<co_return%> statement"); >>> >>> diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc >>> index 1b7a31d32f3..74cc59bfb87 100644 >>> --- a/gcc/cp/typeck.cc >>> +++ b/gcc/cp/typeck.cc >>> @@ -11062,7 +11062,7 @@ check_return_expr (tree retval, bool *no_warning, bool *dangling) >>> (This is a G++ extension, used to get better code for functions >>> that call the `volatile' function.) */ >>> if (TREE_THIS_VOLATILE (current_function_decl)) >>> - warning (0, "function declared %<noreturn%> has a %<return%> statement"); >>> + warning (OPT_Winvalid_noreturn_explicit, "function declared %<noreturn%> has a %<return%> statement"); >>> >>> /* Check for various simple errors. */ >>> if (DECL_DESTRUCTOR_P (current_function_decl)) >>> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi >>> index 2cba380718b..27d880fd4f0 100644 >>> --- a/gcc/doc/invoke.texi >>> +++ b/gcc/doc/invoke.texi >>> @@ -376,6 +376,7 @@ Objective-C and Objective-C++ Dialects}. >>> -Winfinite-recursion >>> -Winit-self -Winline -Wno-int-conversion -Wint-in-bool-context >>> -Wno-int-to-pointer-cast -Wno-invalid-memory-model >>> +-Winvalid-noreturn=explicit -Winvalid-noreturn=implicit >>> -Winvalid-pch -Winvalid-utf8 -Wno-unicode -Wjump-misses-init >>> -Wlarger-than=@var{byte-size} -Wlogical-not-parentheses -Wlogical-op >>> -Wlong-long -Wno-lto-type-mismatch -Wmain -Wmaybe-uninitialized >>> @@ -10482,6 +10483,18 @@ an error. @option{Wint-to-pointer-cast} is enabled by default. >>> Suppress warnings from casts from a pointer to an integer type of a >>> different size. >>> >>> +@opindex Winvalid-noreturn=explicit >>> +@opindex Wno-invalid-noreturn=explicit >>> +@item -Winvalid-noreturn=explicit >>> +Warn if a function marked noreturn returns explicitly, that is, it >>> +contains a return statement. >>> + >>> +@opindex Winvalid-noreturn=implicit >>> +@opindex Wno-invalid-noreturn=implicit >>> +@item -Winvalid-noreturn=implicit >>> +Warn if a function marked noreturn returns implicitly, that is, it >>> +has a path in its control flow that can reach the end of its body. >>> + >>> @opindex Winvalid-pch >>> @opindex Wno-invalid-pch >>> @item -Winvalid-pch >>> diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc >>> index 7fb7b92966b..cfe91038dd1 100644 >>> --- a/gcc/tree-cfg.cc >>> +++ b/gcc/tree-cfg.cc >>> @@ -9817,7 +9817,7 @@ pass_warn_function_return::execute (function *fun) >>> } >>> if (location == UNKNOWN_LOCATION) >>> location = cfun->function_end_locus; >>> - warning_at (location, 0, "%<noreturn%> function does return"); >>> + warning_at (location, OPT_Winvalid_noreturn_implicit, "%<noreturn%> function does return"); >>> } >>> >>> /* If we see "return;" in some basic block, then we do reach the end >> >
Hi Jason, Thanks for the reply. I'm a little bit overwhelmed with university at the moment, would it be ok if I delay implementing this a little bit? best regards, Julian On Tue, Jun 4, 2024 at 1:04 AM Jason Merrill <jason@redhat.com> wrote: > > On 6/1/24 11:31, Julian Waters wrote: > > Hi Jason, > > > > Thanks for the reply! I'll address your comments soon. I have a > > question, if there is an option defined in c.opt as an Enum, like > > fstrong-eval-order, and the -no variant of the option is passed, would > > the Var somehow reflect the negated option? Eg > > > > Winvalid-noreturn= > > C ObjC C++ ObjC++ Var(warn_invalid_noreturn) Joined > > Enum(invalid_noreturn) Warning > > > > Enum > > Name(invalid_noreturn) Type(int) > > > > EnumValue > > Enum(invalid_noreturn) String(explicit) Value(0) > > -fstrong-eval-order has > > fstrong-eval-order > C++ ObjC++ Common Alias(fstrong-eval-order=, all, none) > > to represent that plain -fstrong-eval-order is equivalent to > -fstrong-eval-order=all, and -fno-strong-eval-order is equivalent to =none. > > > Would warn_invalid_noreturn then != 0 if > > -Wno-invalid-noreturn=explicit is passed? Or is there a way to make a > > warning call depend on 2 different OPT_ entries? > > Typically = options will specify RejectNegative so the driver will > reject e.g. -Wno-invalid-noreturn=explicit. > > Jason > > > best regards, > > Julian > > > > On Sat, Jun 1, 2024 at 4:57 AM Jason Merrill <jason@redhat.com> wrote: > >> > >> On 5/29/24 09:58, Julian Waters wrote: > >>> Currently, gcc warns about noreturn marked functions that return both explicitly and implicitly, with no way to turn this warning off. clang does have an option for these classes of warnings, -Winvalid-noreturn. However, we can do better. Instead of just having 1 option that switches the warnings for both on and off, we can define an extra layer of granularity, and have a separate options for implicit returns and explicit returns, as in -Winvalid-return=explicit and -Winvalid-noreturn=implicit. This patch adds both to gcc, for compatibility with clang. > >> > >> Thanks! > >> > >>> Do note that I am relatively new to gcc's codebase, and as such couldn't figure out how to cleanly define a general -Winvalid-noreturn warning that switch both on and off, for better compatibility with clang. If someone should point out how to do so, I'll happily rewrite my patch. > >> > >> See -fstrong-eval-order for an example of an option that can be used > >> with or without =arg. > >> > >>> I also do not have write access to gcc, and will need help pushing this patch once the green light is given > >> > >> Good to know, I can take care of that. > >> > >>> best regards, > >>> Julian > >>> > >>> gcc/c-family/ChangeLog: > >>> > >>> * c.opt: Introduce -Winvalid-noreturn=explicit and -Winvalid-noreturn=implicit > >>> > >>> gcc/ChangeLog: > >>> > >>> * tree-cfg.cc (pass_warn_function_return::execute): Use it > >>> > >>> gcc/c/ChangeLog: > >>> > >>> * c-typeck.cc (c_finish_return): Use it > >>> * gimple-parser.cc (c_finish_gimple_return): Use it > >>> > >>> gcc/config/mingw/ChangeLog: > >>> > >>> * mingw32.h (EXTRA_OS_CPP_BUILTINS): Fix semicolons > >>> > >>> gcc/cp/ChangeLog: > >>> > >>> * coroutines.cc (finish_co_return_stmt): Use it > >>> * typeck.cc (check_return_expr): Use it > >>> > >>> gcc/doc/ChangeLog: > >>> > >>> * invoke.texi: Document new options > >>> > >>> From 4daf884f8bbc1e318ba93121a6fdf4139da80b64 Mon Sep 17 00:00:00 2001 > >>> From: TheShermanTanker <tanksherman27@gmail.com> > >>> Date: Wed, 29 May 2024 21:32:08 +0800 > >>> Subject: [PATCH] Introduce the -Winvalid-noreturn flag from clang with extra > >>> tuneability > >> > >> The rationale and ChangeLog entries should be part of the commit message > >> (and so the git format-patch output). > >> > >>> > >>> Signed-off-by: TheShermanTanker <tanksherman27@gmail.com> > >> > >> A DCO sign-off can't use a pseudonym, sorry; please either sign off > >> using your real name or file a copyright assignment for the pseudonym > >> with the FSF. > >> > >> See https://gcc.gnu.org/contribute.html#legal for more detail. > >> > >>> --- > >>> gcc/c-family/c.opt | 8 ++++++++ > >>> gcc/c/c-typeck.cc | 2 +- > >>> gcc/c/gimple-parser.cc | 2 +- > >>> gcc/config/mingw/mingw32.h | 6 +++--- > >>> gcc/cp/coroutines.cc | 2 +- > >>> gcc/cp/typeck.cc | 2 +- > >>> gcc/doc/invoke.texi | 13 +++++++++++++ > >>> gcc/tree-cfg.cc | 2 +- > >>> 8 files changed, 29 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt > >>> index fb34c3b7031..32a2859fdcc 100644 > >>> --- a/gcc/c-family/c.opt > >>> +++ b/gcc/c-family/c.opt > >>> @@ -886,6 +886,14 @@ Winvalid-constexpr > >>> C++ ObjC++ Var(warn_invalid_constexpr) Init(-1) Warning > >>> Warn when a function never produces a constant expression. > >>> > >>> +Winvalid-noreturn=explicit > >>> +C ObjC C++ ObjC++ Warning > >>> +Warn when a function marked noreturn returns explicitly. > >>> + > >>> +Winvalid-noreturn=implicit > >>> +C ObjC C++ ObjC++ Warning > >>> +Warn when a function marked noreturn returns implicitly. > >>> + > >>> Winvalid-offsetof > >>> C++ ObjC++ Var(warn_invalid_offsetof) Init(1) Warning > >>> Warn about invalid uses of the \"offsetof\" macro. > >>> diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc > >>> index ad4c7add562..1941fbc44cb 100644 > >>> --- a/gcc/c/c-typeck.cc > >>> +++ b/gcc/c/c-typeck.cc > >>> @@ -11468,7 +11468,7 @@ c_finish_return (location_t loc, tree retval, tree origtype) > >>> location_t xloc = expansion_point_location_if_in_system_header (loc); > >>> > >>> if (TREE_THIS_VOLATILE (current_function_decl)) > >>> - warning_at (xloc, 0, > >>> + warning_at (xloc, OPT_Winvalid_noreturn_explicit, > >>> "function declared %<noreturn%> has a %<return%> statement"); > >>> > >>> if (retval) > >>> diff --git a/gcc/c/gimple-parser.cc b/gcc/c/gimple-parser.cc > >>> index d156d83cd37..1acaf75f844 100644 > >>> --- a/gcc/c/gimple-parser.cc > >>> +++ b/gcc/c/gimple-parser.cc > >>> @@ -2593,7 +2593,7 @@ c_finish_gimple_return (location_t loc, tree retval) > >>> location_t xloc = expansion_point_location_if_in_system_header (loc); > >>> > >>> if (TREE_THIS_VOLATILE (current_function_decl)) > >>> - warning_at (xloc, 0, > >>> + warning_at (xloc, OPT_Winvalid_noreturn_explicit, > >>> "function declared %<noreturn%> has a %<return%> statement"); > >>> > >>> if (! retval) > >>> diff --git a/gcc/config/mingw/mingw32.h b/gcc/config/mingw/mingw32.h > >>> index fa6e307476c..a69926133b1 100644 > >>> --- a/gcc/config/mingw/mingw32.h > >>> +++ b/gcc/config/mingw/mingw32.h > >>> @@ -35,9 +35,9 @@ along with GCC; see the file COPYING3. If not see > >>> | MASK_MS_BITFIELD_LAYOUT) > >>> > >>> #ifdef TARGET_USING_MCFGTHREAD > >>> -#define DEFINE_THREAD_MODEL builtin_define ("__USING_MCFGTHREAD__"); > >>> +#define DEFINE_THREAD_MODEL builtin_define ("__USING_MCFGTHREAD__") > >>> #elif defined(TARGET_USE_PTHREAD_BY_DEFAULT) > >>> -#define DEFINE_THREAD_MODEL builtin_define ("__USING_POSIXTHREAD__"); > >>> +#define DEFINE_THREAD_MODEL builtin_define ("__USING_POSIXTHREAD__") > >>> #else > >>> #define DEFINE_THREAD_MODEL > >>> #endif > >>> @@ -60,7 +60,7 @@ along with GCC; see the file COPYING3. If not see > >>> builtin_define_std ("WIN64"); \ > >>> builtin_define ("_WIN64"); \ > >>> } \ > >>> - DEFINE_THREAD_MODEL \ > >>> + DEFINE_THREAD_MODEL; \ > >>> } \ > >>> while (0) > >>> > >>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc > >>> index 97bc211ff67..53e56bd4959 100644 > >>> --- a/gcc/cp/coroutines.cc > >>> +++ b/gcc/cp/coroutines.cc > >>> @@ -1375,7 +1375,7 @@ finish_co_return_stmt (location_t kw, tree expr) > >>> > >>> /* Makes no sense for a co-routine really. */ > >>> if (TREE_THIS_VOLATILE (current_function_decl)) > >>> - warning_at (kw, 0, > >>> + warning_at (kw, OPT_Winvalid_noreturn_explicit, > >>> "function declared %<noreturn%> has a" > >>> " %<co_return%> statement"); > >>> > >>> diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc > >>> index 1b7a31d32f3..74cc59bfb87 100644 > >>> --- a/gcc/cp/typeck.cc > >>> +++ b/gcc/cp/typeck.cc > >>> @@ -11062,7 +11062,7 @@ check_return_expr (tree retval, bool *no_warning, bool *dangling) > >>> (This is a G++ extension, used to get better code for functions > >>> that call the `volatile' function.) */ > >>> if (TREE_THIS_VOLATILE (current_function_decl)) > >>> - warning (0, "function declared %<noreturn%> has a %<return%> statement"); > >>> + warning (OPT_Winvalid_noreturn_explicit, "function declared %<noreturn%> has a %<return%> statement"); > >>> > >>> /* Check for various simple errors. */ > >>> if (DECL_DESTRUCTOR_P (current_function_decl)) > >>> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > >>> index 2cba380718b..27d880fd4f0 100644 > >>> --- a/gcc/doc/invoke.texi > >>> +++ b/gcc/doc/invoke.texi > >>> @@ -376,6 +376,7 @@ Objective-C and Objective-C++ Dialects}. > >>> -Winfinite-recursion > >>> -Winit-self -Winline -Wno-int-conversion -Wint-in-bool-context > >>> -Wno-int-to-pointer-cast -Wno-invalid-memory-model > >>> +-Winvalid-noreturn=explicit -Winvalid-noreturn=implicit > >>> -Winvalid-pch -Winvalid-utf8 -Wno-unicode -Wjump-misses-init > >>> -Wlarger-than=@var{byte-size} -Wlogical-not-parentheses -Wlogical-op > >>> -Wlong-long -Wno-lto-type-mismatch -Wmain -Wmaybe-uninitialized > >>> @@ -10482,6 +10483,18 @@ an error. @option{Wint-to-pointer-cast} is enabled by default. > >>> Suppress warnings from casts from a pointer to an integer type of a > >>> different size. > >>> > >>> +@opindex Winvalid-noreturn=explicit > >>> +@opindex Wno-invalid-noreturn=explicit > >>> +@item -Winvalid-noreturn=explicit > >>> +Warn if a function marked noreturn returns explicitly, that is, it > >>> +contains a return statement. > >>> + > >>> +@opindex Winvalid-noreturn=implicit > >>> +@opindex Wno-invalid-noreturn=implicit > >>> +@item -Winvalid-noreturn=implicit > >>> +Warn if a function marked noreturn returns implicitly, that is, it > >>> +has a path in its control flow that can reach the end of its body. > >>> + > >>> @opindex Winvalid-pch > >>> @opindex Wno-invalid-pch > >>> @item -Winvalid-pch > >>> diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc > >>> index 7fb7b92966b..cfe91038dd1 100644 > >>> --- a/gcc/tree-cfg.cc > >>> +++ b/gcc/tree-cfg.cc > >>> @@ -9817,7 +9817,7 @@ pass_warn_function_return::execute (function *fun) > >>> } > >>> if (location == UNKNOWN_LOCATION) > >>> location = cfun->function_end_locus; > >>> - warning_at (location, 0, "%<noreturn%> function does return"); > >>> + warning_at (location, OPT_Winvalid_noreturn_implicit, "%<noreturn%> function does return"); > >>> } > >>> > >>> /* If we see "return;" in some basic block, then we do reach the end > >> > > >
On 6/10/24 03:13, Julian Waters wrote: > Hi Jason, > > Thanks for the reply. I'm a little bit overwhelmed with university at > the moment, would it be ok if I delay implementing this a little bit? Sure, we're still early in GCC 15 development, no time pressure. > On Tue, Jun 4, 2024 at 1:04 AM Jason Merrill <jason@redhat.com> wrote: >> >> On 6/1/24 11:31, Julian Waters wrote: >>> Hi Jason, >>> >>> Thanks for the reply! I'll address your comments soon. I have a >>> question, if there is an option defined in c.opt as an Enum, like >>> fstrong-eval-order, and the -no variant of the option is passed, would >>> the Var somehow reflect the negated option? Eg >>> >>> Winvalid-noreturn= >>> C ObjC C++ ObjC++ Var(warn_invalid_noreturn) Joined >>> Enum(invalid_noreturn) Warning >>> >>> Enum >>> Name(invalid_noreturn) Type(int) >>> >>> EnumValue >>> Enum(invalid_noreturn) String(explicit) Value(0) >> >> -fstrong-eval-order has >> >> fstrong-eval-order >> C++ ObjC++ Common Alias(fstrong-eval-order=, all, none) >> >> to represent that plain -fstrong-eval-order is equivalent to >> -fstrong-eval-order=all, and -fno-strong-eval-order is equivalent to =none. >> >>> Would warn_invalid_noreturn then != 0 if >>> -Wno-invalid-noreturn=explicit is passed? Or is there a way to make a >>> warning call depend on 2 different OPT_ entries? >> >> Typically = options will specify RejectNegative so the driver will >> reject e.g. -Wno-invalid-noreturn=explicit. >> >> Jason >> >>> best regards, >>> Julian >>> >>> On Sat, Jun 1, 2024 at 4:57 AM Jason Merrill <jason@redhat.com> wrote: >>>> >>>> On 5/29/24 09:58, Julian Waters wrote: >>>>> Currently, gcc warns about noreturn marked functions that return both explicitly and implicitly, with no way to turn this warning off. clang does have an option for these classes of warnings, -Winvalid-noreturn. However, we can do better. Instead of just having 1 option that switches the warnings for both on and off, we can define an extra layer of granularity, and have a separate options for implicit returns and explicit returns, as in -Winvalid-return=explicit and -Winvalid-noreturn=implicit. This patch adds both to gcc, for compatibility with clang. >>>> >>>> Thanks! >>>> >>>>> Do note that I am relatively new to gcc's codebase, and as such couldn't figure out how to cleanly define a general -Winvalid-noreturn warning that switch both on and off, for better compatibility with clang. If someone should point out how to do so, I'll happily rewrite my patch. >>>> >>>> See -fstrong-eval-order for an example of an option that can be used >>>> with or without =arg. >>>> >>>>> I also do not have write access to gcc, and will need help pushing this patch once the green light is given >>>> >>>> Good to know, I can take care of that. >>>> >>>>> best regards, >>>>> Julian >>>>> >>>>> gcc/c-family/ChangeLog: >>>>> >>>>> * c.opt: Introduce -Winvalid-noreturn=explicit and -Winvalid-noreturn=implicit >>>>> >>>>> gcc/ChangeLog: >>>>> >>>>> * tree-cfg.cc (pass_warn_function_return::execute): Use it >>>>> >>>>> gcc/c/ChangeLog: >>>>> >>>>> * c-typeck.cc (c_finish_return): Use it >>>>> * gimple-parser.cc (c_finish_gimple_return): Use it >>>>> >>>>> gcc/config/mingw/ChangeLog: >>>>> >>>>> * mingw32.h (EXTRA_OS_CPP_BUILTINS): Fix semicolons >>>>> >>>>> gcc/cp/ChangeLog: >>>>> >>>>> * coroutines.cc (finish_co_return_stmt): Use it >>>>> * typeck.cc (check_return_expr): Use it >>>>> >>>>> gcc/doc/ChangeLog: >>>>> >>>>> * invoke.texi: Document new options >>>>> >>>>> From 4daf884f8bbc1e318ba93121a6fdf4139da80b64 Mon Sep 17 00:00:00 2001 >>>>> From: TheShermanTanker <tanksherman27@gmail.com> >>>>> Date: Wed, 29 May 2024 21:32:08 +0800 >>>>> Subject: [PATCH] Introduce the -Winvalid-noreturn flag from clang with extra >>>>> tuneability >>>> >>>> The rationale and ChangeLog entries should be part of the commit message >>>> (and so the git format-patch output). >>>> >>>>> >>>>> Signed-off-by: TheShermanTanker <tanksherman27@gmail.com> >>>> >>>> A DCO sign-off can't use a pseudonym, sorry; please either sign off >>>> using your real name or file a copyright assignment for the pseudonym >>>> with the FSF. >>>> >>>> See https://gcc.gnu.org/contribute.html#legal for more detail. >>>> >>>>> --- >>>>> gcc/c-family/c.opt | 8 ++++++++ >>>>> gcc/c/c-typeck.cc | 2 +- >>>>> gcc/c/gimple-parser.cc | 2 +- >>>>> gcc/config/mingw/mingw32.h | 6 +++--- >>>>> gcc/cp/coroutines.cc | 2 +- >>>>> gcc/cp/typeck.cc | 2 +- >>>>> gcc/doc/invoke.texi | 13 +++++++++++++ >>>>> gcc/tree-cfg.cc | 2 +- >>>>> 8 files changed, 29 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt >>>>> index fb34c3b7031..32a2859fdcc 100644 >>>>> --- a/gcc/c-family/c.opt >>>>> +++ b/gcc/c-family/c.opt >>>>> @@ -886,6 +886,14 @@ Winvalid-constexpr >>>>> C++ ObjC++ Var(warn_invalid_constexpr) Init(-1) Warning >>>>> Warn when a function never produces a constant expression. >>>>> >>>>> +Winvalid-noreturn=explicit >>>>> +C ObjC C++ ObjC++ Warning >>>>> +Warn when a function marked noreturn returns explicitly. >>>>> + >>>>> +Winvalid-noreturn=implicit >>>>> +C ObjC C++ ObjC++ Warning >>>>> +Warn when a function marked noreturn returns implicitly. >>>>> + >>>>> Winvalid-offsetof >>>>> C++ ObjC++ Var(warn_invalid_offsetof) Init(1) Warning >>>>> Warn about invalid uses of the \"offsetof\" macro. >>>>> diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc >>>>> index ad4c7add562..1941fbc44cb 100644 >>>>> --- a/gcc/c/c-typeck.cc >>>>> +++ b/gcc/c/c-typeck.cc >>>>> @@ -11468,7 +11468,7 @@ c_finish_return (location_t loc, tree retval, tree origtype) >>>>> location_t xloc = expansion_point_location_if_in_system_header (loc); >>>>> >>>>> if (TREE_THIS_VOLATILE (current_function_decl)) >>>>> - warning_at (xloc, 0, >>>>> + warning_at (xloc, OPT_Winvalid_noreturn_explicit, >>>>> "function declared %<noreturn%> has a %<return%> statement"); >>>>> >>>>> if (retval) >>>>> diff --git a/gcc/c/gimple-parser.cc b/gcc/c/gimple-parser.cc >>>>> index d156d83cd37..1acaf75f844 100644 >>>>> --- a/gcc/c/gimple-parser.cc >>>>> +++ b/gcc/c/gimple-parser.cc >>>>> @@ -2593,7 +2593,7 @@ c_finish_gimple_return (location_t loc, tree retval) >>>>> location_t xloc = expansion_point_location_if_in_system_header (loc); >>>>> >>>>> if (TREE_THIS_VOLATILE (current_function_decl)) >>>>> - warning_at (xloc, 0, >>>>> + warning_at (xloc, OPT_Winvalid_noreturn_explicit, >>>>> "function declared %<noreturn%> has a %<return%> statement"); >>>>> >>>>> if (! retval) >>>>> diff --git a/gcc/config/mingw/mingw32.h b/gcc/config/mingw/mingw32.h >>>>> index fa6e307476c..a69926133b1 100644 >>>>> --- a/gcc/config/mingw/mingw32.h >>>>> +++ b/gcc/config/mingw/mingw32.h >>>>> @@ -35,9 +35,9 @@ along with GCC; see the file COPYING3. If not see >>>>> | MASK_MS_BITFIELD_LAYOUT) >>>>> >>>>> #ifdef TARGET_USING_MCFGTHREAD >>>>> -#define DEFINE_THREAD_MODEL builtin_define ("__USING_MCFGTHREAD__"); >>>>> +#define DEFINE_THREAD_MODEL builtin_define ("__USING_MCFGTHREAD__") >>>>> #elif defined(TARGET_USE_PTHREAD_BY_DEFAULT) >>>>> -#define DEFINE_THREAD_MODEL builtin_define ("__USING_POSIXTHREAD__"); >>>>> +#define DEFINE_THREAD_MODEL builtin_define ("__USING_POSIXTHREAD__") >>>>> #else >>>>> #define DEFINE_THREAD_MODEL >>>>> #endif >>>>> @@ -60,7 +60,7 @@ along with GCC; see the file COPYING3. If not see >>>>> builtin_define_std ("WIN64"); \ >>>>> builtin_define ("_WIN64"); \ >>>>> } \ >>>>> - DEFINE_THREAD_MODEL \ >>>>> + DEFINE_THREAD_MODEL; \ >>>>> } \ >>>>> while (0) >>>>> >>>>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc >>>>> index 97bc211ff67..53e56bd4959 100644 >>>>> --- a/gcc/cp/coroutines.cc >>>>> +++ b/gcc/cp/coroutines.cc >>>>> @@ -1375,7 +1375,7 @@ finish_co_return_stmt (location_t kw, tree expr) >>>>> >>>>> /* Makes no sense for a co-routine really. */ >>>>> if (TREE_THIS_VOLATILE (current_function_decl)) >>>>> - warning_at (kw, 0, >>>>> + warning_at (kw, OPT_Winvalid_noreturn_explicit, >>>>> "function declared %<noreturn%> has a" >>>>> " %<co_return%> statement"); >>>>> >>>>> diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc >>>>> index 1b7a31d32f3..74cc59bfb87 100644 >>>>> --- a/gcc/cp/typeck.cc >>>>> +++ b/gcc/cp/typeck.cc >>>>> @@ -11062,7 +11062,7 @@ check_return_expr (tree retval, bool *no_warning, bool *dangling) >>>>> (This is a G++ extension, used to get better code for functions >>>>> that call the `volatile' function.) */ >>>>> if (TREE_THIS_VOLATILE (current_function_decl)) >>>>> - warning (0, "function declared %<noreturn%> has a %<return%> statement"); >>>>> + warning (OPT_Winvalid_noreturn_explicit, "function declared %<noreturn%> has a %<return%> statement"); >>>>> >>>>> /* Check for various simple errors. */ >>>>> if (DECL_DESTRUCTOR_P (current_function_decl)) >>>>> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi >>>>> index 2cba380718b..27d880fd4f0 100644 >>>>> --- a/gcc/doc/invoke.texi >>>>> +++ b/gcc/doc/invoke.texi >>>>> @@ -376,6 +376,7 @@ Objective-C and Objective-C++ Dialects}. >>>>> -Winfinite-recursion >>>>> -Winit-self -Winline -Wno-int-conversion -Wint-in-bool-context >>>>> -Wno-int-to-pointer-cast -Wno-invalid-memory-model >>>>> +-Winvalid-noreturn=explicit -Winvalid-noreturn=implicit >>>>> -Winvalid-pch -Winvalid-utf8 -Wno-unicode -Wjump-misses-init >>>>> -Wlarger-than=@var{byte-size} -Wlogical-not-parentheses -Wlogical-op >>>>> -Wlong-long -Wno-lto-type-mismatch -Wmain -Wmaybe-uninitialized >>>>> @@ -10482,6 +10483,18 @@ an error. @option{Wint-to-pointer-cast} is enabled by default. >>>>> Suppress warnings from casts from a pointer to an integer type of a >>>>> different size. >>>>> >>>>> +@opindex Winvalid-noreturn=explicit >>>>> +@opindex Wno-invalid-noreturn=explicit >>>>> +@item -Winvalid-noreturn=explicit >>>>> +Warn if a function marked noreturn returns explicitly, that is, it >>>>> +contains a return statement. >>>>> + >>>>> +@opindex Winvalid-noreturn=implicit >>>>> +@opindex Wno-invalid-noreturn=implicit >>>>> +@item -Winvalid-noreturn=implicit >>>>> +Warn if a function marked noreturn returns implicitly, that is, it >>>>> +has a path in its control flow that can reach the end of its body. >>>>> + >>>>> @opindex Winvalid-pch >>>>> @opindex Wno-invalid-pch >>>>> @item -Winvalid-pch >>>>> diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc >>>>> index 7fb7b92966b..cfe91038dd1 100644 >>>>> --- a/gcc/tree-cfg.cc >>>>> +++ b/gcc/tree-cfg.cc >>>>> @@ -9817,7 +9817,7 @@ pass_warn_function_return::execute (function *fun) >>>>> } >>>>> if (location == UNKNOWN_LOCATION) >>>>> location = cfun->function_end_locus; >>>>> - warning_at (location, 0, "%<noreturn%> function does return"); >>>>> + warning_at (location, OPT_Winvalid_noreturn_implicit, "%<noreturn%> function does return"); >>>>> } >>>>> >>>>> /* If we see "return;" in some basic block, then we do reach the end >>>> >>> >> >
Hi Jason, Sorry for the long period of radio silence, but I'm finally done with all my university coursework. The main issue I see here is how to process the Winvalid-noreturn= entry. Can I define it in c.opt and then process it in c_common_handle_option and then store whether the -W or -Wno form was used in a bool somewhere? If I can do that somehow, and then access the bool at the callsites where it is required, I can figure out the rest on my own best regards, Julian On Tue, Jun 11, 2024 at 10:26 AM Jason Merrill <jason@redhat.com> wrote: > > On 6/10/24 03:13, Julian Waters wrote: > > Hi Jason, > > > > Thanks for the reply. I'm a little bit overwhelmed with university at > > the moment, would it be ok if I delay implementing this a little bit? > > Sure, we're still early in GCC 15 development, no time pressure. > > > On Tue, Jun 4, 2024 at 1:04 AM Jason Merrill <jason@redhat.com> wrote: > >> > >> On 6/1/24 11:31, Julian Waters wrote: > >>> Hi Jason, > >>> > >>> Thanks for the reply! I'll address your comments soon. I have a > >>> question, if there is an option defined in c.opt as an Enum, like > >>> fstrong-eval-order, and the -no variant of the option is passed, would > >>> the Var somehow reflect the negated option? Eg > >>> > >>> Winvalid-noreturn= > >>> C ObjC C++ ObjC++ Var(warn_invalid_noreturn) Joined > >>> Enum(invalid_noreturn) Warning > >>> > >>> Enum > >>> Name(invalid_noreturn) Type(int) > >>> > >>> EnumValue > >>> Enum(invalid_noreturn) String(explicit) Value(0) > >> > >> -fstrong-eval-order has > >> > >> fstrong-eval-order > >> C++ ObjC++ Common Alias(fstrong-eval-order=, all, none) > >> > >> to represent that plain -fstrong-eval-order is equivalent to > >> -fstrong-eval-order=all, and -fno-strong-eval-order is equivalent to =none. > >> > >>> Would warn_invalid_noreturn then != 0 if > >>> -Wno-invalid-noreturn=explicit is passed? Or is there a way to make a > >>> warning call depend on 2 different OPT_ entries? > >> > >> Typically = options will specify RejectNegative so the driver will > >> reject e.g. -Wno-invalid-noreturn=explicit. > >> > >> Jason > >> > >>> best regards, > >>> Julian > >>> > >>> On Sat, Jun 1, 2024 at 4:57 AM Jason Merrill <jason@redhat.com> wrote: > >>>> > >>>> On 5/29/24 09:58, Julian Waters wrote: > >>>>> Currently, gcc warns about noreturn marked functions that return both explicitly and implicitly, with no way to turn this warning off. clang does have an option for these classes of warnings, -Winvalid-noreturn. However, we can do better. Instead of just having 1 option that switches the warnings for both on and off, we can define an extra layer of granularity, and have a separate options for implicit returns and explicit returns, as in -Winvalid-return=explicit and -Winvalid-noreturn=implicit. This patch adds both to gcc, for compatibility with clang. > >>>> > >>>> Thanks! > >>>> > >>>>> Do note that I am relatively new to gcc's codebase, and as such couldn't figure out how to cleanly define a general -Winvalid-noreturn warning that switch both on and off, for better compatibility with clang. If someone should point out how to do so, I'll happily rewrite my patch. > >>>> > >>>> See -fstrong-eval-order for an example of an option that can be used > >>>> with or without =arg. > >>>> > >>>>> I also do not have write access to gcc, and will need help pushing this patch once the green light is given > >>>> > >>>> Good to know, I can take care of that. > >>>> > >>>>> best regards, > >>>>> Julian > >>>>> > >>>>> gcc/c-family/ChangeLog: > >>>>> > >>>>> * c.opt: Introduce -Winvalid-noreturn=explicit and -Winvalid-noreturn=implicit > >>>>> > >>>>> gcc/ChangeLog: > >>>>> > >>>>> * tree-cfg.cc (pass_warn_function_return::execute): Use it > >>>>> > >>>>> gcc/c/ChangeLog: > >>>>> > >>>>> * c-typeck.cc (c_finish_return): Use it > >>>>> * gimple-parser.cc (c_finish_gimple_return): Use it > >>>>> > >>>>> gcc/config/mingw/ChangeLog: > >>>>> > >>>>> * mingw32.h (EXTRA_OS_CPP_BUILTINS): Fix semicolons > >>>>> > >>>>> gcc/cp/ChangeLog: > >>>>> > >>>>> * coroutines.cc (finish_co_return_stmt): Use it > >>>>> * typeck.cc (check_return_expr): Use it > >>>>> > >>>>> gcc/doc/ChangeLog: > >>>>> > >>>>> * invoke.texi: Document new options > >>>>> > >>>>> From 4daf884f8bbc1e318ba93121a6fdf4139da80b64 Mon Sep 17 00:00:00 2001 > >>>>> From: TheShermanTanker <tanksherman27@gmail.com> > >>>>> Date: Wed, 29 May 2024 21:32:08 +0800 > >>>>> Subject: [PATCH] Introduce the -Winvalid-noreturn flag from clang with extra > >>>>> tuneability > >>>> > >>>> The rationale and ChangeLog entries should be part of the commit message > >>>> (and so the git format-patch output). > >>>> > >>>>> > >>>>> Signed-off-by: TheShermanTanker <tanksherman27@gmail.com> > >>>> > >>>> A DCO sign-off can't use a pseudonym, sorry; please either sign off > >>>> using your real name or file a copyright assignment for the pseudonym > >>>> with the FSF. > >>>> > >>>> See https://gcc.gnu.org/contribute.html#legal for more detail. > >>>> > >>>>> --- > >>>>> gcc/c-family/c.opt | 8 ++++++++ > >>>>> gcc/c/c-typeck.cc | 2 +- > >>>>> gcc/c/gimple-parser.cc | 2 +- > >>>>> gcc/config/mingw/mingw32.h | 6 +++--- > >>>>> gcc/cp/coroutines.cc | 2 +- > >>>>> gcc/cp/typeck.cc | 2 +- > >>>>> gcc/doc/invoke.texi | 13 +++++++++++++ > >>>>> gcc/tree-cfg.cc | 2 +- > >>>>> 8 files changed, 29 insertions(+), 8 deletions(-) > >>>>> > >>>>> diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt > >>>>> index fb34c3b7031..32a2859fdcc 100644 > >>>>> --- a/gcc/c-family/c.opt > >>>>> +++ b/gcc/c-family/c.opt > >>>>> @@ -886,6 +886,14 @@ Winvalid-constexpr > >>>>> C++ ObjC++ Var(warn_invalid_constexpr) Init(-1) Warning > >>>>> Warn when a function never produces a constant expression. > >>>>> > >>>>> +Winvalid-noreturn=explicit > >>>>> +C ObjC C++ ObjC++ Warning > >>>>> +Warn when a function marked noreturn returns explicitly. > >>>>> + > >>>>> +Winvalid-noreturn=implicit > >>>>> +C ObjC C++ ObjC++ Warning > >>>>> +Warn when a function marked noreturn returns implicitly. > >>>>> + > >>>>> Winvalid-offsetof > >>>>> C++ ObjC++ Var(warn_invalid_offsetof) Init(1) Warning > >>>>> Warn about invalid uses of the \"offsetof\" macro. > >>>>> diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc > >>>>> index ad4c7add562..1941fbc44cb 100644 > >>>>> --- a/gcc/c/c-typeck.cc > >>>>> +++ b/gcc/c/c-typeck.cc > >>>>> @@ -11468,7 +11468,7 @@ c_finish_return (location_t loc, tree retval, tree origtype) > >>>>> location_t xloc = expansion_point_location_if_in_system_header (loc); > >>>>> > >>>>> if (TREE_THIS_VOLATILE (current_function_decl)) > >>>>> - warning_at (xloc, 0, > >>>>> + warning_at (xloc, OPT_Winvalid_noreturn_explicit, > >>>>> "function declared %<noreturn%> has a %<return%> statement"); > >>>>> > >>>>> if (retval) > >>>>> diff --git a/gcc/c/gimple-parser.cc b/gcc/c/gimple-parser.cc > >>>>> index d156d83cd37..1acaf75f844 100644 > >>>>> --- a/gcc/c/gimple-parser.cc > >>>>> +++ b/gcc/c/gimple-parser.cc > >>>>> @@ -2593,7 +2593,7 @@ c_finish_gimple_return (location_t loc, tree retval) > >>>>> location_t xloc = expansion_point_location_if_in_system_header (loc); > >>>>> > >>>>> if (TREE_THIS_VOLATILE (current_function_decl)) > >>>>> - warning_at (xloc, 0, > >>>>> + warning_at (xloc, OPT_Winvalid_noreturn_explicit, > >>>>> "function declared %<noreturn%> has a %<return%> statement"); > >>>>> > >>>>> if (! retval) > >>>>> diff --git a/gcc/config/mingw/mingw32.h b/gcc/config/mingw/mingw32.h > >>>>> index fa6e307476c..a69926133b1 100644 > >>>>> --- a/gcc/config/mingw/mingw32.h > >>>>> +++ b/gcc/config/mingw/mingw32.h > >>>>> @@ -35,9 +35,9 @@ along with GCC; see the file COPYING3. If not see > >>>>> | MASK_MS_BITFIELD_LAYOUT) > >>>>> > >>>>> #ifdef TARGET_USING_MCFGTHREAD > >>>>> -#define DEFINE_THREAD_MODEL builtin_define ("__USING_MCFGTHREAD__"); > >>>>> +#define DEFINE_THREAD_MODEL builtin_define ("__USING_MCFGTHREAD__") > >>>>> #elif defined(TARGET_USE_PTHREAD_BY_DEFAULT) > >>>>> -#define DEFINE_THREAD_MODEL builtin_define ("__USING_POSIXTHREAD__"); > >>>>> +#define DEFINE_THREAD_MODEL builtin_define ("__USING_POSIXTHREAD__") > >>>>> #else > >>>>> #define DEFINE_THREAD_MODEL > >>>>> #endif > >>>>> @@ -60,7 +60,7 @@ along with GCC; see the file COPYING3. If not see > >>>>> builtin_define_std ("WIN64"); \ > >>>>> builtin_define ("_WIN64"); \ > >>>>> } \ > >>>>> - DEFINE_THREAD_MODEL \ > >>>>> + DEFINE_THREAD_MODEL; \ > >>>>> } \ > >>>>> while (0) > >>>>> > >>>>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc > >>>>> index 97bc211ff67..53e56bd4959 100644 > >>>>> --- a/gcc/cp/coroutines.cc > >>>>> +++ b/gcc/cp/coroutines.cc > >>>>> @@ -1375,7 +1375,7 @@ finish_co_return_stmt (location_t kw, tree expr) > >>>>> > >>>>> /* Makes no sense for a co-routine really. */ > >>>>> if (TREE_THIS_VOLATILE (current_function_decl)) > >>>>> - warning_at (kw, 0, > >>>>> + warning_at (kw, OPT_Winvalid_noreturn_explicit, > >>>>> "function declared %<noreturn%> has a" > >>>>> " %<co_return%> statement"); > >>>>> > >>>>> diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc > >>>>> index 1b7a31d32f3..74cc59bfb87 100644 > >>>>> --- a/gcc/cp/typeck.cc > >>>>> +++ b/gcc/cp/typeck.cc > >>>>> @@ -11062,7 +11062,7 @@ check_return_expr (tree retval, bool *no_warning, bool *dangling) > >>>>> (This is a G++ extension, used to get better code for functions > >>>>> that call the `volatile' function.) */ > >>>>> if (TREE_THIS_VOLATILE (current_function_decl)) > >>>>> - warning (0, "function declared %<noreturn%> has a %<return%> statement"); > >>>>> + warning (OPT_Winvalid_noreturn_explicit, "function declared %<noreturn%> has a %<return%> statement"); > >>>>> > >>>>> /* Check for various simple errors. */ > >>>>> if (DECL_DESTRUCTOR_P (current_function_decl)) > >>>>> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > >>>>> index 2cba380718b..27d880fd4f0 100644 > >>>>> --- a/gcc/doc/invoke.texi > >>>>> +++ b/gcc/doc/invoke.texi > >>>>> @@ -376,6 +376,7 @@ Objective-C and Objective-C++ Dialects}. > >>>>> -Winfinite-recursion > >>>>> -Winit-self -Winline -Wno-int-conversion -Wint-in-bool-context > >>>>> -Wno-int-to-pointer-cast -Wno-invalid-memory-model > >>>>> +-Winvalid-noreturn=explicit -Winvalid-noreturn=implicit > >>>>> -Winvalid-pch -Winvalid-utf8 -Wno-unicode -Wjump-misses-init > >>>>> -Wlarger-than=@var{byte-size} -Wlogical-not-parentheses -Wlogical-op > >>>>> -Wlong-long -Wno-lto-type-mismatch -Wmain -Wmaybe-uninitialized > >>>>> @@ -10482,6 +10483,18 @@ an error. @option{Wint-to-pointer-cast} is enabled by default. > >>>>> Suppress warnings from casts from a pointer to an integer type of a > >>>>> different size. > >>>>> > >>>>> +@opindex Winvalid-noreturn=explicit > >>>>> +@opindex Wno-invalid-noreturn=explicit > >>>>> +@item -Winvalid-noreturn=explicit > >>>>> +Warn if a function marked noreturn returns explicitly, that is, it > >>>>> +contains a return statement. > >>>>> + > >>>>> +@opindex Winvalid-noreturn=implicit > >>>>> +@opindex Wno-invalid-noreturn=implicit > >>>>> +@item -Winvalid-noreturn=implicit > >>>>> +Warn if a function marked noreturn returns implicitly, that is, it > >>>>> +has a path in its control flow that can reach the end of its body. > >>>>> + > >>>>> @opindex Winvalid-pch > >>>>> @opindex Wno-invalid-pch > >>>>> @item -Winvalid-pch > >>>>> diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc > >>>>> index 7fb7b92966b..cfe91038dd1 100644 > >>>>> --- a/gcc/tree-cfg.cc > >>>>> +++ b/gcc/tree-cfg.cc > >>>>> @@ -9817,7 +9817,7 @@ pass_warn_function_return::execute (function *fun) > >>>>> } > >>>>> if (location == UNKNOWN_LOCATION) > >>>>> location = cfun->function_end_locus; > >>>>> - warning_at (location, 0, "%<noreturn%> function does return"); > >>>>> + warning_at (location, OPT_Winvalid_noreturn_implicit, "%<noreturn%> function does return"); > >>>>> } > >>>>> > >>>>> /* If we see "return;" in some basic block, then we do reach the end > >>>> > >>> > >> > > >
On 7/9/24 6:46 AM, Julian Waters wrote: > Hi Jason, > > Sorry for the long period of radio silence, but I'm finally done with > all my university coursework. The main issue I see here is how to > process the Winvalid-noreturn= entry. Can I define it in c.opt and > then process it in c_common_handle_option and then store whether the > -W or -Wno form was used in a bool somewhere? If I can do that > somehow, and then access the bool at the callsites where it is > required, I can figure out the rest on my own You should be able to handle setting the variable entirely in c.opt, following the pattern of -fstrong-eval-order. The variable is defined by the Var(...) notation in c.opt, and the opts code handles setting it. > On Tue, Jun 11, 2024 at 10:26 AM Jason Merrill <jason@redhat.com> wrote: >> >> On 6/10/24 03:13, Julian Waters wrote: >>> Hi Jason, >>> >>> Thanks for the reply. I'm a little bit overwhelmed with university at >>> the moment, would it be ok if I delay implementing this a little bit? >> >> Sure, we're still early in GCC 15 development, no time pressure. >> >>> On Tue, Jun 4, 2024 at 1:04 AM Jason Merrill <jason@redhat.com> wrote: >>>> >>>> On 6/1/24 11:31, Julian Waters wrote: >>>>> Hi Jason, >>>>> >>>>> Thanks for the reply! I'll address your comments soon. I have a >>>>> question, if there is an option defined in c.opt as an Enum, like >>>>> fstrong-eval-order, and the -no variant of the option is passed, would >>>>> the Var somehow reflect the negated option? Eg >>>>> >>>>> Winvalid-noreturn= >>>>> C ObjC C++ ObjC++ Var(warn_invalid_noreturn) Joined >>>>> Enum(invalid_noreturn) Warning >>>>> >>>>> Enum >>>>> Name(invalid_noreturn) Type(int) >>>>> >>>>> EnumValue >>>>> Enum(invalid_noreturn) String(explicit) Value(0) >>>> >>>> -fstrong-eval-order has >>>> >>>> fstrong-eval-order >>>> C++ ObjC++ Common Alias(fstrong-eval-order=, all, none) >>>> >>>> to represent that plain -fstrong-eval-order is equivalent to >>>> -fstrong-eval-order=all, and -fno-strong-eval-order is equivalent to =none. >>>> >>>>> Would warn_invalid_noreturn then != 0 if >>>>> -Wno-invalid-noreturn=explicit is passed? Or is there a way to make a >>>>> warning call depend on 2 different OPT_ entries? >>>> >>>> Typically = options will specify RejectNegative so the driver will >>>> reject e.g. -Wno-invalid-noreturn=explicit. >>>> >>>> Jason >>>> >>>>> best regards, >>>>> Julian >>>>> >>>>> On Sat, Jun 1, 2024 at 4:57 AM Jason Merrill <jason@redhat.com> wrote: >>>>>> >>>>>> On 5/29/24 09:58, Julian Waters wrote: >>>>>>> Currently, gcc warns about noreturn marked functions that return both explicitly and implicitly, with no way to turn this warning off. clang does have an option for these classes of warnings, -Winvalid-noreturn. However, we can do better. Instead of just having 1 option that switches the warnings for both on and off, we can define an extra layer of granularity, and have a separate options for implicit returns and explicit returns, as in -Winvalid-return=explicit and -Winvalid-noreturn=implicit. This patch adds both to gcc, for compatibility with clang. >>>>>> >>>>>> Thanks! >>>>>> >>>>>>> Do note that I am relatively new to gcc's codebase, and as such couldn't figure out how to cleanly define a general -Winvalid-noreturn warning that switch both on and off, for better compatibility with clang. If someone should point out how to do so, I'll happily rewrite my patch. >>>>>> >>>>>> See -fstrong-eval-order for an example of an option that can be used >>>>>> with or without =arg. >>>>>> >>>>>>> I also do not have write access to gcc, and will need help pushing this patch once the green light is given >>>>>> >>>>>> Good to know, I can take care of that. >>>>>> >>>>>>> best regards, >>>>>>> Julian >>>>>>> >>>>>>> gcc/c-family/ChangeLog: >>>>>>> >>>>>>> * c.opt: Introduce -Winvalid-noreturn=explicit and -Winvalid-noreturn=implicit >>>>>>> >>>>>>> gcc/ChangeLog: >>>>>>> >>>>>>> * tree-cfg.cc (pass_warn_function_return::execute): Use it >>>>>>> >>>>>>> gcc/c/ChangeLog: >>>>>>> >>>>>>> * c-typeck.cc (c_finish_return): Use it >>>>>>> * gimple-parser.cc (c_finish_gimple_return): Use it >>>>>>> >>>>>>> gcc/config/mingw/ChangeLog: >>>>>>> >>>>>>> * mingw32.h (EXTRA_OS_CPP_BUILTINS): Fix semicolons >>>>>>> >>>>>>> gcc/cp/ChangeLog: >>>>>>> >>>>>>> * coroutines.cc (finish_co_return_stmt): Use it >>>>>>> * typeck.cc (check_return_expr): Use it >>>>>>> >>>>>>> gcc/doc/ChangeLog: >>>>>>> >>>>>>> * invoke.texi: Document new options >>>>>>> >>>>>>> From 4daf884f8bbc1e318ba93121a6fdf4139da80b64 Mon Sep 17 00:00:00 2001 >>>>>>> From: TheShermanTanker <tanksherman27@gmail.com> >>>>>>> Date: Wed, 29 May 2024 21:32:08 +0800 >>>>>>> Subject: [PATCH] Introduce the -Winvalid-noreturn flag from clang with extra >>>>>>> tuneability >>>>>> >>>>>> The rationale and ChangeLog entries should be part of the commit message >>>>>> (and so the git format-patch output). >>>>>> >>>>>>> >>>>>>> Signed-off-by: TheShermanTanker <tanksherman27@gmail.com> >>>>>> >>>>>> A DCO sign-off can't use a pseudonym, sorry; please either sign off >>>>>> using your real name or file a copyright assignment for the pseudonym >>>>>> with the FSF. >>>>>> >>>>>> See https://gcc.gnu.org/contribute.html#legal for more detail. >>>>>> >>>>>>> --- >>>>>>> gcc/c-family/c.opt | 8 ++++++++ >>>>>>> gcc/c/c-typeck.cc | 2 +- >>>>>>> gcc/c/gimple-parser.cc | 2 +- >>>>>>> gcc/config/mingw/mingw32.h | 6 +++--- >>>>>>> gcc/cp/coroutines.cc | 2 +- >>>>>>> gcc/cp/typeck.cc | 2 +- >>>>>>> gcc/doc/invoke.texi | 13 +++++++++++++ >>>>>>> gcc/tree-cfg.cc | 2 +- >>>>>>> 8 files changed, 29 insertions(+), 8 deletions(-) >>>>>>> >>>>>>> diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt >>>>>>> index fb34c3b7031..32a2859fdcc 100644 >>>>>>> --- a/gcc/c-family/c.opt >>>>>>> +++ b/gcc/c-family/c.opt >>>>>>> @@ -886,6 +886,14 @@ Winvalid-constexpr >>>>>>> C++ ObjC++ Var(warn_invalid_constexpr) Init(-1) Warning >>>>>>> Warn when a function never produces a constant expression. >>>>>>> >>>>>>> +Winvalid-noreturn=explicit >>>>>>> +C ObjC C++ ObjC++ Warning >>>>>>> +Warn when a function marked noreturn returns explicitly. >>>>>>> + >>>>>>> +Winvalid-noreturn=implicit >>>>>>> +C ObjC C++ ObjC++ Warning >>>>>>> +Warn when a function marked noreturn returns implicitly. >>>>>>> + >>>>>>> Winvalid-offsetof >>>>>>> C++ ObjC++ Var(warn_invalid_offsetof) Init(1) Warning >>>>>>> Warn about invalid uses of the \"offsetof\" macro. >>>>>>> diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc >>>>>>> index ad4c7add562..1941fbc44cb 100644 >>>>>>> --- a/gcc/c/c-typeck.cc >>>>>>> +++ b/gcc/c/c-typeck.cc >>>>>>> @@ -11468,7 +11468,7 @@ c_finish_return (location_t loc, tree retval, tree origtype) >>>>>>> location_t xloc = expansion_point_location_if_in_system_header (loc); >>>>>>> >>>>>>> if (TREE_THIS_VOLATILE (current_function_decl)) >>>>>>> - warning_at (xloc, 0, >>>>>>> + warning_at (xloc, OPT_Winvalid_noreturn_explicit, >>>>>>> "function declared %<noreturn%> has a %<return%> statement"); >>>>>>> >>>>>>> if (retval) >>>>>>> diff --git a/gcc/c/gimple-parser.cc b/gcc/c/gimple-parser.cc >>>>>>> index d156d83cd37..1acaf75f844 100644 >>>>>>> --- a/gcc/c/gimple-parser.cc >>>>>>> +++ b/gcc/c/gimple-parser.cc >>>>>>> @@ -2593,7 +2593,7 @@ c_finish_gimple_return (location_t loc, tree retval) >>>>>>> location_t xloc = expansion_point_location_if_in_system_header (loc); >>>>>>> >>>>>>> if (TREE_THIS_VOLATILE (current_function_decl)) >>>>>>> - warning_at (xloc, 0, >>>>>>> + warning_at (xloc, OPT_Winvalid_noreturn_explicit, >>>>>>> "function declared %<noreturn%> has a %<return%> statement"); >>>>>>> >>>>>>> if (! retval) >>>>>>> diff --git a/gcc/config/mingw/mingw32.h b/gcc/config/mingw/mingw32.h >>>>>>> index fa6e307476c..a69926133b1 100644 >>>>>>> --- a/gcc/config/mingw/mingw32.h >>>>>>> +++ b/gcc/config/mingw/mingw32.h >>>>>>> @@ -35,9 +35,9 @@ along with GCC; see the file COPYING3. If not see >>>>>>> | MASK_MS_BITFIELD_LAYOUT) >>>>>>> >>>>>>> #ifdef TARGET_USING_MCFGTHREAD >>>>>>> -#define DEFINE_THREAD_MODEL builtin_define ("__USING_MCFGTHREAD__"); >>>>>>> +#define DEFINE_THREAD_MODEL builtin_define ("__USING_MCFGTHREAD__") >>>>>>> #elif defined(TARGET_USE_PTHREAD_BY_DEFAULT) >>>>>>> -#define DEFINE_THREAD_MODEL builtin_define ("__USING_POSIXTHREAD__"); >>>>>>> +#define DEFINE_THREAD_MODEL builtin_define ("__USING_POSIXTHREAD__") >>>>>>> #else >>>>>>> #define DEFINE_THREAD_MODEL >>>>>>> #endif >>>>>>> @@ -60,7 +60,7 @@ along with GCC; see the file COPYING3. If not see >>>>>>> builtin_define_std ("WIN64"); \ >>>>>>> builtin_define ("_WIN64"); \ >>>>>>> } \ >>>>>>> - DEFINE_THREAD_MODEL \ >>>>>>> + DEFINE_THREAD_MODEL; \ >>>>>>> } \ >>>>>>> while (0) >>>>>>> >>>>>>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc >>>>>>> index 97bc211ff67..53e56bd4959 100644 >>>>>>> --- a/gcc/cp/coroutines.cc >>>>>>> +++ b/gcc/cp/coroutines.cc >>>>>>> @@ -1375,7 +1375,7 @@ finish_co_return_stmt (location_t kw, tree expr) >>>>>>> >>>>>>> /* Makes no sense for a co-routine really. */ >>>>>>> if (TREE_THIS_VOLATILE (current_function_decl)) >>>>>>> - warning_at (kw, 0, >>>>>>> + warning_at (kw, OPT_Winvalid_noreturn_explicit, >>>>>>> "function declared %<noreturn%> has a" >>>>>>> " %<co_return%> statement"); >>>>>>> >>>>>>> diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc >>>>>>> index 1b7a31d32f3..74cc59bfb87 100644 >>>>>>> --- a/gcc/cp/typeck.cc >>>>>>> +++ b/gcc/cp/typeck.cc >>>>>>> @@ -11062,7 +11062,7 @@ check_return_expr (tree retval, bool *no_warning, bool *dangling) >>>>>>> (This is a G++ extension, used to get better code for functions >>>>>>> that call the `volatile' function.) */ >>>>>>> if (TREE_THIS_VOLATILE (current_function_decl)) >>>>>>> - warning (0, "function declared %<noreturn%> has a %<return%> statement"); >>>>>>> + warning (OPT_Winvalid_noreturn_explicit, "function declared %<noreturn%> has a %<return%> statement"); >>>>>>> >>>>>>> /* Check for various simple errors. */ >>>>>>> if (DECL_DESTRUCTOR_P (current_function_decl)) >>>>>>> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi >>>>>>> index 2cba380718b..27d880fd4f0 100644 >>>>>>> --- a/gcc/doc/invoke.texi >>>>>>> +++ b/gcc/doc/invoke.texi >>>>>>> @@ -376,6 +376,7 @@ Objective-C and Objective-C++ Dialects}. >>>>>>> -Winfinite-recursion >>>>>>> -Winit-self -Winline -Wno-int-conversion -Wint-in-bool-context >>>>>>> -Wno-int-to-pointer-cast -Wno-invalid-memory-model >>>>>>> +-Winvalid-noreturn=explicit -Winvalid-noreturn=implicit >>>>>>> -Winvalid-pch -Winvalid-utf8 -Wno-unicode -Wjump-misses-init >>>>>>> -Wlarger-than=@var{byte-size} -Wlogical-not-parentheses -Wlogical-op >>>>>>> -Wlong-long -Wno-lto-type-mismatch -Wmain -Wmaybe-uninitialized >>>>>>> @@ -10482,6 +10483,18 @@ an error. @option{Wint-to-pointer-cast} is enabled by default. >>>>>>> Suppress warnings from casts from a pointer to an integer type of a >>>>>>> different size. >>>>>>> >>>>>>> +@opindex Winvalid-noreturn=explicit >>>>>>> +@opindex Wno-invalid-noreturn=explicit >>>>>>> +@item -Winvalid-noreturn=explicit >>>>>>> +Warn if a function marked noreturn returns explicitly, that is, it >>>>>>> +contains a return statement. >>>>>>> + >>>>>>> +@opindex Winvalid-noreturn=implicit >>>>>>> +@opindex Wno-invalid-noreturn=implicit >>>>>>> +@item -Winvalid-noreturn=implicit >>>>>>> +Warn if a function marked noreturn returns implicitly, that is, it >>>>>>> +has a path in its control flow that can reach the end of its body. >>>>>>> + >>>>>>> @opindex Winvalid-pch >>>>>>> @opindex Wno-invalid-pch >>>>>>> @item -Winvalid-pch >>>>>>> diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc >>>>>>> index 7fb7b92966b..cfe91038dd1 100644 >>>>>>> --- a/gcc/tree-cfg.cc >>>>>>> +++ b/gcc/tree-cfg.cc >>>>>>> @@ -9817,7 +9817,7 @@ pass_warn_function_return::execute (function *fun) >>>>>>> } >>>>>>> if (location == UNKNOWN_LOCATION) >>>>>>> location = cfun->function_end_locus; >>>>>>> - warning_at (location, 0, "%<noreturn%> function does return"); >>>>>>> + warning_at (location, OPT_Winvalid_noreturn_implicit, "%<noreturn%> function does return"); >>>>>>> } >>>>>>> >>>>>>> /* If we see "return;" in some basic block, then we do reach the end >>>>>> >>>>> >>>> >>> >> >
Darn, not even sending it from my friend's mail account worked to combat the patch formatting issues. Not only is the patch a complete wreck on the mailing list archive, but my friend's mail account has now somehow merged into mine on the list, resulting in a truly confusing author display name. I really hope you've somehow gotten the patch intact on your end... best regards, Julian On Fri, Jul 19, 2024 at 11:47 AM Tay Joseph <tanksherman27@gmail.com> wrote: > Hi Jason, > > > > I’ve managed to address your review comments, and rewrite the patch to > include both -Winvalid-noreturn and -Winvalid-noreturn= but have trouble > figuring out how to format invoke.texi and where to add the documentation > for the new warning options. I’ve also made this a Common option, since > it’s come to my attention that the warning is not specific to c-family, but > is also used by other languages too (See tree-cfg.cc). Here’s the current > version of the patch, hope it’s good to go this time > > > > best regards, > > Julian > > > > From fe1b4d5747e05101410b6bb6db9430362e3977d9 Mon Sep 17 00:00:00 2001 > > From: Julian Waters <tanksherman27@gmail.com> > > Date: Fri, 19 Jul 2024 11:22:38 +0800 > > Subject: [PATCH] Introduce the -Winvalid-noreturn flag from clang with > extra > > tuneability > > > > Currently, gcc warns about noreturn marked functions that return both > explicitly and implicitly, with no way to turn this warning off. clang does > have an option for these classes of warnings, -Winvalid-noreturn. However, > we can do better. Instead of just having 1 option that switches the > warnings for both on and off, we can define an extra layer of granularity, > and have a separate options for implicit returns and explicit returns, as > in -Winvalid-return=explicit and -Winvalid-noreturn=implicit, on top of the > regular -Winvalid-noreturn. This patch adds both to gcc, for compatibility > with clang. > > > > gcc/ChangeLog: > > > > * common.opt: Introduce -Winvalid-noreturn and -Winvalid-noreturn= > > * opts.cc (common_handle_option): Handle new option > > * flag-types.h: New flags for -Winvalid-noreturn= > > * tree-cfg.cc (pass_warn_function_return::execute): Use it > > > > gcc/c/ChangeLog: > > > > * c-typeck.cc (c_finish_return): Use it > > * gimple-parser.cc (c_finish_gimple_return): Use it > > > > gcc/config/mingw/ChangeLog: > > > > * mingw32.h (EXTRA_OS_CPP_BUILTINS): Fix semicolons > > > > gcc/cp/ChangeLog: > > > > * coroutines.cc (finish_co_return_stmt): Use it > > * typeck.cc (check_return_expr): Use it > > > > Signed-off-by: Julian Waters <tanksherman27@gmail.com> > > --- > > gcc/c/c-typeck.cc | 4 ++-- > > gcc/c/gimple-parser.cc | 4 ++-- > > gcc/common.opt | 13 +++++++++++++ > > gcc/config/mingw/mingw32.h | 6 +++--- > > gcc/cp/coroutines.cc | 4 ++-- > > gcc/cp/typeck.cc | 4 ++-- > > gcc/flag-types.h | 7 +++++++ > > gcc/opts.cc | 19 +++++++++++++++++++ > > gcc/tree-cfg.cc | 3 ++- > > 9 files changed, 52 insertions(+), 12 deletions(-) > > > > diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc > > index 7e0f01ed22b..0f8c8cfff2e 100644 > > --- a/gcc/c/c-typeck.cc > > +++ b/gcc/c/c-typeck.cc > > @@ -11738,8 +11738,8 @@ c_finish_return (location_t loc, tree retval, tree > origtype) > > in a function returning void. */ > > location_t xloc = expansion_point_location_if_in_system_header (loc); > > - if (TREE_THIS_VOLATILE (current_function_decl)) > > - warning_at (xloc, 0, > > + if (TREE_THIS_VOLATILE (current_function_decl) && flag_invalid_noreturn > != invalid_noreturn_kind::EXPLICIT) > > + warning_at (xloc, OPT_Winvalid_noreturn, > > "function declared %<noreturn%> has a %<return%> > statement"); > > if (retval) > > diff --git a/gcc/c/gimple-parser.cc b/gcc/c/gimple-parser.cc > > index d156d83cd37..8a684fe334a 100644 > > --- a/gcc/c/gimple-parser.cc > > +++ b/gcc/c/gimple-parser.cc > > @@ -2592,8 +2592,8 @@ c_finish_gimple_return (location_t loc, tree retval) > > in a function returning void. */ > > location_t xloc = expansion_point_location_if_in_system_header (loc); > > - if (TREE_THIS_VOLATILE (current_function_decl)) > > - warning_at (xloc, 0, > > + if (TREE_THIS_VOLATILE (current_function_decl) && flag_invalid_noreturn > != invalid_noreturn_kind::EXPLICIT) > > + warning_at (xloc, OPT_Winvalid_noreturn, > > "function declared %<noreturn%> has a %<return%> > statement"); > > if (! retval) > > diff --git a/gcc/common.opt b/gcc/common.opt > > index ea39f87ae71..d44ff0231c3 100644 > > --- a/gcc/common.opt > > +++ b/gcc/common.opt > > @@ -94,6 +94,11 @@ void *flag_instrument_functions_exclude_files > > Variable > > void *flag_ignored_attributes > > +; The noreturn kind to ignore when warning for noreturn code that > > +; does return. > > +Variable > > +invalid_noreturn_kind flag_invalid_noreturn = invalid_noreturn_kind::NONE > > + > > ; Generic structs (e.g. templates not explicitly specialized) > > ; may not have a compilation unit associated with them, and so > > ; may need to be treated differently from ordinary structs. > > @@ -667,6 +672,14 @@ Winvalid-memory-model > > Common Var(warn_invalid_memory_model) Init(1) Warning > > Warn when an atomic memory model parameter is known to be outside the > valid range. > > +Winvalid-noreturn > > +Common Warning > > +Warn when code marked noreturn returns implicitly and/or explicitly. > > + > > +Winvalid-noreturn= > > +Common Joined Warning > > +Warn when code marked noreturn returns in the manner specified. > > + > > Wlarger-than- > > Common RejectNegative Joined Warning Undocumented Alias(Wlarger-than=) > > diff --git a/gcc/config/mingw/mingw32.h b/gcc/config/mingw/mingw32.h > > index 0dfe8e995b6..48eadfa2c2c 100644 > > --- a/gcc/config/mingw/mingw32.h > > +++ b/gcc/config/mingw/mingw32.h > > @@ -35,9 +35,9 @@ along with GCC; see the file COPYING3. If not see > > | MASK_MS_BITFIELD_LAYOUT) > > #ifdef TARGET_USING_MCFGTHREAD > > -#define DEFINE_THREAD_MODEL builtin_define ("__USING_MCFGTHREAD__"); > > +#define DEFINE_THREAD_MODEL builtin_define ("__USING_MCFGTHREAD__") > > #elif defined(TARGET_USE_PTHREAD_BY_DEFAULT) > > -#define DEFINE_THREAD_MODEL builtin_define ("__USING_POSIXTHREAD__"); > > +#define DEFINE_THREAD_MODEL builtin_define ("__USING_POSIXTHREAD__") > > #else > > #define DEFINE_THREAD_MODEL > > #endif > > @@ -60,7 +60,7 @@ along with GCC; see the file COPYING3. If not see > > builtin_define_std > ("WIN64"); \ > > builtin_define ("_WIN64"); > \ > > > } > \ > > - DEFINE_THREAD_MODEL \ > > + > DEFINE_THREAD_MODEL; \ > > > } > \ > > while (0) > > diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc > > index f350fc33e9b..f49ff82b1ee 100644 > > --- a/gcc/cp/coroutines.cc > > +++ b/gcc/cp/coroutines.cc > > @@ -1374,8 +1374,8 @@ finish_co_return_stmt (location_t kw, tree expr) > > } > > /* Makes no sense for a co-routine really. */ > > - if (TREE_THIS_VOLATILE (current_function_decl)) > > - warning_at (kw, 0, > > + if (TREE_THIS_VOLATILE (current_function_decl) && flag_invalid_noreturn > != invalid_noreturn_kind::EXPLICIT) > > + warning_at (kw, OPT_Winvalid_noreturn, > > "function declared %<noreturn%> has a" > > " %<co_return%> statement"); > > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc > > index 8df8b871676..260a9f99b08 100644 > > --- a/gcc/cp/typeck.cc > > +++ b/gcc/cp/typeck.cc > > @@ -11080,8 +11080,8 @@ check_return_expr (tree retval, bool *no_warning, > bool *dangling) > > /* A `volatile' function is one that isn't supposed to return, ever. > > (This is a G++ extension, used to get better code for functions > > that call the `volatile' function.) */ > > - if (TREE_THIS_VOLATILE (current_function_decl)) > > - warning (0, "function declared %<noreturn%> has a %<return%> > statement"); > > + if (TREE_THIS_VOLATILE (current_function_decl) && flag_invalid_noreturn > != invalid_noreturn_kind::EXPLICIT) > > + warning (OPT_Winvalid_noreturn, "function declared %<noreturn%> has a > %<return%> statement"); > > /* Check for various simple errors. */ > > if (DECL_DESTRUCTOR_P (current_function_decl)) > > diff --git a/gcc/flag-types.h b/gcc/flag-types.h > > index 1e497f0bb91..591acd2a7d2 100644 > > --- a/gcc/flag-types.h > > +++ b/gcc/flag-types.h > > @@ -375,6 +375,13 @@ enum incremental_link { > > INCREMENTAL_LINK_LTO > > }; > > +/* Kind of noreturn to ignore when reporting invalid noreturns */ > > +enum class invalid_noreturn_kind { > > + NONE, > > + IMPLICIT, > > + EXPLICIT > > +}; > > + > > /* Different trace modes. */ > > enum sanitize_coverage_code { > > /* Trace PC. */ > > diff --git a/gcc/opts.cc b/gcc/opts.cc > > index be90a632338..19d31c262d7 100644 > > --- a/gcc/opts.cc > > +++ b/gcc/opts.cc > > @@ -2860,6 +2860,25 @@ common_handle_option (struct gcc_options *opts, > > dc->m_fatal_errors = value; > > break; > > + case OPT_Winvalid_noreturn_: > > + if (lang_mask == CL_DRIVER) > > + break; > > + > > + if (value) > > + warning (0, "%<-Winvalid-noreturn=%> is enabled by default, > ignoring"); > > + else if (strcmp (arg, "explicit")) > > + { > > + opts->x_flag_invalid_noreturn = > invalid_noreturn_kind::EXPLICIT; > > + } > > + else if (strcmp (arg, "implicit")) > > + { > > + opts->x_flag_invalid_noreturn = > invalid_noreturn_kind::IMPLICIT; > > + } > > + else > > + error_at (loc, "Unknown option for %<-Winvalid-noreturn=%>"); > > + > > + break; > > + > > case OPT_Wstack_usage_: > > opts->x_flag_stack_usage_info = value != -1; > > break; > > diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc > > index e6fd1294b95..f28dabc2bbf 100644 > > --- a/gcc/tree-cfg.cc > > +++ b/gcc/tree-cfg.cc > > @@ -9820,7 +9820,8 @@ pass_warn_function_return::execute (function *fun) > > } > > if (location == UNKNOWN_LOCATION) > > location = cfun->function_end_locus; > > - warning_at (location, 0, "%<noreturn%> function does return"); > > + if (flag_invalid_noreturn != invalid_noreturn_kind::IMPLICIT) > > + warning_at (location, 0, "%<noreturn%> function does return"); > > } > > /* If we see "return;" in some basic block, then we do reach the end > > -- > > 2.45.1 > > > > > > Sent from Mail <https://go.microsoft.com/fwlink/?LinkId=550986> for > Windows > > > > *From: *Jason Merrill <jason@redhat.com> > *Sent: *Thursday, 18 July 2024 3:27 am > *To: *Julian Waters <tanksherman27@gmail.com>; gcc-patches@gcc.gnu.org > *Subject: *Re: [PATCH] c-family: Introduce the -Winvalid-noreturn flag > from clang with extra tuneability > > > > On 7/9/24 6:46 AM, Julian Waters wrote: > > > Hi Jason, > > > > > > Sorry for the long period of radio silence, but I'm finally done with > > > all my university coursework. The main issue I see here is how to > > > process the Winvalid-noreturn= entry. Can I define it in c.opt and > > > then process it in c_common_handle_option and then store whether the > > > -W or -Wno form was used in a bool somewhere? If I can do that > > > somehow, and then access the bool at the callsites where it is > > > required, I can figure out the rest on my own > > > > You should be able to handle setting the variable entirely in c.opt, > > following the pattern of -fstrong-eval-order. The variable is defined > > by the Var(...) notation in c.opt, and the opts code handles setting it. > > > > > On Tue, Jun 11, 2024 at 10:26 AM Jason Merrill <jason@redhat.com> wrote: > > >> > > >> On 6/10/24 03:13, Julian Waters wrote: > > >>> Hi Jason, > > >>> > > >>> Thanks for the reply. I'm a little bit overwhelmed with university at > > >>> the moment, would it be ok if I delay implementing this a little bit? > > >> > > >> Sure, we're still early in GCC 15 development, no time pressure. > > >> > > >>> On Tue, Jun 4, 2024 at 1:04 AM Jason Merrill <jason@redhat.com> wrote: > > >>>> > > >>>> On 6/1/24 11:31, Julian Waters wrote: > > >>>>> Hi Jason, > > >>>>> > > >>>>> Thanks for the reply! I'll address your comments soon. I have a > > >>>>> question, if there is an option defined in c.opt as an Enum, like > > >>>>> fstrong-eval-order, and the -no variant of the option is passed, > would > > >>>>> the Var somehow reflect the negated option? Eg > > >>>>> > > >>>>> Winvalid-noreturn= > > >>>>> C ObjC C++ ObjC++ Var(warn_invalid_noreturn) Joined > > >>>>> Enum(invalid_noreturn) Warning > > >>>>> > > >>>>> Enum > > >>>>> Name(invalid_noreturn) Type(int) > > >>>>> > > >>>>> EnumValue > > >>>>> Enum(invalid_noreturn) String(explicit) Value(0) > > >>>> > > >>>> -fstrong-eval-order has > > >>>> > > >>>> fstrong-eval-order > > >>>> C++ ObjC++ Common Alias(fstrong-eval-order=, all, none) > > >>>> > > >>>> to represent that plain -fstrong-eval-order is equivalent to > > >>>> -fstrong-eval-order=all, and -fno-strong-eval-order is equivalent to > =none. > > >>>> > > >>>>> Would warn_invalid_noreturn then != 0 if > > >>>>> -Wno-invalid-noreturn=explicit is passed? Or is there a way to make a > > >>>>> warning call depend on 2 different OPT_ entries? > > >>>> > > >>>> Typically = options will specify RejectNegative so the driver will > > >>>> reject e.g. -Wno-invalid-noreturn=explicit. > > >>>> > > >>>> Jason > > >>>> > > >>>>> best regards, > > >>>>> Julian > > >>>>> > > >>>>> On Sat, Jun 1, 2024 at 4:57 AM Jason Merrill <jason@redhat.com> > wrote: > > >>>>>> > > >>>>>> On 5/29/24 09:58, Julian Waters wrote: > > >>>>>>> Currently, gcc warns about noreturn marked functions that return > both explicitly and implicitly, with no way to turn this warning off. clang > does have an option for these classes of warnings, -Winvalid-noreturn. > However, we can do better. Instead of just having 1 option that switches > the warnings for both on and off, we can define an extra layer of > granularity, and have a separate options for implicit returns and explicit > returns, as in -Winvalid-return=explicit and -Winvalid-noreturn=implicit. > This patch adds both to gcc, for compatibility with clang. > > >>>>>> > > >>>>>> Thanks! > > >>>>>> > > >>>>>>> Do note that I am relatively new to gcc's codebase, and as such > couldn't figure out how to cleanly define a general -Winvalid-noreturn > warning that switch both on and off, for better compatibility with clang. > If someone should point out how to do so, I'll happily rewrite my patch. > > >>>>>> > > >>>>>> See -fstrong-eval-order for an example of an option that can be used > > >>>>>> with or without =arg. > > >>>>>> > > >>>>>>> I also do not have write access to gcc, and will need help pushing > this patch once the green light is given > > >>>>>> > > >>>>>> Good to know, I can take care of that. > > >>>>>> > > >>>>>>> best regards, > > >>>>>>> Julian > > >>>>>>> > > >>>>>>> gcc/c-family/ChangeLog: > > >>>>>>> > > >>>>>>> * c.opt: Introduce -Winvalid-noreturn=explicit and > -Winvalid-noreturn=implicit > > >>>>>>> > > >>>>>>> gcc/ChangeLog: > > >>>>>>> > > >>>>>>> * tree-cfg.cc (pass_warn_function_return::execute): Use it > > >>>>>>> > > >>>>>>> gcc/c/ChangeLog: > > >>>>>>> > > >>>>>>> * c-typeck.cc (c_finish_return): Use it > > >>>>>>> * gimple-parser.cc (c_finish_gimple_return): Use it > > >>>>>>> > > >>>>>>> gcc/config/mingw/ChangeLog: > > >>>>>>> > > >>>>>>> * mingw32.h (EXTRA_OS_CPP_BUILTINS): Fix semicolons > > >>>>>>> > > >>>>>>> gcc/cp/ChangeLog: > > >>>>>>> > > >>>>>>> * coroutines.cc (finish_co_return_stmt): Use it > > >>>>>>> * typeck.cc (check_return_expr): Use it > > >>>>>>> > > >>>>>>> gcc/doc/ChangeLog: > > >>>>>>> > > >>>>>>> * invoke.texi: Document new options > > >>>>>>> > > >>>>>>> From 4daf884f8bbc1e318ba93121a6fdf4139da80b64 Mon Sep 17 > 00:00:00 2001 > > >>>>>>> From: TheShermanTanker <tanksherman27@gmail.com> > > >>>>>>> Date: Wed, 29 May 2024 21:32:08 +0800 > > >>>>>>> Subject: [PATCH] Introduce the -Winvalid-noreturn flag from clang > with extra > > >>>>>>> tuneability > > >>>>>> > > >>>>>> The rationale and ChangeLog entries should be part of the commit > message > > >>>>>> (and so the git format-patch output). > > >>>>>> > > >>>>>>> > > >>>>>>> Signed-off-by: TheShermanTanker <tanksherman27@gmail.com> > > >>>>>> > > >>>>>> A DCO sign-off can't use a pseudonym, sorry; please either sign off > > >>>>>> using your real name or file a copyright assignment for the > pseudonym > > >>>>>> with the FSF. > > >>>>>> > > >>>>>> See https://gcc.gnu.org/contribute.html#legal for more detail. > > >>>>>> > > >>>>>>> --- > > >>>>>>> gcc/c-family/c.opt | 8 ++++++++ > > >>>>>>> gcc/c/c-typeck.cc | 2 +- > > >>>>>>> gcc/c/gimple-parser.cc | 2 +- > > >>>>>>> gcc/config/mingw/mingw32.h | 6 +++--- > > >>>>>>> gcc/cp/coroutines.cc | 2 +- > > >>>>>>> gcc/cp/typeck.cc | 2 +- > > >>>>>>> gcc/doc/invoke.texi | 13 +++++++++++++ > > >>>>>>> gcc/tree-cfg.cc | 2 +- > > >>>>>>> 8 files changed, 29 insertions(+), 8 deletions(-) > > >>>>>>> > > >>>>>>> diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt > > >>>>>>> index fb34c3b7031..32a2859fdcc 100644 > > >>>>>>> --- a/gcc/c-family/c.opt > > >>>>>>> +++ b/gcc/c-family/c.opt > > >>>>>>> @@ -886,6 +886,14 @@ Winvalid-constexpr > > >>>>>>> C++ ObjC++ Var(warn_invalid_constexpr) Init(-1) Warning > > >>>>>>> Warn when a function never produces a constant expression. > > >>>>>>> > > >>>>>>> +Winvalid-noreturn=explicit > > >>>>>>> +C ObjC C++ ObjC++ Warning > > >>>>>>> +Warn when a function marked noreturn returns explicitly. > > >>>>>>> + > > >>>>>>> +Winvalid-noreturn=implicit > > >>>>>>> +C ObjC C++ ObjC++ Warning > > >>>>>>> +Warn when a function marked noreturn returns implicitly. > > >>>>>>> + > > >>>>>>> Winvalid-offsetof > > >>>>>>> C++ ObjC++ Var(warn_invalid_offsetof) Init(1) Warning > > >>>>>>> Warn about invalid uses of the \"offsetof\" macro. > > >>>>>>> diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc > > >>>>>>> index ad4c7add562..1941fbc44cb 100644 > > >>>>>>> --- a/gcc/c/c-typeck.cc > > >>>>>>> +++ b/gcc/c/c-typeck.cc > > >>>>>>> @@ -11468,7 +11468,7 @@ c_finish_return (location_t loc, tree > retval, tree origtype) > > >>>>>>> location_t xloc = > expansion_point_location_if_in_system_header (loc); > > >>>>>>> > > >>>>>>> if (TREE_THIS_VOLATILE (current_function_decl)) > > >>>>>>> - warning_at (xloc, 0, > > >>>>>>> + warning_at (xloc, OPT_Winvalid_noreturn_explicit, > > >>>>>>> "function declared %<noreturn%> has a %<return%> > statement"); > > >>>>>>> > > >>>>>>> if (retval) > > >>>>>>> diff --git a/gcc/c/gimple-parser.cc b/gcc/c/gimple-parser.cc > > >>>>>>> index d156d83cd37..1acaf75f844 100644 > > >>>>>>> --- a/gcc/c/gimple-parser.cc > > >>>>>>> +++ b/gcc/c/gimple-parser.cc > > >>>>>>> @@ -2593,7 +2593,7 @@ c_finish_gimple_return (location_t loc, tree > retval) > > >>>>>>> location_t xloc = > expansion_point_location_if_in_system_header (loc); > > >>>>>>> > > >>>>>>> if (TREE_THIS_VOLATILE (current_function_decl)) > > >>>>>>> - warning_at (xloc, 0, > > >>>>>>> + warning_at (xloc, OPT_Winvalid_noreturn_explicit, > > >>>>>>> "function declared %<noreturn%> has a %<return%> > statement"); > > >>>>>>> > > >>>>>>> if (! retval) > > >>>>>>> diff --git a/gcc/config/mingw/mingw32.h > b/gcc/config/mingw/mingw32.h > > >>>>>>> index fa6e307476c..a69926133b1 100644 > > >>>>>>> --- a/gcc/config/mingw/mingw32.h > > >>>>>>> +++ b/gcc/config/mingw/mingw32.h > > >>>>>>> @@ -35,9 +35,9 @@ along with GCC; see the file COPYING3. If not > see > > >>>>>>> | MASK_MS_BITFIELD_LAYOUT) > > >>>>>>> > > >>>>>>> #ifdef TARGET_USING_MCFGTHREAD > > >>>>>>> -#define DEFINE_THREAD_MODEL builtin_define > ("__USING_MCFGTHREAD__"); > > >>>>>>> +#define DEFINE_THREAD_MODEL builtin_define > ("__USING_MCFGTHREAD__") > > >>>>>>> #elif defined(TARGET_USE_PTHREAD_BY_DEFAULT) > > >>>>>>> -#define DEFINE_THREAD_MODEL builtin_define > ("__USING_POSIXTHREAD__"); > > >>>>>>> +#define DEFINE_THREAD_MODEL builtin_define > ("__USING_POSIXTHREAD__") > > >>>>>>> #else > > >>>>>>> #define DEFINE_THREAD_MODEL > > >>>>>>> #endif > > >>>>>>> @@ -60,7 +60,7 @@ along with GCC; see the file COPYING3. If not > see > > >>>>>>> builtin_define_std ("WIN64"); \ > > >>>>>>> builtin_define ("_WIN64"); \ > > >>>>>>> } \ > > >>>>>>> - DEFINE_THREAD_MODEL \ > > >>>>>>> + DEFINE_THREAD_MODEL; \ > > >>>>>>> > } \ > > >>>>>>> while (0) > > >>>>>>> > > >>>>>>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc > > >>>>>>> index 97bc211ff67..53e56bd4959 100644 > > >>>>>>> --- a/gcc/cp/coroutines.cc > > >>>>>>> +++ b/gcc/cp/coroutines.cc > > >>>>>>> @@ -1375,7 +1375,7 @@ finish_co_return_stmt (location_t kw, tree > expr) > > >>>>>>> > > >>>>>>> /* Makes no sense for a co-routine really. */ > > >>>>>>> if (TREE_THIS_VOLATILE (current_function_decl)) > > >>>>>>> - warning_at (kw, 0, > > >>>>>>> + warning_at (kw, OPT_Winvalid_noreturn_explicit, > > >>>>>>> "function declared %<noreturn%> has a" > > >>>>>>> " %<co_return%> statement"); > > >>>>>>> > > >>>>>>> diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc > > >>>>>>> index 1b7a31d32f3..74cc59bfb87 100644 > > >>>>>>> --- a/gcc/cp/typeck.cc > > >>>>>>> +++ b/gcc/cp/typeck.cc > > >>>>>>> @@ -11062,7 +11062,7 @@ check_return_expr (tree retval, bool > *no_warning, bool *dangling) > > >>>>>>> (This is a G++ extension, used to get better code for > functions > > >>>>>>> that call the `volatile' function.) */ > > >>>>>>> if (TREE_THIS_VOLATILE (current_function_decl)) > > >>>>>>> - warning (0, "function declared %<noreturn%> has a %<return%> > statement"); > > >>>>>>> + warning (OPT_Winvalid_noreturn_explicit, "function declared > %<noreturn%> has a %<return%> statement"); > > >>>>>>> > > >>>>>>> /* Check for various simple errors. */ > > >>>>>>> if (DECL_DESTRUCTOR_P (current_function_decl)) > > >>>>>>> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > > >>>>>>> index 2cba380718b..27d880fd4f0 100644 > > >>>>>>> --- a/gcc/doc/invoke.texi > > >>>>>>> +++ b/gcc/doc/invoke.texi > > >>>>>>> @@ -376,6 +376,7 @@ Objective-C and Objective-C++ Dialects}. > > >>>>>>> -Winfinite-recursion > > >>>>>>> -Winit-self -Winline -Wno-int-conversion > -Wint-in-bool-context > > >>>>>>> -Wno-int-to-pointer-cast -Wno-invalid-memory-model > > >>>>>>> +-Winvalid-noreturn=explicit -Winvalid-noreturn=implicit > > >>>>>>> -Winvalid-pch -Winvalid-utf8 -Wno-unicode > -Wjump-misses-init > > >>>>>>> -Wlarger-than=@var{byte-size} -Wlogical-not-parentheses > -Wlogical-op > > >>>>>>> -Wlong-long -Wno-lto-type-mismatch -Wmain > -Wmaybe-uninitialized > > >>>>>>> @@ -10482,6 +10483,18 @@ an error. @option{Wint-to-pointer-cast} > is enabled by default. > > >>>>>>> Suppress warnings from casts from a pointer to an integer > type of a > > >>>>>>> different size. > > >>>>>>> > > >>>>>>> +@opindex Winvalid-noreturn=explicit > > >>>>>>> +@opindex Wno-invalid-noreturn=explicit > > >>>>>>> +@item -Winvalid-noreturn=explicit > > >>>>>>> +Warn if a function marked noreturn returns explicitly, that is, it > > >>>>>>> +contains a return statement. > > >>>>>>> + > > >>>>>>> +@opindex Winvalid-noreturn=implicit > > >>>>>>> +@opindex Wno-invalid-noreturn=implicit > > >>>>>>> +@item -Winvalid-noreturn=implicit > > >>>>>>> +Warn if a function marked noreturn returns implicitly, that is, it > > >>>>>>> +has a path in its control flow that can reach the end of its body. > > >>>>>>> + > > >>>>>>> @opindex Winvalid-pch > > >>>>>>> @opindex Wno-invalid-pch > > >>>>>>> @item -Winvalid-pch > > >>>>>>> diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc > > >>>>>>> index 7fb7b92966b..cfe91038dd1 100644 > > >>>>>>> --- a/gcc/tree-cfg.cc > > >>>>>>> +++ b/gcc/tree-cfg.cc > > >>>>>>> @@ -9817,7 +9817,7 @@ pass_warn_function_return::execute (function > *fun) > > >>>>>>> } > > >>>>>>> if (location == UNKNOWN_LOCATION) > > >>>>>>> location = cfun->function_end_locus; > > >>>>>>> - warning_at (location, 0, "%<noreturn%> function does > return"); > > >>>>>>> + warning_at (location, OPT_Winvalid_noreturn_implicit, > "%<noreturn%> function does return"); > > >>>>>>> } > > >>>>>>> > > >>>>>>> /* If we see "return;" in some basic block, then we do > reach the end > > >>>>>> > > >>>>> > > >>>> > > >>> > > >> > > > > > > > >
Julian Waters <tanksherman27@gmail.com> writes: > Darn, not even sending it from my friend's mail account worked to combat the patch formatting issues. Not only is the > patch a complete wreck on the mailing list archive, but my friend's mail account has now somehow merged into mine on the > list, resulting in a truly confusing author display name. I really hope you've somehow gotten the patch intact on your > end... Just use `git send-email` and nothing else, and make sure to use plain-text emails when writing anything in your mail client (not HTML). > > best regards, > Julian > [...]
Attempting to resend with a different client, plain text enabled... Hi Jason, I’ve managed to address your review comments, and rewrite the patch to include both -Winvalid-noreturn and -Winvalid-noreturn= but have trouble figuring out how to format invoke.texi and where to add the documentation for the new warning options. I’ve also made this a Common option, since it’s come to my attention that the warning is not specific to c-family, but is also used by other languages too (See tree-cfg.cc). Here’s the current version of the patch, hope it’s good to go this time best regards, Julian From fe1b4d5747e05101410b6bb6db9430362e3977d9 Mon Sep 17 00:00:00 2001 From: Julian Waters <tanksherman27@gmail.com> Date: Fri, 19 Jul 2024 11:22:38 +0800 Subject: [PATCH] Introduce the -Winvalid-noreturn flag from clang with extra tuneability Currently, gcc warns about noreturn marked functions that return both explicitly and implicitly, with no way to turn this warning off. clang does have an option for these classes of warnings, -Winvalid-noreturn. However, we can do better. Instead of just having 1 option that switches the warnings for both on and off, we can define an extra layer of granularity, and have a separate options for implicit returns and explicit returns, as in -Winvalid-return=explicit and -Winvalid-noreturn=implicit, on top of the regular -Winvalid-noreturn. This patch adds both to gcc, for compatibility with clang. gcc/ChangeLog: * common.opt: Introduce -Winvalid-noreturn and -Winvalid-noreturn= * opts.cc (common_handle_option): Handle new option * flag-types.h: New flags for -Winvalid-noreturn= * tree-cfg.cc (pass_warn_function_return::execute): Use it gcc/c/ChangeLog: * c-typeck.cc (c_finish_return): Use it * gimple-parser.cc (c_finish_gimple_return): Use it gcc/config/mingw/ChangeLog: * mingw32.h (EXTRA_OS_CPP_BUILTINS): Fix semicolons gcc/cp/ChangeLog: * coroutines.cc (finish_co_return_stmt): Use it * typeck.cc (check_return_expr): Use it Signed-off-by: Julian Waters <tanksherman27@gmail.com> --- gcc/c/c-typeck.cc | 4 ++-- gcc/c/gimple-parser.cc | 4 ++-- gcc/common.opt | 13 +++++++++++++ gcc/config/mingw/mingw32.h | 6 +++--- gcc/cp/coroutines.cc | 4 ++-- gcc/cp/typeck.cc | 4 ++-- gcc/flag-types.h | 7 +++++++ gcc/opts.cc | 19 +++++++++++++++++++ gcc/tree-cfg.cc | 3 ++- 9 files changed, 52 insertions(+), 12 deletions(-) diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc index 7e0f01ed22b..0f8c8cfff2e 100644 --- a/gcc/c/c-typeck.cc +++ b/gcc/c/c-typeck.cc @@ -11738,8 +11738,8 @@ c_finish_return (location_t loc, tree retval, tree origtype) in a function returning void. */ location_t xloc = expansion_point_location_if_in_system_header (loc); - if (TREE_THIS_VOLATILE (current_function_decl)) - warning_at (xloc, 0, + if (TREE_THIS_VOLATILE (current_function_decl) && flag_invalid_noreturn != invalid_noreturn_kind::EXPLICIT) + warning_at (xloc, OPT_Winvalid_noreturn, "function declared %<noreturn%> has a %<return%> statement"); if (retval) diff --git a/gcc/c/gimple-parser.cc b/gcc/c/gimple-parser.cc index d156d83cd37..8a684fe334a 100644 --- a/gcc/c/gimple-parser.cc +++ b/gcc/c/gimple-parser.cc @@ -2592,8 +2592,8 @@ c_finish_gimple_return (location_t loc, tree retval) in a function returning void. */ location_t xloc = expansion_point_location_if_in_system_header (loc); - if (TREE_THIS_VOLATILE (current_function_decl)) - warning_at (xloc, 0, + if (TREE_THIS_VOLATILE (current_function_decl) && flag_invalid_noreturn != invalid_noreturn_kind::EXPLICIT) + warning_at (xloc, OPT_Winvalid_noreturn, "function declared %<noreturn%> has a %<return%> statement"); if (! retval) diff --git a/gcc/common.opt b/gcc/common.opt index ea39f87ae71..d44ff0231c3 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -94,6 +94,11 @@ void *flag_instrument_functions_exclude_files Variable void *flag_ignored_attributes +; The noreturn kind to ignore when warning for noreturn code that +; does return. +Variable +invalid_noreturn_kind flag_invalid_noreturn = invalid_noreturn_kind::NONE + ; Generic structs (e.g. templates not explicitly specialized) ; may not have a compilation unit associated with them, and so ; may need to be treated differently from ordinary structs. @@ -667,6 +672,14 @@ Winvalid-memory-model Common Var(warn_invalid_memory_model) Init(1) Warning Warn when an atomic memory model parameter is known to be outside the valid range. +Winvalid-noreturn +Common Warning +Warn when code marked noreturn returns implicitly and/or explicitly. + +Winvalid-noreturn= +Common Joined Warning +Warn when code marked noreturn returns in the manner specified. + Wlarger-than- Common RejectNegative Joined Warning Undocumented Alias(Wlarger-than=) diff --git a/gcc/config/mingw/mingw32.h b/gcc/config/mingw/mingw32.h index 0dfe8e995b6..48eadfa2c2c 100644 --- a/gcc/config/mingw/mingw32.h +++ b/gcc/config/mingw/mingw32.h @@ -35,9 +35,9 @@ along with GCC; see the file COPYING3. If not see | MASK_MS_BITFIELD_LAYOUT) #ifdef TARGET_USING_MCFGTHREAD -#define DEFINE_THREAD_MODEL builtin_define ("__USING_MCFGTHREAD__"); +#define DEFINE_THREAD_MODEL builtin_define ("__USING_MCFGTHREAD__") #elif defined(TARGET_USE_PTHREAD_BY_DEFAULT) -#define DEFINE_THREAD_MODEL builtin_define ("__USING_POSIXTHREAD__"); +#define DEFINE_THREAD_MODEL builtin_define ("__USING_POSIXTHREAD__") #else #define DEFINE_THREAD_MODEL #endif @@ -60,7 +60,7 @@ along with GCC; see the file COPYING3. If not see builtin_define_std ("WIN64"); \ builtin_define ("_WIN64"); \ } \ - DEFINE_THREAD_MODEL \ + DEFINE_THREAD_MODEL; \ } \ while (0) diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc index f350fc33e9b..f49ff82b1ee 100644 --- a/gcc/cp/coroutines.cc +++ b/gcc/cp/coroutines.cc @@ -1374,8 +1374,8 @@ finish_co_return_stmt (location_t kw, tree expr) } /* Makes no sense for a co-routine really. */ - if (TREE_THIS_VOLATILE (current_function_decl)) - warning_at (kw, 0, + if (TREE_THIS_VOLATILE (current_function_decl) && flag_invalid_noreturn != invalid_noreturn_kind::EXPLICIT) + warning_at (kw, OPT_Winvalid_noreturn, "function declared %<noreturn%> has a" " %<co_return%> statement"); diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc index 8df8b871676..260a9f99b08 100644 --- a/gcc/cp/typeck.cc +++ b/gcc/cp/typeck.cc @@ -11080,8 +11080,8 @@ check_return_expr (tree retval, bool *no_warning, bool *dangling) /* A `volatile' function is one that isn't supposed to return, ever. (This is a G++ extension, used to get better code for functions that call the `volatile' function.) */ - if (TREE_THIS_VOLATILE (current_function_decl)) - warning (0, "function declared %<noreturn%> has a %<return%> statement"); + if (TREE_THIS_VOLATILE (current_function_decl) && flag_invalid_noreturn != invalid_noreturn_kind::EXPLICIT) + warning (OPT_Winvalid_noreturn, "function declared %<noreturn%> has a %<return%> statement"); /* Check for various simple errors. */ if (DECL_DESTRUCTOR_P (current_function_decl)) diff --git a/gcc/flag-types.h b/gcc/flag-types.h index 1e497f0bb91..591acd2a7d2 100644 --- a/gcc/flag-types.h +++ b/gcc/flag-types.h @@ -375,6 +375,13 @@ enum incremental_link { INCREMENTAL_LINK_LTO }; +/* Kind of noreturn to ignore when reporting invalid noreturns */ +enum class invalid_noreturn_kind { + NONE, + IMPLICIT, + EXPLICIT +}; + /* Different trace modes. */ enum sanitize_coverage_code { /* Trace PC. */ diff --git a/gcc/opts.cc b/gcc/opts.cc index be90a632338..19d31c262d7 100644 --- a/gcc/opts.cc +++ b/gcc/opts.cc @@ -2860,6 +2860,25 @@ common_handle_option (struct gcc_options *opts, dc->m_fatal_errors = value; break; + case OPT_Winvalid_noreturn_: + if (lang_mask == CL_DRIVER) + break; + + if (value) + warning (0, "%<-Winvalid-noreturn=%> is enabled by default, ignoring"); + else if (strcmp (arg, "explicit")) + { + opts->x_flag_invalid_noreturn = invalid_noreturn_kind::EXPLICIT; + } + else if (strcmp (arg, "implicit")) + { + opts->x_flag_invalid_noreturn = invalid_noreturn_kind::IMPLICIT; + } + else + error_at (loc, "Unknown option for %<-Winvalid-noreturn=%>"); + + break; + case OPT_Wstack_usage_: opts->x_flag_stack_usage_info = value != -1; break; diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc index e6fd1294b95..f28dabc2bbf 100644 --- a/gcc/tree-cfg.cc +++ b/gcc/tree-cfg.cc @@ -9820,7 +9820,8 @@ pass_warn_function_return::execute (function *fun) } if (location == UNKNOWN_LOCATION) location = cfun->function_end_locus; - warning_at (location, 0, "%<noreturn%> function does return"); + if (flag_invalid_noreturn != invalid_noreturn_kind::IMPLICIT) + warning_at (location, 0, "%<noreturn%> function does return"); } /* If we see "return;" in some basic block, then we do reach the end
On 7/19/24 2:29 AM, Julian Waters wrote: > Attempting to resend with a different client, plain text enabled... Still corrupted by word wrap and quoted-printable encoding, unfortunately. Probably the easiest solution is to send the patch as an attachment rather than pasted into the message. > I’ve managed to address your review comments, and rewrite the patch to > include both -Winvalid-noreturn and -Winvalid-noreturn= Hmm, I keep suggesting that you follow the pattern of -fstrong-eval-order, and this patch moves away from that pattern without any discussion. Could you say something about why you decided not to? > but have trouble > figuring out how to format invoke.texi and where to add the > documentation for the new warning options. For documentation, I don't see any proposed change, so I'm not sure where to start on formatting advice. I would expect the documentation to go in alphabetical order under @node Warning Options. > I’ve also made this a Common > option, since it’s come to my attention that the warning is not specific > to c-family, but is also used by other languages too (See tree-cfg.cc). Sounds good. > gcc/ChangeLog: > > * common.opt: Introduce -Winvalid-noreturn and -Winvalid-noreturn= > * opts.cc (common_handle_option): Handle new option > * flag-types.h: New flags for -Winvalid-noreturn= > * tree-cfg.cc (pass_warn_function_return::execute): Use it Conventionally, change descriptions should end with a period. > gcc/config/mingw/ChangeLog: > > * mingw32.h (EXTRA_OS_CPP_BUILTINS): Fix semicolons This change should not be part of this patch. It also seems unnecessary to me, but you can propose it separately if you want. > @@ -667,6 +672,14 @@ Winvalid-memory-model > Common Var(warn_invalid_memory_model) Init(1) Warning > Warn when an atomic memory model parameter is known to be outside the > valid range. > +Winvalid-noreturn > +Common Warning > +Warn when code marked noreturn returns implicitly and/or explicitly. > + > +Winvalid-noreturn= > +Common Joined Warning This should have RejectNegative. > diff --git a/gcc/flag-types.h b/gcc/flag-types.h > index 1e497f0bb91..591acd2a7d2 100644 > --- a/gcc/flag-types.h > +++ b/gcc/flag-types.h > @@ -375,6 +375,13 @@ enum incremental_link { > INCREMENTAL_LINK_LTO > }; > +/* Kind of noreturn to ignore when reporting invalid noreturns */ Missing period. > +enum class invalid_noreturn_kind { > + NONE, > + IMPLICIT, > + EXPLICIT > +}; The inverse sense of this enum is confusing, since it isn't reflected in the name. When I see "invalid_noreturn_kind" I think it expresses which category to warn about. Can we reverse it? > /* Different trace modes. */ > enum sanitize_coverage_code { > /* Trace PC. */ > diff --git a/gcc/opts.cc b/gcc/opts.cc > index be90a632338..19d31c262d7 100644 > --- a/gcc/opts.cc > +++ b/gcc/opts.cc > @@ -2860,6 +2860,25 @@ common_handle_option (struct gcc_options *opts, > dc->m_fatal_errors = value; > break; > + case OPT_Winvalid_noreturn_: > + if (lang_mask == CL_DRIVER) > +break; > + > + if (value) > +warning (0, "%<-Winvalid-noreturn=%> is enabled by default, ignoring"); We don't in general warn about redundant flags in general. They might be intended to override e.g. a previous -Wno-invalid-noreturn. > + else if (strcmp (arg, "explicit")) > +{ > + opts->x_flag_invalid_noreturn = invalid_noreturn_kind::EXPLICIT; If I write -Winvalid-noreturn=explicit, I would expect to get warnings about explicit returns, but as discussed above this seems to mean ignoring explicit returns. > +} > + else if (strcmp (arg, "implicit")) > +{ > + opts->x_flag_invalid_noreturn = invalid_noreturn_kind::IMPLICIT; > +} > + else > +error_at (loc, "Unknown option for %<-Winvalid-noreturn=%>"); Seems like we also need options for warning about all or none. I see no handling for -W{no-,}invalid-noreturn; the if (value) above won't handle it because those options don't hit this case; you would need a separate case OPT_Winvalid_noreturn: or use the Alias property in the common.opt entry. You also need testcases for all the different option forms and their effects. Jason
HI Jason, I was hoping to have -Wno-invalid-noreturn=explicit directly disable explicit noreturn warnings for instance, following the style of the -Wno-attributes=vendor:: option, while the pattern in -fstrong-eval-order seems to be that of using RejectNegative and handling each specified positive case rather than disabling the unwanted case, which would mean one would have to write -Winvalid-noreturn=explicit to avoid implicit noreturns, which at least I found to be less clear. If there was a way to only allow the -Wno form of a warning option, I feel it would be simpler to implement this (A positive form like -Winvalid-noreturn=explicit wouldn't make much sense since gcc already warns for it even without the option, in my opinion), but I don't think there's a RejectPositive variant of RejectNegative, unfortunately. I can change it to be more like -fstrong-eval-order if needed, but I feel that is less clear than just allowing the user to disable the warning variant directly I'm a little unsure about the noreturn warnings being a Common warning now actually, any warnings implemented under the gcc directory and not under c/c-family/cp is a Common warning right? Just need some confirmation to be sure I'll revert the semicolon fix for mingw and propose that separately, but I'll leave further discussion about the other review comments raised for this patch for a later time, since it seems to me that I'll be rewriting the patch again and possibly removing some code that was reviewed, so I'll save it for when the patch is more stable best regards, Julian
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt index fb34c3b7031..32a2859fdcc 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -886,6 +886,14 @@ Winvalid-constexpr C++ ObjC++ Var(warn_invalid_constexpr) Init(-1) Warning Warn when a function never produces a constant expression. +Winvalid-noreturn=explicit +C ObjC C++ ObjC++ Warning +Warn when a function marked noreturn returns explicitly. + +Winvalid-noreturn=implicit +C ObjC C++ ObjC++ Warning +Warn when a function marked noreturn returns implicitly. + Winvalid-offsetof C++ ObjC++ Var(warn_invalid_offsetof) Init(1) Warning Warn about invalid uses of the \"offsetof\" macro. diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc index ad4c7add562..1941fbc44cb 100644 --- a/gcc/c/c-typeck.cc +++ b/gcc/c/c-typeck.cc @@ -11468,7 +11468,7 @@ c_finish_return (location_t loc, tree retval, tree origtype) location_t xloc = expansion_point_location_if_in_system_header (loc); if (TREE_THIS_VOLATILE (current_function_decl)) - warning_at (xloc, 0, + warning_at (xloc, OPT_Winvalid_noreturn_explicit, "function declared %<noreturn%> has a %<return%> statement"); if (retval) diff --git a/gcc/c/gimple-parser.cc b/gcc/c/gimple-parser.cc index d156d83cd37..1acaf75f844 100644 --- a/gcc/c/gimple-parser.cc +++ b/gcc/c/gimple-parser.cc @@ -2593,7 +2593,7 @@ c_finish_gimple_return (location_t loc, tree retval) location_t xloc = expansion_point_location_if_in_system_header (loc); if (TREE_THIS_VOLATILE (current_function_decl)) - warning_at (xloc, 0, + warning_at (xloc, OPT_Winvalid_noreturn_explicit, "function declared %<noreturn%> has a %<return%> statement"); if (! retval) diff --git a/gcc/config/mingw/mingw32.h b/gcc/config/mingw/mingw32.h index fa6e307476c..a69926133b1 100644 --- a/gcc/config/mingw/mingw32.h +++ b/gcc/config/mingw/mingw32.h @@ -35,9 +35,9 @@ along with GCC; see the file COPYING3. If not see | MASK_MS_BITFIELD_LAYOUT) #ifdef TARGET_USING_MCFGTHREAD -#define DEFINE_THREAD_MODEL builtin_define ("__USING_MCFGTHREAD__"); +#define DEFINE_THREAD_MODEL builtin_define ("__USING_MCFGTHREAD__") #elif defined(TARGET_USE_PTHREAD_BY_DEFAULT) -#define DEFINE_THREAD_MODEL builtin_define ("__USING_POSIXTHREAD__"); +#define DEFINE_THREAD_MODEL builtin_define ("__USING_POSIXTHREAD__") #else #define DEFINE_THREAD_MODEL #endif @@ -60,7 +60,7 @@ along with GCC; see the file COPYING3. If not see builtin_define_std ("WIN64"); \ builtin_define ("_WIN64"); \ } \ - DEFINE_THREAD_MODEL \ + DEFINE_THREAD_MODEL; \ } \ while (0) diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc index 97bc211ff67..53e56bd4959 100644 --- a/gcc/cp/coroutines.cc +++ b/gcc/cp/coroutines.cc @@ -1375,7 +1375,7 @@ finish_co_return_stmt (location_t kw, tree expr) /* Makes no sense for a co-routine really. */ if (TREE_THIS_VOLATILE (current_function_decl)) - warning_at (kw, 0, + warning_at (kw, OPT_Winvalid_noreturn_explicit, "function declared %<noreturn%> has a" " %<co_return%> statement"); diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc index 1b7a31d32f3..74cc59bfb87 100644 --- a/gcc/cp/typeck.cc +++ b/gcc/cp/typeck.cc @@ -11062,7 +11062,7 @@ check_return_expr (tree retval, bool *no_warning, bool *dangling) (This is a G++ extension, used to get better code for functions that call the `volatile' function.) */ if (TREE_THIS_VOLATILE (current_function_decl)) - warning (0, "function declared %<noreturn%> has a %<return%> statement"); + warning (OPT_Winvalid_noreturn_explicit, "function declared %<noreturn%> has a %<return%> statement"); /* Check for various simple errors. */ if (DECL_DESTRUCTOR_P (current_function_decl)) diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 2cba380718b..27d880fd4f0 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -376,6 +376,7 @@ Objective-C and Objective-C++ Dialects}. -Winfinite-recursion -Winit-self -Winline -Wno-int-conversion -Wint-in-bool-context -Wno-int-to-pointer-cast -Wno-invalid-memory-model +-Winvalid-noreturn=explicit -Winvalid-noreturn=implicit -Winvalid-pch -Winvalid-utf8 -Wno-unicode -Wjump-misses-init -Wlarger-than=@var{byte-size} -Wlogical-not-parentheses -Wlogical-op -Wlong-long -Wno-lto-type-mismatch -Wmain -Wmaybe-uninitialized @@ -10482,6 +10483,18 @@ an error. @option{Wint-to-pointer-cast} is enabled by default. Suppress warnings from casts from a pointer to an integer type of a different size. +@opindex Winvalid-noreturn=explicit +@opindex Wno-invalid-noreturn=explicit +@item -Winvalid-noreturn=explicit +Warn if a function marked noreturn returns explicitly, that is, it +contains a return statement. + +@opindex Winvalid-noreturn=implicit +@opindex Wno-invalid-noreturn=implicit +@item -Winvalid-noreturn=implicit +Warn if a function marked noreturn returns implicitly, that is, it +has a path in its control flow that can reach the end of its body. + @opindex Winvalid-pch @opindex Wno-invalid-pch @item -Winvalid-pch diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc index 7fb7b92966b..cfe91038dd1 100644 --- a/gcc/tree-cfg.cc +++ b/gcc/tree-cfg.cc @@ -9817,7 +9817,7 @@ pass_warn_function_return::execute (function *fun) } if (location == UNKNOWN_LOCATION) location = cfun->function_end_locus; - warning_at (location, 0, "%<noreturn%> function does return"); + warning_at (location, OPT_Winvalid_noreturn_implicit, "%<noreturn%> function does return"); } /* If we see "return;" in some basic block, then we do reach the end