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 <ja...@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 <ja...@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 <ja...@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 <tanksherma...@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 <tanksherma...@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 > >>>> > >>> > >> > > >
