| Message ID | 811c1f62-3baa-3a66-7e98-f3fa41f38753@gmail.com |
|---|---|
| State | New |
| Headers | show |
This patch is OK.
On 07/07/2017 10:58 AM, Joseph Myers wrote: > This patch is OK. > Thanks. Committed in r250104. Do you have any comments on or concerns with changing how LangEnabledBy interprets the opt argument as I suggested below? IMO, it makes little sense for an option that takes an argument and that specifies a binary option like -Wall in LangEnabledBy to default to the binary value of the latter option. I think it would be more intuitive and convenient for it to default to the value set by its Init directive for the positive form of the binary option and to zero for the negative form (or to empty for strings, if that's ever done). Martin
On Mon, 10 Jul 2017, Martin Sebor wrote: > On 07/07/2017 10:58 AM, Joseph Myers wrote: > > This patch is OK. > > > > Thanks. Committed in r250104. > > Do you have any comments on or concerns with changing how > LangEnabledBy interprets the opt argument as I suggested below? > > IMO, it makes little sense for an option that takes an argument > and that specifies a binary option like -Wall in LangEnabledBy > to default to the binary value of the latter option. I think > it would be more intuitive and convenient for it to default to > the value set by its Init directive for the positive form of > the binary option and to zero for the negative form (or to empty > for strings, if that's ever done). I'm uneasy about the notion of -Wall implying an option that's on-by-default at all. If it's on-by-default, why should -Wall have anything to do with it? (Resetting from 2 to 1 is obviously wrong.) Such an implication only makes sense to me if -Wall implies a *different* (presumably higher) value than the default. And in the case of Init(-1) (for special logic to initialize an option), copying the Init value certainly doesn't make sense in an implication.
On 07/10/2017 05:27 PM, Joseph Myers wrote: > On Mon, 10 Jul 2017, Martin Sebor wrote: > >> On 07/07/2017 10:58 AM, Joseph Myers wrote: >>> This patch is OK. >>> >> >> Thanks. Committed in r250104. >> >> Do you have any comments on or concerns with changing how >> LangEnabledBy interprets the opt argument as I suggested below? >> >> IMO, it makes little sense for an option that takes an argument >> and that specifies a binary option like -Wall in LangEnabledBy >> to default to the binary value of the latter option. I think >> it would be more intuitive and convenient for it to default to >> the value set by its Init directive for the positive form of >> the binary option and to zero for the negative form (or to empty >> for strings, if that's ever done). > > I'm uneasy about the notion of -Wall implying an option that's > on-by-default at all. If it's on-by-default, why should -Wall have > anything to do with it? (Resetting from 2 to 1 is obviously wrong.) > Such an implication only makes sense to me if -Wall implies a *different* > (presumably higher) value than the default. And in the case of Init(-1) > (for special logic to initialize an option), copying the Init value > certainly doesn't make sense in an implication. That sounds like a good point for -Wstringop-verflow. I think there I copied the whole LangEnabledBy attribute from another option without fully considering (or even understanding) its effect. I'm pretty sure copying options like this is uncommon, and what I'd like to do is help avoid similar kinds of mistake in the future. This is one small tweak that can help. Better checking of the attributes for consistency and sanity would be another, provided "boolean to integer conversions" were treated as invalid by the option scripts. The checking could also reject the mistake of redundantly specifying -Wall for an option that's on by default (or worse, having -Wall or -Wextra lower a numeric option's default argument value). Martin
On 07/17/2017 01:17 PM, Martin Sebor wrote: > On 07/10/2017 05:27 PM, Joseph Myers wrote: >> On Mon, 10 Jul 2017, Martin Sebor wrote: >> >>> On 07/07/2017 10:58 AM, Joseph Myers wrote: >>>> This patch is OK. >>>> >>> >>> Thanks. Committed in r250104. >>> >>> Do you have any comments on or concerns with changing how >>> LangEnabledBy interprets the opt argument as I suggested below? >>> >>> IMO, it makes little sense for an option that takes an argument >>> and that specifies a binary option like -Wall in LangEnabledBy >>> to default to the binary value of the latter option. I think >>> it would be more intuitive and convenient for it to default to >>> the value set by its Init directive for the positive form of >>> the binary option and to zero for the negative form (or to empty >>> for strings, if that's ever done). >> >> I'm uneasy about the notion of -Wall implying an option that's >> on-by-default at all. If it's on-by-default, why should -Wall have >> anything to do with it? (Resetting from 2 to 1 is obviously wrong.) >> Such an implication only makes sense to me if -Wall implies a *different* >> (presumably higher) value than the default. And in the case of Init(-1) >> (for special logic to initialize an option), copying the Init value >> certainly doesn't make sense in an implication. > > That sounds like a good point for -Wstringop-verflow. I think > there I copied the whole LangEnabledBy attribute from another > option without fully considering (or even understanding) its > effect. I'm pretty sure copying options like this is uncommon, ...is "not uncommon" is what I meant above. > and what I'd like to do is help avoid similar kinds of mistake > in the future. This is one small tweak that can help. Better > checking of the attributes for consistency and sanity would be > another, provided "boolean to integer conversions" were treated > as invalid by the option scripts. The checking could also reject > the mistake of redundantly specifying -Wall for an option that's > on by default (or worse, having -Wall or -Wextra lower a numeric > option's default argument value). > > Martin
PR other/81345 - -Wall resets -Wstringop-overflow to 1 from the default 2 gcc/c-family/ChangeLog: PR other/81345 * c.opt (-Wstringop-overflow): Set defaults in LangEnabledBy. gcc/testsuite/ChangeLog: PR other/81345 * gcc.dg/pr81345.c: New test. diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt index 05766c4..e0ad3ab 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -732,7 +732,7 @@ Warn about buffer overflow in string manipulation functions like memcpy and strcpy. Wstringop-overflow= -C ObjC C++ ObjC++ Joined RejectNegative UInteger Var(warn_stringop_overflow) Init(2) Warning LangEnabledBy(C ObjC C++ ObjC++, Wall) IntegerRange(0, 4) +C ObjC C++ ObjC++ Joined RejectNegative UInteger Var(warn_stringop_overflow) Init(2) Warning LangEnabledBy(C ObjC C++ ObjC++, Wall, 2, 0) IntegerRange(0, 4) Under the control of Object Size type, warn about buffer overflow in string manipulation functions like memcpy and strcpy. diff --git a/gcc/testsuite/gcc.dg/pr81345.c b/gcc/testsuite/gcc.dg/pr81345.c new file mode 100644 index 0000000..c2cbad7 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr81345.c @@ -0,0 +1,17 @@ +/* PR other/81345 - -Wall resets -Wstringop-overflow to 1 from the default 2 + { dg-do compile } + { dg-options "-O2 -Wall" } */ + +char a[3]; + +void f (const char *s) +{ + __builtin_strncpy (a, s, sizeof a + 1); /* { dg-warning "\\\[-Wstringop-overflow=]" } */ +} + +struct S { char a[3]; int i; }; + +void g (struct S *d, const char *s) +{ + __builtin_strncpy (d->a, s, sizeof d->a + 1); /* { dg-warning "\\\[-Wstringop-overflow=]" } */ +}
The -Wstringop-overflow option defaults to 2 (for Object Size Checking type 1). But when -Wall is used it resets the default value to 1. This happens because when I added the option to c.opt I assumed it would default to, well, the default value set by the Init() directive regardless of whether or not -Wall was used. The attached patch explicitly specifies the defaults to correct this. Btw., I think this behavior is too surprising to be correct or (I hope) even intended for options with arguments. -Wstringop- overflow is specified like this: C ObjC C++ ObjC++ Joined RejectNegative UInteger Var(warn_stringop_overflow) Init(2) Warning LangEnabledBy(C ObjC C++ ObjC++, Wall) IntegerRange(0, 4) with the LangEnabledBy form used above documented like this: LangEnabledBy(language, opt) When compiling for the given language, the option is set to the value of -opt, IMO, it makes little sense for an option that takes an argument and that specifies a binary option like -Wall in LangEnabledBy to default to the binary value of the latter option. I think it would be more intuitive and convenient for it to default to the value set by its Init directive for the positive form of the binary option and to zero for the negative form (or to empty for strings, if that's ever done). Martin