[patch] fix impliedness of -Wunused-parameter depending on -Wexta option ordering

[patch] fix impliedness of -Wunused-parameter depending on -Wexta option ordering

[Matthias Klose]

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

This fixes a regression introduced with 4.8, where the option ordering of
-Wextra and -Wunused-parameter emits a warning, which is not emitted with 4.7.
No regressions with the trunk, the 4.9 and 4.8 branches. Ok to check in for these?

  Matthias

2014-05-08  Manuel L<C3><B3>pez-Ib<C3><A1><C3><B1>ez  <manu@gcc.gnu.org>
            Matthias Klose  <doko@ubuntu.com>

        PR driver/61106
        * optc-gen.awk: Fix option handling for -Wunused-parameter.

gcc/testsuite/

2014-05-08  Matthias Klose  <doko@ubuntu.com>

        PR driver/61106
        * gcc-dg/unused-8a.c: New.
        * gcc-dg/unused-8b.c: Likewise.



[-- Attachment #2: warn-unused.diff --]
[-- Type: text/plain, Size: 1976 bytes --]

gcc/

2014-05-08  Manuel López-Ibáñez  <manu@gcc.gnu.org>
	    Matthias Klose  <doko@ubuntu.com>

	PR driver/61106
	* optc-gen.awk: Fix option handling for -Wunused-parameter.

gcc/testsuite/

2014-05-08  Matthias Klose  <doko@ubuntu.com>

	PR driver/61106
	* gcc-dg/unused-8a.c: New.
	* gcc-dg/unused-8b.c: Likewise.

Index: gcc/optc-gen.awk
===================================================================
--- gcc/optc-gen.awk	(revision 210245)
+++ gcc/optc-gen.awk	(working copy)
@@ -406,11 +406,13 @@
         if (opt_var_name != "") {
             condition = "!opts_set->x_" opt_var_name
             if (thisenableif[j] != "") {
-                condition = condition " && (" thisenableif[j] ")"
+                value = "(" thisenableif[j] ")"
+            } else {
+                value = "value"
             }
             print "      if (" condition ")"
             print "        handle_generated_option (opts, opts_set,"
-            print "                                 " opt_enum(thisenable[j]) ", NULL, value,"
+            print "                                 " opt_enum(thisenable[j]) ", NULL, " value ","
             print "                                 lang_mask, kind, loc, handlers, dc);"
         } else {
             print "#error " thisenable[j] " does not have a Var() flag"
Index: gcc/testsuite/gcc.dg/unused-8a.c
===================================================================
--- gcc/testsuite/gcc.dg/unused-8a.c	(revision 0)
+++ gcc/testsuite/gcc.dg/unused-8a.c	(working copy)
@@ -0,0 +1,4 @@
+/* { dg-do compile } */
+/* { dg-options "-Wall -Wextra -Wno-unused" } */
+
+void foo(int x) { }
Index: gcc/testsuite/gcc.dg/unused-8b.c
===================================================================
--- gcc/testsuite/gcc.dg/unused-8b.c	(revision 0)
+++ gcc/testsuite/gcc.dg/unused-8b.c	(working copy)
@@ -0,0 +1,4 @@
+/* { dg-do compile } */
+/* { dg-options "-Wall -Wno-unused -Wextra" } */
+
+void foo(int x) { }

Re: [patch] fix impliedness of -Wunused-parameter depending on -Wexta option ordering

[Joseph S. Myers]

On Thu, 8 May 2014, Matthias Klose wrote:

> This fixes a regression introduced with 4.8, where the option ordering 
> of -Wextra and -Wunused-parameter emits a warning, which is not emitted 
> with 4.7. No regressions with the trunk, the 4.9 and 4.8 branches. Ok to 
> check in for these?

OK.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [patch] fix impliedness of -Wunused-parameter depending on -Wexta option ordering

[Matthias Klose]

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

Am 08.05.2014 23:36, schrieb Joseph S. Myers:
> On Thu, 8 May 2014, Matthias Klose wrote:
> 
>> This fixes a regression introduced with 4.8, where the option ordering 
>> of -Wextra and -Wunused-parameter emits a warning, which is not emitted 
>> with 4.7. No regressions with the trunk, the 4.9 and 4.8 branches. Ok to 
>> check in for these?
> 
> OK.

I didn't look close enough to the gfortran test results.  PR driver/61126 is a
fix for the regression introduced with the fix for the above issue.  With this
patch proposed by Manuel, gfortran.dg/wextra_1.f now passes, and no new
regressions seen on the trunk and the branches.

  Matthias



[-- Attachment #2: pr61126.diff --]
[-- Type: text/plain, Size: 2046 bytes --]

gcc/fortran/

	PR driver/61126
	* options.c (gfc_handle_option): Don't enable -Wunused-parameter
	with -Wextra
	* lang.opt (Wunused-parameter): New.

gcc/

	PR driver/61126
	* opts-global.c (set_default_handlers): Fix order of handlers.

Index: gcc/fortran/lang.opt
===================================================================
--- a/src/gcc/fortran/lang.opt	(revision 210277)
+++ b/src/gcc/fortran/lang.opt	(working copy)
@@ -301,6 +301,10 @@
 Fortran Warning
 Warn about unused dummy arguments.
 
+Wunused-parameter
+LangEnabledBy(Fortran,Wextra)
+; Documented in common.opt
+
 Wzerotrip
 Fortran Warning
 Warn about zero-trip DO loops
Index: gcc/fortran/options.c
===================================================================
--- a/src/gcc/fortran/options.c	(revision 210277)
+++ b/src/gcc/fortran/options.c	(working copy)
@@ -674,12 +674,7 @@
       break;
 
     case OPT_Wextra:
-      handle_generated_option (&global_options, &global_options_set,
-			       OPT_Wunused_parameter, NULL, value,
-			       gfc_option_lang_mask (), kind, loc,
-			       handlers, global_dc);
       set_Wextra (value);
-
       break;
 
     case OPT_Wfunction_elimination:
Index: gcc/opts-global.c
===================================================================
--- a/src/gcc/opts-global.c	(revision 210277)
+++ b/src/gcc/opts-global.c	(working copy)
@@ -273,10 +273,10 @@
   handlers->unknown_option_callback = unknown_option_callback;
   handlers->wrong_lang_callback = complain_wrong_lang;
   handlers->num_handlers = 3;
-  handlers->handlers[0].handler = lang_handle_option;
-  handlers->handlers[0].mask = initial_lang_mask;
-  handlers->handlers[1].handler = common_handle_option;
-  handlers->handlers[1].mask = CL_COMMON;
+  handlers->handlers[0].handler = common_handle_option;
+  handlers->handlers[0].mask = CL_COMMON;
+  handlers->handlers[1].handler = lang_handle_option;
+  handlers->handlers[1].mask = initial_lang_mask;
   handlers->handlers[2].handler = target_handle_option;
   handlers->handlers[2].mask = CL_TARGET;
 }

Re: [patch] fix impliedness of -Wunused-parameter depending on -Wexta option ordering

[Joseph S. Myers]

On Mon, 12 May 2014, Matthias Klose wrote:

> I didn't look close enough to the gfortran test results.  PR driver/61126 is a
> fix for the regression introduced with the fix for the above issue.  With this
> patch proposed by Manuel, gfortran.dg/wextra_1.f now passes, and no new
> regressions seen on the trunk and the branches.

I think changing the order of the handlers has far too high a risk of 
introducing further nonobvious regressions to consider it for the 
branches.  You need a clear and careful analysis of the circumstances 
under which the order of the handlers can affect observable compiler 
behavior in order to justify such a change as safe.  But I think a better 
principle is that if the order matters, there is a bug in those handlers 
and they should be fixed so that the order doesn't matter (absent a clear 
design showing why it is desirable for the order to matter).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [patch] fix impliedness of -Wunused-parameter depending on -Wexta option ordering

[Manuel López-Ibáñez]

On 12/05/2014, Joseph S. Myers <joseph@codesourcery.com> wrote:
> On Mon, 12 May 2014, Matthias Klose wrote:
>
>> I didn't look close enough to the gfortran test results.  PR driver/61126
>> is a
>> fix for the regression introduced with the fix for the above issue.  With
>> this
>> patch proposed by Manuel, gfortran.dg/wextra_1.f now passes, and no new
>> regressions seen on the trunk and the branches.
>
> I think changing the order of the handlers has far too high a risk of
> introducing further nonobvious regressions to consider it for the
> branches.  You need a clear and careful analysis of the circumstances
> under which the order of the handlers can affect observable compiler
> behavior in order to justify such a change as safe.  But I think a better
> principle is that if the order matters, there is a bug in those handlers
> and they should be fixed so that the order doesn't matter (absent a clear
> design showing why it is desirable for the order to matter)

