C PATCH for -Wshift-negative-value (PR c/66066)

C PATCH for -Wshift-negative-value (PR c/66066)

[Marek Polacek]

The -Wshift-negative-value patch caused grief since it breaks building
some programs.  The following patch should alleviate the pain a bit: mark
a left shift of a negative value as non-const only if pedantic.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2015-05-11  Marek Polacek  <polacek@redhat.com>

	PR c/66066
	* c-typeck.c (build_binary_op): Mark left shift of a negative value
	as non-const only if pedantic.

	* gcc.dg/c99-left-shift-2.c: New test.

diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c
index 3fcb7c2..05b2709 100644
--- gcc/c/c-typeck.c
+++ gcc/c/c-typeck.c
@@ -10702,7 +10702,7 @@ build_binary_op (location_t location, enum tree_code code,
 	    {
 	      /* Don't reject a left shift of a negative value in a context
 		 where a constant expression is needed in C90.  */
-	      if (flag_isoc99)
+	      if (pedantic && flag_isoc99)
 		int_const = false;
 	      if (c_inhibit_evaluation_warnings == 0)
 		warning_at (location, OPT_Wshift_negative_value,
diff --git gcc/testsuite/gcc.dg/c99-left-shift-2.c gcc/testsuite/gcc.dg/c99-left-shift-2.c
index e69de29..a4cd9d0 100644
--- gcc/testsuite/gcc.dg/c99-left-shift-2.c
+++ gcc/testsuite/gcc.dg/c99-left-shift-2.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-options "-std=iso9899:1999" } */
+
+enum E { A = -2 << 1 };
+int i = -1 << 0;
+
+int
+f (int i)
+{
+  switch (i)
+  case -1 << 0: break;
+}

	Marek

Re: C PATCH for -Wshift-negative-value (PR c/66066)

[Manuel López-Ibáñez]

On 11/05/15 16:21, Marek Polacek wrote:
> The -Wshift-negative-value patch caused grief since it breaks building
> some programs.  The following patch should alleviate the pain a bit: mark
> a left shift of a negative value as non-const only if pedantic.

Either this is not correct according to the guidelines ("the flag pedantic 
should not cause generated code differences or errors", 
https://gcc.gnu.org/wiki/DiagnosticsGuidelines) or the guidelines need updating.

My take is that this should work exactly like OPT_Woverflow (see 
constant_expression_warning):

!flag_isoc99 || !constant_required should produce a warning() with 
OPT_Wshift_negative_value

flag_isoc99 && constant_required should produce a pedwarn() using (pedantic ? 
OPT_Wpedantic : OPT_Wshift_negative_value).

The above means that we only produce errors with -std=c99|gnu99 
-pedantic-errors in the places that are mandated by the standard. Otherwise, 
there are no code or error differences and at most there are warnings that are 
controlled by -Wshift-negative-value for those that want to disable them.

Of course, the problem here is that we are folding this even before we know if 
a constant is actually required by the standard, no? Is there no way to move 
the warning to the point where we actually know whether a constant is required 
or not?

Cheers,

Manuel.

Re: C PATCH for -Wshift-negative-value (PR c/66066)

[Marek Polacek]

On Mon, May 11, 2015 at 05:09:26PM +0200, Manuel López-Ibáñez wrote:
> On 11/05/15 16:21, Marek Polacek wrote:
> >The -Wshift-negative-value patch caused grief since it breaks building
> >some programs.  The following patch should alleviate the pain a bit: mark
> >a left shift of a negative value as non-const only if pedantic.
> 
> Either this is not correct according to the guidelines ("the flag pedantic
> should not cause generated code differences or errors",
> https://gcc.gnu.org/wiki/DiagnosticsGuidelines) or the guidelines need
> updating.
[...]

The problem here isn't in the -Wshift-negative-value warning itself; the
problem is with marking -1 << 0 as a non-constant: later on, we warn in
a context where a constant expression is needed ("initializer element is
not a constant expression"), and for e.g. int foo = -1 << 0 | 9; there's
an error ("initializer element is not constant").

My change means that we wouldn't complain unless -pedantic (to not upset
too many users).  I'm not particularly fond of it, but it seems like the
simplest solution.

	Marek

Re: C PATCH for -Wshift-negative-value (PR c/66066)

[Markus Trippelsdorf]

On 2015.05.11 at 17:54 +0200, Marek Polacek wrote:
> On Mon, May 11, 2015 at 05:09:26PM +0200, Manuel López-Ibáñez wrote:
> > On 11/05/15 16:21, Marek Polacek wrote:
> > >The -Wshift-negative-value patch caused grief since it breaks building
> > >some programs.  The following patch should alleviate the pain a bit: mark
> > >a left shift of a negative value as non-const only if pedantic.
> > 
> > Either this is not correct according to the guidelines ("the flag pedantic
> > should not cause generated code differences or errors",
> > https://gcc.gnu.org/wiki/DiagnosticsGuidelines) or the guidelines need
> > updating.
> [...]
> 
> The problem here isn't in the -Wshift-negative-value warning itself; the
> problem is with marking -1 << 0 as a non-constant: later on, we warn in
> a context where a constant expression is needed ("initializer element is
> not a constant expression"), and for e.g. int foo = -1 << 0 | 9; there's
> an error ("initializer element is not constant").

I find both the warning and the error confusing. Instead of "is not a
constant", it should rather say "invokes undefined behavior".
Because it is not obvious at first sight why (-1 << 0 | 9) shouldn't be a
constant expression.

-- 
Markus

Re: C PATCH for -Wshift-negative-value (PR c/66066)

[Manuel López-Ibáñez]

On 11 May 2015 at 17:54, Marek Polacek <polacek@redhat.com> wrote:
> The problem here isn't in the -Wshift-negative-value warning itself; the
> problem is with marking -1 << 0 as a non-constant: later on, we warn in
> a context where a constant expression is needed ("initializer element is
> not a constant expression"), and for e.g. int foo = -1 << 0 | 9; there's
> an error ("initializer element is not constant").

Yes, I understand. What I'm saying is that if folding was done at the
moment that the constant is requested, this would be a case of
pedwarn-if-pedantic, which is exactly what Joseph is discussing here
for case (b):
https://gcc.gnu.org/ml/gcc/2013-11/msg00253.html

It doesn't matter that the error "initializer element is not a
constant expression" is not given as long as an error (e.g., "left
shift of negative value is undefined according to ISO C99") is given
with -pedantic-errors.

I'm sorry to sound so "pedantic" but we have faced this same issue in
the past (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19976#c7) and
the solution was to delay folding
(https://gcc.gnu.org/ml/gcc-patches/2008-10/msg01061.html).

> My change means that we wouldn't complain unless -pedantic (to not upset
> too many users).  I'm not particularly fond of it, but it seems like the
> simplest solution.

-Wpedantic (which is the same as -pedantic) should not give errors.
Otherwise, it would mean that someone who uses -Wpedantic-
-Wno-pedantic will silence the error, but someone that uses
-Werror=pedantic -Wno-error=pedantic will still see an error (!).
There is also no way to tell that error is related to -Wpedantic.

But if this is the accepted solution, please at least test
global_dc->pedantic_errors instead and put a big /* ???? This is a
hack to reject non-conforming programs with -pedantic-errors and
accept them otherwise. See
https://gcc.gnu.org/ml/gcc-patches/2008-10/msg01061.html and
https://gcc.gnu.org/ml/gcc/2013-11/msg00253.html about to fix this
properly. */

Cheers,

Manuel.

Re: C PATCH for -Wshift-negative-value (PR c/66066)

[Jeff Law]

On 05/11/2015 12:14 PM, Manuel López-Ibáñez wrote:
> On 11 May 2015 at 17:54, Marek Polacek <polacek@redhat.com> wrote:
>
> I'm sorry to sound so "pedantic" but we have faced this same issue in
> the past (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19976#c7) and
> the solution was to delay folding
> (https://gcc.gnu.org/ml/gcc-patches/2008-10/msg01061.html).
And it's still the answer to many issues.  There's an effort to delay 
folding of various things in the C++ front-end and once done, that 
effort may be extended to other things.

Jeff

Re: C PATCH for -Wshift-negative-value (PR c/66066)

[Joseph Myers]

On Mon, 11 May 2015, Marek Polacek wrote:

> The problem here isn't in the -Wshift-negative-value warning itself; the
> problem is with marking -1 << 0 as a non-constant: later on, we warn in
> a context where a constant expression is needed ("initializer element is
> not a constant expression"), and for e.g. int foo = -1 << 0 | 9; there's
> an error ("initializer element is not constant").

For cases that are not integer constant expressions but can be folded to 
constants (e.g. where the unevaluated half of ?: contains a function call, 
which is not allowed in constant expressions), the general approach is a 
pedwarn-if-pedantic, e.g.:

          if (TREE_CODE (value) != INTEGER_CST)
            {
              value = c_fully_fold (value, false, NULL);
              if (TREE_CODE (value) == INTEGER_CST)
                pedwarn (loc, OPT_Wpedantic,
                         "enumerator value for %qE is not an integer "
                         "constant expression", name);
            }
          if (TREE_CODE (value) != INTEGER_CST)
            {
              error ("enumerator value for %qE is not an integer constant",
                     name);
              value = 0;
            }

(and similar for various cases other than enums - it turns out the Linux 
kernel has or had lots of cases that aren't strictly integer constant 
expressions but are used in contexts requiring such).

So I think the issue here is that the above expression isn't folded to a 
constant by c_fully_fold.  If the expression involves division by zero, 
say, clearly it's appropriate not to fold; no meaningful result value can 
be assigned.  But for the above expression, a meaningful result can be 
assigned, so it would seem to make sense to fold.  That is, it would make 
sense for c_fully_fold_internal to fold inside a C_MAYBE_CONST_EXPR with 
C_MAYBE_CONST_EXPR_INT_OPERANDS set, in the cases where meaningful results 
can be assigned.

The rationale for not folding the contents of a C_MAYBE_CONST_EXPR in 
c_fully_fold is that folding already took place when the 
C_MAYBE_CONST_EXPR was created, and it's inefficient to potentially fold 
the same expression recursively many times.  But that's only the case for 
"normal" C_MAYBE_CONST_EXPRs, not those with 
C_MAYBE_CONST_EXPR_INT_OPERANDS set that indicate something that could 
occur an unevaluated part of an integer constant expression.  And in any 
case, if folding had already occurred, there would already be the 
potential for duplicate folding through the existing uses of 
remove_c_maybe_const_expr.

Thus I think this issue should be addressed through folding in 
c_fully_fold_internal in this case, while watching carefully to make sure 
it doesn't reduce any other cases (division by zero, negative or out of 
range shift, overflows) to pedwarns-if-pedantic if they aren't already.

There would then remain the matter of "initializer element is not a 
constant expression" being an unconditional pedwarn_init rather than a 
pedwarn-if-pedantic, so I think you'd still get warnings that couldn't be 
disabled for the above code (which would turn into errors with -Werror).  
The answer for that is probably to make this pedwarn_init use 
OPT_Wpedantic, consistent with the various other cases that handle things 
that aren't constant expressions but folded to constants.

-- 
Joseph S. Myers
joseph@codesourcery.com