| Message ID | 50f26c0c-e458-9790-6cd4-7ed9299593a4@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | c/55976 -Werror=return-type should error on returning a value from a void function | expand |
On 04/03/2018 10:26 AM, dave.pagan@oracle.com wrote: > This patch fixes handlng of -Werror=return-type. Currently, even with > the flag specified, return type errors remain warnings. > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55976 > > Function c_finish_return (), when calling pedwarn, should specifiy > OPT_Wreturn_type as the diagnostic index if warn_return_type is set so > that the given diagnostic is generated as an error, not a warning. > > Patch was successfully bootstrapped and tested on x86_64-linux. I would expect the option to control the warning consistently so that when the test case is compiled with just -Wno-return-type (and no other options) the warning is not issued. But that's not what happens because pedwarn() is called with a zero argument as the option. Martin
Hi Martin, Hadn't thought about, but you're right ... it should be consistent. Currently, -Wno-return-type has no effect on this warning, but it should. Since this patch does fix the original problem, what do you recommend? Scrap this patch? Or let it proceed and submit a new bug noting the (existing) incorrect behavior of -Wno-return-type? --Dave On 04/03/2018 12:36 PM, Martin Sebor wrote: > On 04/03/2018 10:26 AM, dave.pagan@oracle.com wrote: >> This patch fixes handlng of -Werror=return-type. Currently, even with >> the flag specified, return type errors remain warnings. >> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55976 >> >> Function c_finish_return (), when calling pedwarn, should specifiy >> OPT_Wreturn_type as the diagnostic index if warn_return_type is set so >> that the given diagnostic is generated as an error, not a warning. >> >> Patch was successfully bootstrapped and tested on x86_64-linux. > > I would expect the option to control the warning consistently so > that when the test case is compiled with just -Wno-return-type > (and no other options) the warning is not issued. But that's not > what happens because pedwarn() is called with a zero argument as > the option. > > Martin
On Tue, Apr 03, 2018 at 01:36:13PM -0600, Martin Sebor wrote: > On 04/03/2018 10:26 AM, dave.pagan@oracle.com wrote: > > This patch fixes handlng of -Werror=return-type. Currently, even with > > the flag specified, return type errors remain warnings. > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55976 > > > > Function c_finish_return (), when calling pedwarn, should specifiy > > OPT_Wreturn_type as the diagnostic index if warn_return_type is set so > > that the given diagnostic is generated as an error, not a warning. > > > > Patch was successfully bootstrapped and tested on x86_64-linux. > > I would expect the option to control the warning consistently so > that when the test case is compiled with just -Wno-return-type > (and no other options) the warning is not issued. But that's not > what happens because pedwarn() is called with a zero argument as > the option. I think we need to make sure we error out even with -Wno-return-type when -pedantic-errors. Especially when issues this pedwarn warns about are very hard to debug show stoppers for anybody calling such functions in GCC 8 (because we turn such spots into __builtin_unreachable () and thus randomly can execute completely unrelated code). So, I think consistency isn't that important here. Jakub
On 04/04/2018 11:15 AM, Jakub Jelinek wrote: > On Tue, Apr 03, 2018 at 01:36:13PM -0600, Martin Sebor wrote: >> On 04/03/2018 10:26 AM, dave.pagan@oracle.com wrote: >>> This patch fixes handlng of -Werror=return-type. Currently, even with >>> the flag specified, return type errors remain warnings. >>> >>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55976 >>> >>> Function c_finish_return (), when calling pedwarn, should specifiy >>> OPT_Wreturn_type as the diagnostic index if warn_return_type is set so >>> that the given diagnostic is generated as an error, not a warning. >>> >>> Patch was successfully bootstrapped and tested on x86_64-linux. >> >> I would expect the option to control the warning consistently so >> that when the test case is compiled with just -Wno-return-type >> (and no other options) the warning is not issued. But that's not >> what happens because pedwarn() is called with a zero argument as >> the option. > > I think we need to make sure we error out even with -Wno-return-type > when -pedantic-errors. That would seem surprising to me. Is there an existing precedent for this in GCC? (Any other warnings or options that are treated this way?) It would also diverge from Clang, which would be particularly unhelpful to users of both compilers. I would suggest to follow what Clang does in terms of controlling the warning (though not necessarily in its severity). It's consistent and intuitive. (Clang has -Werror=return-type by default; that may be overly strict.) Martin > Especially when issues this pedwarn warns about are > very hard to debug show stoppers for anybody calling such functions in GCC 8 > (because we turn such spots into __builtin_unreachable () and thus randomly > can execute completely unrelated code). So, I think consistency isn't that > important here. > > Jakub >
On 04/04/2018 10:58 AM, Martin Sebor wrote: > On 04/04/2018 11:15 AM, Jakub Jelinek wrote: >> On Tue, Apr 03, 2018 at 01:36:13PM -0600, Martin Sebor wrote: >>> On 04/03/2018 10:26 AM, dave.pagan@oracle.com wrote: >>>> This patch fixes handlng of -Werror=return-type. Currently, even with >>>> the flag specified, return type errors remain warnings. >>>> >>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55976 >>>> >>>> Function c_finish_return (), when calling pedwarn, should specifiy >>>> OPT_Wreturn_type as the diagnostic index if warn_return_type is set so >>>> that the given diagnostic is generated as an error, not a warning. >>>> >>>> Patch was successfully bootstrapped and tested on x86_64-linux. >>> >>> I would expect the option to control the warning consistently so >>> that when the test case is compiled with just -Wno-return-type >>> (and no other options) the warning is not issued. But that's not >>> what happens because pedwarn() is called with a zero argument as >>> the option. >> >> I think we need to make sure we error out even with -Wno-return-type >> when -pedantic-errors. > > That would seem surprising to me. Is there an existing > precedent for this in GCC? (Any other warnings or options > that are treated this way?) > > It would also diverge from Clang, which would be particularly > unhelpful to users of both compilers. I would suggest to > follow what Clang does in terms of controlling the warning > (though not necessarily in its severity). It's consistent > and intuitive. (Clang has -Werror=return-type by default; > that may be overly strict.) I think these are both good points. While I tend to lean toward consistency (both within GCC and with clang), if this sort of problem is potentially worse in GCC 8 (as Jakub suggests) then perhaps it's worth thinking about how to help prevent it. If we do choose to go this direction with -pedantic-errors, and there isn't a precedent for it, then the documentation would require an update to reflect the new behavior. Also, thoughts on this question from my last email? > Since this patch does fix the original problem, what do you recommend? > Scrap this patch? Or let it proceed and submit a new bug noting the > (existing) incorrect behavior of -Wno-return-type? We could add the discussion in this email to any new bug we create for -Wno-return-type. --Dave > > Martin > >> Especially when issues this pedwarn warns about are >> very hard to debug show stoppers for anybody calling such functions >> in GCC 8 >> (because we turn such spots into __builtin_unreachable () and thus >> randomly >> can execute completely unrelated code). So, I think consistency >> isn't that >> important here. >> >> Jakub >> >
On 04/04/2018 05:50 PM, dave.pagan@oracle.com wrote: > On 04/04/2018 10:58 AM, Martin Sebor wrote: >> On 04/04/2018 11:15 AM, Jakub Jelinek wrote: >>> On Tue, Apr 03, 2018 at 01:36:13PM -0600, Martin Sebor wrote: >>>> On 04/03/2018 10:26 AM, dave.pagan@oracle.com wrote: >>>>> This patch fixes handlng of -Werror=return-type. Currently, even with >>>>> the flag specified, return type errors remain warnings. >>>>> >>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55976 >>>>> >>>>> Function c_finish_return (), when calling pedwarn, should specifiy >>>>> OPT_Wreturn_type as the diagnostic index if warn_return_type is set so >>>>> that the given diagnostic is generated as an error, not a warning. >>>>> >>>>> Patch was successfully bootstrapped and tested on x86_64-linux. >>>> >>>> I would expect the option to control the warning consistently so >>>> that when the test case is compiled with just -Wno-return-type >>>> (and no other options) the warning is not issued. But that's not >>>> what happens because pedwarn() is called with a zero argument as >>>> the option. >>> >>> I think we need to make sure we error out even with -Wno-return-type >>> when -pedantic-errors. >> >> That would seem surprising to me. Is there an existing >> precedent for this in GCC? (Any other warnings or options >> that are treated this way?) >> >> It would also diverge from Clang, which would be particularly >> unhelpful to users of both compilers. I would suggest to >> follow what Clang does in terms of controlling the warning >> (though not necessarily in its severity). It's consistent >> and intuitive. (Clang has -Werror=return-type by default; >> that may be overly strict.) > > I think these are both good points. While I tend to lean toward > consistency (both within GCC and with clang), if this sort of problem is > potentially worse in GCC 8 (as Jakub suggests) then perhaps it's worth > thinking about how to help prevent it. If we do choose to go this > direction with -pedantic-errors, and there isn't a precedent for it, > then the documentation would require an update to reflect the new behavior. I actually don't think -Wpedantic is the appropriate option to control the warning in the case Jakub is concerned about (it may be appropriate for warning about returning a value from a void function because that's a constraint violation). -Wpedantic is meant for diagnostics that are required by the C/C++ standards but that GCC may otherwise silently accept as extensions. Defining a non-void function that doesn't return a value is valid in C is not incorrect and does not require a diagnostic. (Using the return value is undefined, but when it isn't used there is no problem.) (This is different in recent versions of C++ where a return statement with no operand in a non-void function requires a diagnostic, so the C++ handling may need to be different.) > Also, thoughts on this question from my last email? > >> Since this patch does fix the original problem, what do you recommend? >> Scrap this patch? Or let it proceed and submit a new bug noting the >> (existing) incorrect behavior of -Wno-return-type? > > We could add the discussion in this email to any new bug we create for > -Wno-return-type. I think for C, handling it under the same bug and in the same patch would be best. The C++ bits could come later if needed. Martin > > --Dave >> >> Martin >> >>> Especially when issues this pedwarn warns about are >>> very hard to debug show stoppers for anybody calling such functions >>> in GCC 8 >>> (because we turn such spots into __builtin_unreachable () and thus >>> randomly >>> can execute completely unrelated code). So, I think consistency >>> isn't that >>> important here. >>> >>> Jakub >>> >> >
On 04/04/2018 07:44 PM, Martin Sebor wrote: > On 04/04/2018 05:50 PM, dave.pagan@oracle.com wrote: >> On 04/04/2018 10:58 AM, Martin Sebor wrote: >>> On 04/04/2018 11:15 AM, Jakub Jelinek wrote: >>>> On Tue, Apr 03, 2018 at 01:36:13PM -0600, Martin Sebor wrote: >>>>> On 04/03/2018 10:26 AM, dave.pagan@oracle.com wrote: >>>>>> This patch fixes handlng of -Werror=return-type. Currently, even >>>>>> with >>>>>> the flag specified, return type errors remain warnings. >>>>>> >>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55976 >>>>>> >>>>>> Function c_finish_return (), when calling pedwarn, should specifiy >>>>>> OPT_Wreturn_type as the diagnostic index if warn_return_type is >>>>>> set so >>>>>> that the given diagnostic is generated as an error, not a warning. >>>>>> >>>>>> Patch was successfully bootstrapped and tested on x86_64-linux. >>>>> >>>>> I would expect the option to control the warning consistently so >>>>> that when the test case is compiled with just -Wno-return-type >>>>> (and no other options) the warning is not issued. But that's not >>>>> what happens because pedwarn() is called with a zero argument as >>>>> the option. >>>> >>>> I think we need to make sure we error out even with -Wno-return-type >>>> when -pedantic-errors. >>> >>> That would seem surprising to me. Is there an existing >>> precedent for this in GCC? (Any other warnings or options >>> that are treated this way?) >>> >>> It would also diverge from Clang, which would be particularly >>> unhelpful to users of both compilers. I would suggest to >>> follow what Clang does in terms of controlling the warning >>> (though not necessarily in its severity). It's consistent >>> and intuitive. (Clang has -Werror=return-type by default; >>> that may be overly strict.) >> >> I think these are both good points. While I tend to lean toward >> consistency (both within GCC and with clang), if this sort of problem is >> potentially worse in GCC 8 (as Jakub suggests) then perhaps it's worth >> thinking about how to help prevent it. If we do choose to go this >> direction with -pedantic-errors, and there isn't a precedent for it, >> then the documentation would require an update to reflect the new >> behavior. > > I actually don't think -Wpedantic is the appropriate option > to control the warning in the case Jakub is concerned about > (it may be appropriate for warning about returning a value > from a void function because that's a constraint violation). > -Wpedantic is meant for diagnostics that are required by > the C/C++ standards but that GCC may otherwise silently > accept as extensions. Defining a non-void function that > doesn't return a value is valid in C is not incorrect and > does not require a diagnostic. (Using the return value > is undefined, but when it isn't used there is no problem.) > > (This is different in recent versions of C++ where a return > statement with no operand in a non-void function requires > a diagnostic, so the C++ handling may need to be different.) > >> Also, thoughts on this question from my last email? >> >>> Since this patch does fix the original problem, what do you recommend? >>> Scrap this patch? Or let it proceed and submit a new bug noting the >>> (existing) incorrect behavior of -Wno-return-type? >> >> We could add the discussion in this email to any new bug we create for >> -Wno-return-type. > > I think for C, handling it under the same bug and in the same > patch would be best. The C++ bits could come later if needed. Ok. Thanks, Martin. I'll add proper handling of -Wno-return-type to this patch and resubmit it. --Dave > > Martin > >> >> --Dave >>> >>> Martin >>> >>>> Especially when issues this pedwarn warns about are >>>> very hard to debug show stoppers for anybody calling such functions >>>> in GCC 8 >>>> (because we turn such spots into __builtin_unreachable () and thus >>>> randomly >>>> can execute completely unrelated code). So, I think consistency >>>> isn't that >>>> important here. >>>> >>>> Jakub >>>> >>> >> >
Index: gcc/c/c-typeck.c =================================================================== --- gcc/c/c-typeck.c (revision 259017) +++ gcc/c/c-typeck.c (working copy) @@ -10135,7 +10135,7 @@ c_finish_return (location_t loc, tree retval, tree bool warned_here; if (flag_isoc99) warned_here = pedwarn - (loc, 0, + (loc, warn_return_type ? OPT_Wreturn_type : 0, "%<return%> with no value, in function returning non-void"); else warned_here = warning_at @@ -10153,7 +10153,7 @@ c_finish_return (location_t loc, tree retval, tree bool warned_here; if (TREE_CODE (TREE_TYPE (retval)) != VOID_TYPE) warned_here = pedwarn - (xloc, 0, + (xloc, warn_return_type ? OPT_Wreturn_type : 0, "%<return%> with a value, in function returning void"); else warned_here = pedwarn Index: gcc/testsuite/gcc.dg/noncompile/pr55976.c =================================================================== --- gcc/testsuite/gcc.dg/noncompile/pr55976.c (revision 0) +++ gcc/testsuite/gcc.dg/noncompile/pr55976.c (working copy) @@ -0,0 +1,12 @@ +/* PR c/55976 */ +/* { dg-do compile } */ +/* { dg-options "-Werror=return-type" } */ + +void t () { return 1; } /* { dg-error "return" "function returning void" } */ +int b () { return; } /* { dg-error "return" "function returning non-void" } */ + +int main() +{ + t(); b(); + return 0; +}