(Resending because Android Gmail is defective by design, sorry Joseph
and Matthias. )

The only way I can think of making the handlers robust to any ordering
is to have a flag per option that says which handler has set up which
option, so if a default value has been setup by the FE, then the
common handler does not override it.

That seems much more complex and wasteful than simply decree by design
that targets can override FEs and FEs can override common defaults,
but not in the other direction. It is the only order that makes sense
to me.
I will be very surprised if the common defaults are overriding a FE
default and it is not a bug in the FE.

The other alternative is to move -Wunused-parameter out of common.opt,
and have each FE define the same default, except Fortran. That seems
also a unnecessary duplication.

Re: [patch] fix impliedness of -Wunused-parameter depending on -Wexta option ordering

[Joseph S. Myers]

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

On Mon, 12 May 2014, Manuel López-Ibáñez wrote:

> I will be very surprised if the common defaults are overriding a FE
> default and it is not a bug in the FE.

Well, I think that needs justification, not just "very surprised".  
Identify (presumably in an automated way) all the cases where an option is 
handled by more than one of the handlers and, for each one, describe what 
cases would be affected by a change of order and why the proposed new 
order gives the desired effects.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [patch] fix impliedness of -Wunused-parameter depending on -Wexta option ordering

[Manuel López-Ibáñez]

