| Message ID | 5370F366.80300@ubuntu.com |
|---|---|
| State | New |
| Headers | show |
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).
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.
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.
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.
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
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.
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; }