FDO usage: -Wcoverage-mismatch should not ignore -Wno-error

FDO usage: -Wcoverage-mismatch should not ignore -Wno-error

[Xinliang David Li]

[-- Attachment #1: Type: text/plain, Size: 1003 bytes --]

-Wcoverage-mismatch is enabled by default, and the warning is promoted
to error by default. However in the current implementation -Wno-error
can not demote the error back to warning. The patch was ported from
one contributed by Neil.

OK for trunk after regression testing?


2011-04-18  Neil Vachharajani  <nvachhar@gmail.com>

    * flags.c:  New flag variable.
    * opts.c (common_handle_options): Set flag_werror_set.
    * opts-global.c (decode_options): Delay Werror decision
    for Wcoverage-mismatch util after options are parsed.

The following test case can be added, but the test harness does not
like the extra warnings -- how can they be pruned?

Thanks,

David

/* { dg-options "-O2 -Wcoverage-mismatch -Wno-error" } */

int __attribute__((noinline)) bar (void)
{
}

#ifdef _PROFILE_USE
int foo (int i)
{
  if (i)
    bar ();
  else
   bar ();
  return 0;
}
#else
int foo (int i)
{
  if (i)
    bar ();
  return 0;
}
#endif

int main(int argc, char **argv)
{
  foo (argc);
  return 0;
}

[-- Attachment #2: wc.p --]
[-- Type: application/octet-stream, Size: 2334 bytes --]

Index: flags.h
===================================================================
--- flags.h	(revision 172693)
+++ flags.h	(working copy)
@@ -112,6 +112,8 @@ extern struct target_flag_state *this_ta
 #define flag_excess_precision \
   (this_target_flag_state->x_flag_excess_precision)
 
+extern int flag_werror_set;
+
 /* Nonzero if we dump in VCG format, not plain text.  */
 extern int dump_for_graph;
 
Index: opts.c
===================================================================
--- opts.c	(revision 172693)
+++ opts.c	(working copy)
@@ -36,6 +36,9 @@ along with GCC; see the file COPYING3.  
 #include "insn-attr.h"		/* For INSN_SCHEDULING and DELAY_SLOTS.  */
 #include "target.h"
 
+
+int flag_werror_set;
+
 /* Parse the -femit-struct-debug-detailed option value
    and set the flag variables. */
 
@@ -1378,6 +1381,10 @@ common_handle_option (struct gcc_options
       /* Currently handled in a prescan.  */
       break;
 
+    case OPT_Werror:
+      flag_werror_set = true;
+      break;
+
     case OPT_Werror_:
       enable_warning_as_error (arg, value, lang_mask, handlers,
 			       opts, opts_set, loc, dc);
Index: opts-global.c
===================================================================
--- opts-global.c	(revision 172693)
+++ opts-global.c	(working copy)
@@ -310,10 +310,7 @@ decode_options (struct gcc_options *opts
 
   set_default_handlers (&handlers);
 
-  /* Enable -Werror=coverage-mismatch by default.  */
-  control_warning_option (OPT_Wcoverage_mismatch, (int) DK_ERROR, true,
-			  loc, lang_mask,
-			  &handlers, opts, opts_set, dc);
+  warn_coverage_mismatch = true;
 
   default_options_optimization (opts, opts_set,
 				decoded_options, decoded_options_count,
@@ -329,6 +326,18 @@ decode_options (struct gcc_options *opts
 			loc, lang_mask,
 			&handlers, dc);
 
+  if (!flag_werror_set)
+    {
+      /* Enable -Werror=coverage-mismatch by default, if it hasn't
+         be set explicitly.  */
+      if (warn_coverage_mismatch
+          && (global_dc->classify_diagnostic[OPT_Wcoverage_mismatch] ==
+          DK_UNSPECIFIED))
+        control_warning_option (OPT_Wcoverage_mismatch, (int) DK_ERROR, true,
+                                loc, lang_mask,
+                                &handlers, opts, opts_set, dc);
+    }
+
   finish_options (opts, opts_set, loc);
 }
 

FDO usage: -Wcoverage-mismatch should not ignore -Wno-error

[Xinliang David Li]

[-- Attachment #1: Type: text/plain, Size: 1003 bytes --]

-Wcoverage-mismatch is enabled by default, and the warning is promoted
to error by default. However in the current implementation -Wno-error
can not demote the error back to warning. The patch was ported from
one contributed by Neil.

OK for trunk after regression testing?


2011-04-18  Neil Vachharajani  <nvachhar@gmail.com>

    * flags.c:  New flag variable.
    * opts.c (common_handle_options): Set flag_werror_set.
    * opts-global.c (decode_options): Delay Werror decision
    for Wcoverage-mismatch util after options are parsed.

The following test case can be added, but the test harness does not
like the extra warnings -- how can they be pruned?

Thanks,

David

/* { dg-options "-O2 -Wcoverage-mismatch -Wno-error" } */

int __attribute__((noinline)) bar (void)
{
}

#ifdef _PROFILE_USE
int foo (int i)
{
  if (i)
    bar ();
  else
   bar ();
  return 0;
}
#else
int foo (int i)
{
  if (i)
    bar ();
  return 0;
}
#endif

int main(int argc, char **argv)
{
  foo (argc);
  return 0;
}

[-- Attachment #2: wc.p --]
[-- Type: application/octet-stream, Size: 2334 bytes --]

Index: flags.h
===================================================================
--- flags.h	(revision 172693)
+++ flags.h	(working copy)
@@ -112,6 +112,8 @@ extern struct target_flag_state *this_ta
 #define flag_excess_precision \
   (this_target_flag_state->x_flag_excess_precision)
 
+extern int flag_werror_set;
+
 /* Nonzero if we dump in VCG format, not plain text.  */
 extern int dump_for_graph;
 
Index: opts.c
===================================================================
--- opts.c	(revision 172693)
+++ opts.c	(working copy)
@@ -36,6 +36,9 @@ along with GCC; see the file COPYING3.  
 #include "insn-attr.h"		/* For INSN_SCHEDULING and DELAY_SLOTS.  */
 #include "target.h"
 
+
+int flag_werror_set;
+
 /* Parse the -femit-struct-debug-detailed option value
    and set the flag variables. */
 
@@ -1378,6 +1381,10 @@ common_handle_option (struct gcc_options
       /* Currently handled in a prescan.  */
       break;
 
+    case OPT_Werror:
+      flag_werror_set = true;
+      break;
+
     case OPT_Werror_:
       enable_warning_as_error (arg, value, lang_mask, handlers,
 			       opts, opts_set, loc, dc);
Index: opts-global.c
===================================================================
--- opts-global.c	(revision 172693)
+++ opts-global.c	(working copy)
@@ -310,10 +310,7 @@ decode_options (struct gcc_options *opts
 
   set_default_handlers (&handlers);
 
-  /* Enable -Werror=coverage-mismatch by default.  */
-  control_warning_option (OPT_Wcoverage_mismatch, (int) DK_ERROR, true,
-			  loc, lang_mask,
-			  &handlers, opts, opts_set, dc);
+  warn_coverage_mismatch = true;
 
   default_options_optimization (opts, opts_set,
 				decoded_options, decoded_options_count,
@@ -329,6 +326,18 @@ decode_options (struct gcc_options *opts
 			loc, lang_mask,
 			&handlers, dc);
 
+  if (!flag_werror_set)
+    {
+      /* Enable -Werror=coverage-mismatch by default, if it hasn't
+         be set explicitly.  */
+      if (warn_coverage_mismatch
+          && (global_dc->classify_diagnostic[OPT_Wcoverage_mismatch] ==
+          DK_UNSPECIFIED))
+        control_warning_option (OPT_Wcoverage_mismatch, (int) DK_ERROR, true,
+                                loc, lang_mask,
+                                &handlers, opts, opts_set, dc);
+    }
+
   finish_options (opts, opts_set, loc);
 }
 

Re: FDO usage: -Wcoverage-mismatch should not ignore -Wno-error

[Richard Guenther]

On Tue, Apr 19, 2011 at 9:13 AM, Xinliang David Li <davidxl@google.com> wrote:
> -Wcoverage-mismatch is enabled by default, and the warning is promoted
> to error by default. However in the current implementation -Wno-error
> can not demote the error back to warning. The patch was ported from
> one contributed by Neil.
>
> OK for trunk after regression testing?

I am sure there is a better way to achieve this, like making
Werror=coverage-mismatch
the default.  Joseph?

Richard.

>
> 2011-04-18  Neil Vachharajani  <nvachhar@gmail.com>
>
>    * flags.c:  New flag variable.
>    * opts.c (common_handle_options): Set flag_werror_set.
>    * opts-global.c (decode_options): Delay Werror decision
>    for Wcoverage-mismatch util after options are parsed.
>
> The following test case can be added, but the test harness does not
> like the extra warnings -- how can they be pruned?
>
> Thanks,
>
> David
>
> /* { dg-options "-O2 -Wcoverage-mismatch -Wno-error" } */
>
> int __attribute__((noinline)) bar (void)
> {
> }
>
> #ifdef _PROFILE_USE
> int foo (int i)
> {
>  if (i)
>    bar ();
>  else
>   bar ();
>  return 0;
> }
> #else
> int foo (int i)
> {
>  if (i)
>    bar ();
>  return 0;
> }
> #endif
>
> int main(int argc, char **argv)
> {
>  foo (argc);
>  return 0;
> }
>

Re: FDO usage: -Wcoverage-mismatch should not ignore -Wno-error

[Xinliang David Li]

This would work if there is a way to set Werror=coverage-mismatch
without having to explicitly set the option classification as
DK_ERROR.   Does this mechanism exist?

Thanks,

David

On Tue, Apr 19, 2011 at 12:52 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Tue, Apr 19, 2011 at 9:13 AM, Xinliang David Li <davidxl@google.com> wrote:
>> -Wcoverage-mismatch is enabled by default, and the warning is promoted
>> to error by default. However in the current implementation -Wno-error
>> can not demote the error back to warning. The patch was ported from
>> one contributed by Neil.
>>
>> OK for trunk after regression testing?
>
> I am sure there is a better way to achieve this, like making
> Werror=coverage-mismatch
> the default.  Joseph?
>
> Richard.
>
>>
>> 2011-04-18  Neil Vachharajani  <nvachhar@gmail.com>
>>
>>    * flags.c:  New flag variable.
>>    * opts.c (common_handle_options): Set flag_werror_set.
>>    * opts-global.c (decode_options): Delay Werror decision
>>    for Wcoverage-mismatch util after options are parsed.
>>
>> The following test case can be added, but the test harness does not
>> like the extra warnings -- how can they be pruned?
>>
>> Thanks,
>>
>> David
>>
>> /* { dg-options "-O2 -Wcoverage-mismatch -Wno-error" } */
>>
>> int __attribute__((noinline)) bar (void)
>> {
>> }
>>
>> #ifdef _PROFILE_USE
>> int foo (int i)
>> {
>>  if (i)
>>    bar ();
>>  else
>>   bar ();
>>  return 0;
>> }
>> #else
>> int foo (int i)
>> {
>>  if (i)
>>    bar ();
>>  return 0;
>> }
>> #endif
>>
>> int main(int argc, char **argv)
>> {
>>  foo (argc);
>>  return 0;
>> }
>>
>

Re: FDO usage: -Wcoverage-mismatch should not ignore -Wno-error

[Xinliang David Li]

Ping ..

David

On Wed, Apr 20, 2011 at 9:34 AM, Xinliang David Li <davidxl@google.com> wrote:
> This would work if there is a way to set Werror=coverage-mismatch
> without having to explicitly set the option classification as
> DK_ERROR.   Does this mechanism exist?
>
> Thanks,
>
> David
>
> On Tue, Apr 19, 2011 at 12:52 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Tue, Apr 19, 2011 at 9:13 AM, Xinliang David Li <davidxl@google.com> wrote:
>>> -Wcoverage-mismatch is enabled by default, and the warning is promoted
>>> to error by default. However in the current implementation -Wno-error
>>> can not demote the error back to warning. The patch was ported from
>>> one contributed by Neil.
>>>
>>> OK for trunk after regression testing?
>>
>> I am sure there is a better way to achieve this, like making
>> Werror=coverage-mismatch
>> the default.  Joseph?
>>
>> Richard.
>>
>>>
>>> 2011-04-18  Neil Vachharajani  <nvachhar@gmail.com>
>>>
>>>    * flags.c:  New flag variable.
>>>    * opts.c (common_handle_options): Set flag_werror_set.
>>>    * opts-global.c (decode_options): Delay Werror decision
>>>    for Wcoverage-mismatch util after options are parsed.
>>>
>>> The following test case can be added, but the test harness does not
>>> like the extra warnings -- how can they be pruned?
>>>
>>> Thanks,
>>>
>>> David
>>>
>>> /* { dg-options "-O2 -Wcoverage-mismatch -Wno-error" } */
>>>
>>> int __attribute__((noinline)) bar (void)
>>> {
>>> }
>>>
>>> #ifdef _PROFILE_USE
>>> int foo (int i)
>>> {
>>>  if (i)
>>>    bar ();
>>>  else
>>>   bar ();
>>>  return 0;
>>> }
>>> #else
>>> int foo (int i)
>>> {
>>>  if (i)
>>>    bar ();
>>>  return 0;
>>> }
>>> #endif
>>>
>>> int main(int argc, char **argv)
>>> {
>>>  foo (argc);
>>>  return 0;
>>> }
>>>
>>
>

Re: FDO usage: -Wcoverage-mismatch should not ignore -Wno-error

[Joseph S. Myers]

On Tue, 19 Apr 2011, Xinliang David Li wrote:

> 2011-04-18  Neil Vachharajani  <nvachhar@gmail.com>
> 
>     * flags.c:  New flag variable.
>     * opts.c (common_handle_options): Set flag_werror_set.
>     * opts-global.c (decode_options): Delay Werror decision
>     for Wcoverage-mismatch util after options are parsed.

This patch is certainly wrong, since common_handle_option must not set any 
global state, only state pointed to by its arguments.

That said, setting -Werror=coverage-mismatch in decode_options at all is 
bad because decode_options is called when optimize attributes are 
processed; as-is, a -Wno-error=coverage-mismatch option will be overridden 
if such attributes are used.

So the right place to set this is probably later, in process_options.  And 
this can check global_options_set.x_warnings_are_errors to see if an 
explicit -Werror/-Wno-error option was passed.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: FDO usage: -Wcoverage-mismatch should not ignore -Wno-error

[Xinliang David Li]

On Thu, Apr 21, 2011 at 12:54 PM, Joseph S. Myers
<joseph@codesourcery.com> wrote:
> On Tue, 19 Apr 2011, Xinliang David Li wrote:
>
>> 2011-04-18  Neil Vachharajani  <nvachhar@gmail.com>
>>
>>     * flags.c:  New flag variable.
>>     * opts.c (common_handle_options): Set flag_werror_set.
>>     * opts-global.c (decode_options): Delay Werror decision
>>     for Wcoverage-mismatch util after options are parsed.
>
> This patch is certainly wrong, since common_handle_option must not set any
> global state, only state pointed to by its arguments.
>
> That said, setting -Werror=coverage-mismatch in decode_options at all is
> bad because decode_options is called when optimize attributes are
> processed; as-is, a -Wno-error=coverage-mismatch option will be overridden
> if such attributes are used.

Not sure if I understand the comment on the 'option be overriden' --
this is not happening with the patch. As long as the the
diagnostic_classification for coverage-mismatch is explicitly
specified, it will be honored.

>
> So the right place to set this is probably later, in process_options.  And
> this can check global_options_set.x_warnings_are_errors to see if an
> explicit -Werror/-Wno-error option was passed.

The problem is that when warning_as_error_requested is 0, there is no
way to tell if it is the default or it is user has specified
-Wno-error. Maybe we should not make -Wcoverage-mismatch warnings to
be promoted to errors by default and relies on it to be turned on
explicitly via -Werror, or -Werror=coverage-mismatch.  There are
probably not many people depending on the old behavior.

Honza, what is your opinion on this?

Thanks,

David

>
> --
> Joseph S. Myers
> joseph@codesourcery.com
>

Re: FDO usage: -Wcoverage-mismatch should not ignore -Wno-error

[Joseph S. Myers]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1738 bytes --]

On Thu, 21 Apr 2011, Xinliang David Li wrote:

> On Thu, Apr 21, 2011 at 12:54 PM, Joseph S. Myers
> <joseph@codesourcery.com> wrote:
> > On Tue, 19 Apr 2011, Xinliang David Li wrote:
> >
> >> 2011-04-18  Neil Vachharajani  <nvachhar@gmail.com>
> >>
> >>     * flags.c:  New flag variable.
> >>     * opts.c (common_handle_options): Set flag_werror_set.
> >>     * opts-global.c (decode_options): Delay Werror decision
> >>     for Wcoverage-mismatch util after options are parsed.
> >
> > This patch is certainly wrong, since common_handle_option must not set any
> > global state, only state pointed to by its arguments.
> >
> > That said, setting -Werror=coverage-mismatch in decode_options at all is
> > bad because decode_options is called when optimize attributes are
> > processed; as-is, a -Wno-error=coverage-mismatch option will be overridden
> > if such attributes are used.
> 
> Not sure if I understand the comment on the 'option be overriden' --
> this is not happening with the patch. As long as the the

I am referring to the state before the patch.  But in general 
decode_options is the wrong place for any once-only initialization.

> > So the right place to set this is probably later, in process_options.  And
> > this can check global_options_set.x_warnings_are_errors to see if an
> > explicit -Werror/-Wno-error option was passed.
> 
> The problem is that when warning_as_error_requested is 0, there is no
> way to tell if it is the default or it is user has specified
> -Wno-error. Maybe we should not make -Wcoverage-mismatch warnings to

I referred to global_options_set.x_warnings_are_errors, not 
warning_as_error_requested.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: FDO usage: -Wcoverage-mismatch should not ignore -Wno-error

[Xinliang David Li]

That will work. Revised patch coming up.

Thanks,

David

On Thu, Apr 21, 2011 at 3:59 PM, Joseph S. Myers
<joseph@codesourcery.com> wrote:
> On Thu, 21 Apr 2011, Xinliang David Li wrote:
>
>> On Thu, Apr 21, 2011 at 12:54 PM, Joseph S. Myers
>> <joseph@codesourcery.com> wrote:
>> > On Tue, 19 Apr 2011, Xinliang David Li wrote:
>> >
>> >> 2011-04-18  Neil Vachharajani  <nvachhar@gmail.com>
>> >>
>> >>     * flags.c:  New flag variable.
>> >>     * opts.c (common_handle_options): Set flag_werror_set.
>> >>     * opts-global.c (decode_options): Delay Werror decision
>> >>     for Wcoverage-mismatch util after options are parsed.
>> >
>> > This patch is certainly wrong, since common_handle_option must not set any
>> > global state, only state pointed to by its arguments.
>> >
>> > That said, setting -Werror=coverage-mismatch in decode_options at all is
>> > bad because decode_options is called when optimize attributes are
>> > processed; as-is, a -Wno-error=coverage-mismatch option will be overridden
>> > if such attributes are used.
>>
>> Not sure if I understand the comment on the 'option be overriden' --
>> this is not happening with the patch. As long as the the
>
> I am referring to the state before the patch.  But in general
> decode_options is the wrong place for any once-only initialization.
>
>> > So the right place to set this is probably later, in process_options.  And
>> > this can check global_options_set.x_warnings_are_errors to see if an
>> > explicit -Werror/-Wno-error option was passed.
>>
>> The problem is that when warning_as_error_requested is 0, there is no
>> way to tell if it is the default or it is user has specified
>> -Wno-error. Maybe we should not make -Wcoverage-mismatch warnings to
>
> I referred to global_options_set.x_warnings_are_errors, not
> warning_as_error_requested.
>
> --
> Joseph S. Myers
> joseph@codesourcery.com

Re: FDO usage: -Wcoverage-mismatch should not ignore -Wno-error

[Xinliang David Li]

[-- Attachment #1: Type: text/plain, Size: 1920 bytes --]

Please review the new patch.

Thanks,

David

On Thu, Apr 21, 2011 at 3:59 PM, Joseph S. Myers
<joseph@codesourcery.com> wrote:
> On Thu, 21 Apr 2011, Xinliang David Li wrote:
>
>> On Thu, Apr 21, 2011 at 12:54 PM, Joseph S. Myers
>> <joseph@codesourcery.com> wrote:
>> > On Tue, 19 Apr 2011, Xinliang David Li wrote:
>> >
>> >> 2011-04-18  Neil Vachharajani  <nvachhar@gmail.com>
>> >>
>> >>     * flags.c:  New flag variable.
>> >>     * opts.c (common_handle_options): Set flag_werror_set.
>> >>     * opts-global.c (decode_options): Delay Werror decision
>> >>     for Wcoverage-mismatch util after options are parsed.
>> >
>> > This patch is certainly wrong, since common_handle_option must not set any
>> > global state, only state pointed to by its arguments.
>> >
>> > That said, setting -Werror=coverage-mismatch in decode_options at all is
>> > bad because decode_options is called when optimize attributes are
>> > processed; as-is, a -Wno-error=coverage-mismatch option will be overridden
>> > if such attributes are used.
>>
>> Not sure if I understand the comment on the 'option be overriden' --
>> this is not happening with the patch. As long as the the
>
> I am referring to the state before the patch.  But in general
> decode_options is the wrong place for any once-only initialization.
>
>> > So the right place to set this is probably later, in process_options.  And
>> > this can check global_options_set.x_warnings_are_errors to see if an
>> > explicit -Werror/-Wno-error option was passed.
>>
>> The problem is that when warning_as_error_requested is 0, there is no
>> way to tell if it is the default or it is user has specified
>> -Wno-error. Maybe we should not make -Wcoverage-mismatch warnings to
>
> I referred to global_options_set.x_warnings_are_errors, not
> warning_as_error_requested.
>
> --
> Joseph S. Myers
> joseph@codesourcery.com

[-- Attachment #2: wc2.p --]
[-- Type: application/octet-stream, Size: 1397 bytes --]

Index: toplev.c
===================================================================
--- toplev.c	(revision 172847)
+++ toplev.c	(working copy)
@@ -1600,6 +1600,15 @@ process_options (void)
       flag_omit_frame_pointer = 0;
     }
 
+  /* Enable -Werror=coverage-mismatch when -Werror and -Wno-error
+     have not been set.  */
+  if (!global_options_set.x_warnings_are_errors
+      && warn_coverage_mismatch
+      && (global_dc->classify_diagnostic[OPT_Wcoverage_mismatch] ==
+          DK_UNSPECIFIED))
+    diagnostic_classify_diagnostic (global_dc, OPT_Wcoverage_mismatch,
+                                    DK_ERROR, UNKNOWN_LOCATION);
+
   /* Save the current optimization options.  */
   optimization_default_node = build_optimization_node ();
   optimization_current_node = optimization_default_node;
Index: opts-global.c
===================================================================
--- opts-global.c	(revision 172847)
+++ opts-global.c	(working copy)
@@ -310,11 +310,6 @@ decode_options (struct gcc_options *opts
 
   set_default_handlers (&handlers);
 
-  /* Enable -Werror=coverage-mismatch by default.  */
-  control_warning_option (OPT_Wcoverage_mismatch, (int) DK_ERROR, true,
-			  loc, lang_mask,
-			  &handlers, opts, opts_set, dc);
-
   default_options_optimization (opts, opts_set,
 				decoded_options, decoded_options_count,
 				loc, lang_mask, &handlers, dc);

Re: FDO usage: -Wcoverage-mismatch should not ignore -Wno-error

[Joseph S. Myers]

On Thu, 21 Apr 2011, Xinliang David Li wrote:

> Please review the new patch.

The new patch is OK with a suitable ChangeLog entry.

-- 
Joseph S. Myers
joseph@codesourcery.com