Re: [PATCH] prevent -Wall from resetting -Wstringop-overflow=2 to 1 (pr 81345)
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
Re: [PATCH] prevent -Wall from resetting -Wstringop-overflow=2 to 1 (pr 81345)
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
Re: [PATCH] prevent -Wall from resetting -Wstringop-overflow=2 to 1 (pr 81345)
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. -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH] prevent -Wall from resetting -Wstringop-overflow=2 to 1 (pr 81345)
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
Re: [PATCH] prevent -Wall from resetting -Wstringop-overflow=2 to 1 (pr 81345)
This patch is OK. -- Joseph S. Myers jos...@codesourcery.com
[PATCH] prevent -Wall from resetting -Wstringop-overflow=2 to 1 (pr 81345)
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
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 000..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=]" } */
+}