On 12 May 2014 22:24, Joseph S. Myers <joseph@codesourcery.com> wrote:
> On Mon, 12 May 2014, Manuel López-Ibáñez wrote:
>
>> I will be very surprised if the common defaults are overriding a FE
>> default and it is not a bug in the FE.
>
> Well, I think that needs justification, not just "very surprised".
> Identify (presumably in an automated way) all the cases where an option is
> handled by more than one of the handlers and, for each one, describe what
> cases would be affected by a change of order and why the proposed new
> order gives the desired effects.

common_handle_option_auto handles just OPT_Wextra,
OPT_Wuninitialized: and OPT_Wunused.

Fortran is the only one that handles OPT_Wextra in the FE, and the
current order prevents Fortran to override the current default, while
the new order will allow it.

These are the options handled in gcc/opts.c

OPT_auxbase_strip
OPT_aux_info
OPT_d
OPT_fcall_saved_
OPT_fcall_used_
OPT_fdbg_cnt_
OPT_fdbg_cnt_list
OPT_fdebug_prefix_map_
OPT_fdiagnostics_color_
OPT_fdiagnostics_show_caret
OPT_fdiagnostics_show_location_
OPT_fdiagnostics_show_option
OPT_fdump_
OPT_ffast_math
OPT_ffixed_
OPT_finline_limit_
OPT_finstrument_functions_exclude_file_list_
OPT_finstrument_functions_exclude_function_list_
OPT_flto
OPT_fmax_errors_
OPT_fmessage_length_
OPT_fopt_info
OPT_fopt_info_
OPT_fpack_struct_
OPT_fplugin_
OPT_fplugin_arg_
OPT_fprofile_generate
OPT_fprofile_generate_
OPT_fprofile_use
OPT_fprofile_use_
OPT_frandom_seed
OPT_frandom_seed_
OPT_fsanitize_
OPT_fsched_stalled_insns_
OPT_fsched_stalled_insns_dep_
OPT_fsched_verbose_
OPT_fshow_column
OPT_fstack_check_
OPT_fstack_limit
OPT_fstack_limit_register_
OPT_fstack_limit_symbol_
OPT_fstack_usage
OPT_ftrapv
OPT_ftree_vectorize
OPT_funsafe_math_optimizations
OPT_fuse_ld_bfd
OPT_fuse_ld_gold
OPT_fuse_linker_plugin
OPT_fwrapv
OPT_g
OPT_gcoff
OPT_gdwarf
OPT_gdwarf_
OPT_ggdb
OPT_gsplit_dwarf
OPT_gstabs
OPT_gstabs_
OPT_gvms
OPT_gxcoff
OPT_gxcoff_
OPT__help
OPT__help_
OPT_O
OPT_Ofast
OPT_Og
OPT_Os
OPT__param
OPT_pedantic_errors
OPT__target_help
OPT__version
OPT_w
OPT_Werror
OPT_Werror_
OPT_Wfatal_errors
OPT_Wframe_larger_than_
OPT_Wlarger_than_
OPT_Wstack_usage_
OPT_Wstrict_aliasing
OPT_Wstrict_overflow
OPT_Wsystem_headers

I found the ones handled in the FEs by using:

$ for o in $(grep 'case OPT_'  gcc/opts.c | sed 's/^\s\+case \+//g' |
sort -u); do grep $o gcc/ada/gcc-interface/misc.c
gcc/fortran/options.c gcc/fortran/cpp.c  gcc/java/lang.c
gcc/lto/lto-lang.c gcc/c-family/c-opts.c; done
gcc/fortran/options.c:        case OPT_d:
gcc/fortran/options.c:        case OPT_d:
gcc/fortran/cpp.c:    case OPT_d:
gcc/c-family/c-opts.c:    case OPT_d:
gcc/java/lang.c:    case OPT_fdump_:

Of these, OPT_fdump_ is deferred in gcc/opts.c, so the order does not matter.

For OPT_d, the common handler does:

      case 'D': /* These are handled by the preprocessor.  */
      case 'I':
      case 'M':
      case 'N':
      case 'U':
        break;

While Fortran does:

   case OPT_d:
      for ( ; *arg; ++arg)
        switch (*arg)
        {
          case 'D':
          case 'M':
          case 'N':
          case 'U':
            gfc_cpp_option.dump_macros = *arg;
            break;

          case 'I':
            gfc_cpp_option.dump_includes = 1;
            break;
        }
      break;

and C/C++ does:

  while ((c = *arg++) != '\0')
    switch (c)
      {
      case 'M':                 /* Dump macros only.  */
      case 'N':                 /* Dump names.  */
      case 'D':                 /* Dump definitions.  */
      case 'U':                 /* Dump used macros.  */
        flag_dump_macros = c;
        break;

      case 'I':
        flag_dump_includes = 1;
        break;
      }


Am I missing something?

Cheers,

Manuel.

Re: [patch] fix impliedness of -Wunused-parameter depending on -Wexta option ordering

[Matthias Klose]

Am 12.05.2014 19:30, schrieb Joseph S. Myers:
> On Mon, 12 May 2014, Matthias Klose wrote:
> 
>> I didn't look close enough to the gfortran test results.  PR driver/61126 is a
>> fix for the regression introduced with the fix for the above issue.  With this
>> patch proposed by Manuel, gfortran.dg/wextra_1.f now passes, and no new
>> regressions seen on the trunk and the branches.
> 
> I think changing the order of the handlers has far too high a risk of 
> introducing further nonobvious regressions to consider it for the 
> branches.  You need a clear and careful analysis of the circumstances 
> under which the order of the handlers can affect observable compiler 
> behavior in order to justify such a change as safe.  But I think a better 
> principle is that if the order matters, there is a bug in those handlers 
> and they should be fixed so that the order doesn't matter (absent a clear 
> design showing why it is desirable for the order to matter).

reverted the fix for PR driver/61106 on the branches, keeping the unused-8b.c
test case.

  Matthias

Re: [patch] fix impliedness of -Wunused-parameter depending on -Wexta option ordering

[Joseph S. Myers]

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

On Wed, 14 May 2014, Manuel López-Ibáñez wrote:

> Am I missing something?

No, that seems sufficient (together with the observation that the target 
handlers remain called after the others, so while there may be such bugs 
involving them those bugs are irrelevant to this patch).  The patch is OK 
for mainline.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [patch] fix impliedness of -Wunused-parameter depending on -Wexta option ordering

[Dominique Dhumieres]

> This fixes a regression introduced with 4.8, ...

This caused pr61126.

Dominique