| Message ID | 9eb8a897-e4ee-a80a-4456-1e088ab8f301@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCH/RFC] options: Make --help= to emit values post-overrided | expand |
Hi! On Thu, Aug 06, 2020 at 08:37:23PM +0800, Kewen.Lin wrote: > When I was working to update patch as Richard's review comments > here https://gcc.gnu.org/pipermail/gcc-patches/2020-August/551474.html， > I noticed that the options "-Q --help=params" don't show the final values > after target option overriding, instead it emits the default values in > params.opt (without any explicit param settings). > > I guess it's more meaningful to get it to emit values post-overrided, > to avoid possible confusion for users. Does it make sense? > Or are there any concerns? I think this makes a lot of sense. > btw, not sure whether it's a good idea to move target_option_override_hook > call into print_specific_help and use one function local static > variable to control it's called once for all kinds of help dumping > (possible combination), then can remove the calls in function > common_handle_option. I cannot easily imagine what that will look like... it could easily be worse than what you have here (callbacks aren't so nice, but there are worse things). > @@ -2145,9 +2146,11 @@ print_help (struct gcc_options *opts, unsigned int lang_mask, > if (!(include_flags & CL_PARAMS)) > exclude_flags |= CL_PARAMS; > > - if (include_flags) > + if (include_flags) { > + target_option_override_hook (); > print_specific_help (include_flags, exclude_flags, 0, opts, > lang_mask); > + } > } Indenting should be like if (include_flags) { target_option_override_hook (); print_specific_help (include_flags, exclude_flags, 0, opts, lang_mask); } Segher
Hi Segher! Thanks for the comments! on 2020/8/7 上午6:04, Segher Boessenkool wrote: > Hi! > > On Thu, Aug 06, 2020 at 08:37:23PM +0800, Kewen.Lin wrote: >> When I was working to update patch as Richard's review comments >> here https://gcc.gnu.org/pipermail/gcc-patches/2020-August/551474.html， >> I noticed that the options "-Q --help=params" don't show the final values >> after target option overriding, instead it emits the default values in >> params.opt (without any explicit param settings). >> >> I guess it's more meaningful to get it to emit values post-overrided, >> to avoid possible confusion for users. Does it make sense? >> Or are there any concerns? > > I think this makes a lot of sense. > >> btw, not sure whether it's a good idea to move target_option_override_hook >> call into print_specific_help and use one function local static >> variable to control it's called once for all kinds of help dumping >> (possible combination), then can remove the calls in function >> common_handle_option. > > I cannot easily imagine what that will look like... it could easily be > worse than what you have here (callbacks aren't so nice, but there are > worse things). > I attached opts_alt2.diff to be more specific for this, both alt1 and alt2 follow the existing callback scheme, alt2 aims to avoid possible multiple times target_option_override_hook calls when we have several --help= or similar, but I guess alt1 is also fine since the hook should be allowed to be called more than once. >> @@ -2145,9 +2146,11 @@ print_help (struct gcc_options *opts, unsigned int lang_mask, >> if (!(include_flags & CL_PARAMS)) >> exclude_flags |= CL_PARAMS; >> >> - if (include_flags) >> + if (include_flags) { >> + target_option_override_hook (); >> print_specific_help (include_flags, exclude_flags, 0, opts, >> lang_mask); >> + } >> } > > Indenting should be like > > if (include_flags) > { > target_option_override_hook (); > print_specific_help (include_flags, exclude_flags, 0, opts, lang_mask); > } > Thanks for catching! Updated. BR, Kewen diff --git a/gcc/opts-global.c b/gcc/opts-global.c index b1a8429dc3c..ec960c87c9a 100644 --- a/gcc/opts-global.c +++ b/gcc/opts-global.c @@ -328,7 +328,7 @@ decode_options (struct gcc_options *opts, struct gcc_options *opts_set, const char *arg; FOR_EACH_VEC_ELT (help_option_arguments, i, arg) - print_help (opts, lang_mask, arg); + print_help (opts, lang_mask, arg, target_option_override_hook); } /* Hold command-line options associated with stack limitation. */ diff --git a/gcc/opts.c b/gcc/opts.c index 499eb900643..fb4d4b8aa43 100644 --- a/gcc/opts.c +++ b/gcc/opts.c @@ -2017,7 +2017,8 @@ check_alignment_argument (location_t loc, const char *flag, const char *name) void print_help (struct gcc_options *opts, unsigned int lang_mask, - const char *help_option_argument) + const char *help_option_argument, + void (*target_option_override_hook) (void)) { const char *a = help_option_argument; unsigned int include_flags = 0; @@ -2146,8 +2147,11 @@ print_help (struct gcc_options *opts, unsigned int lang_mask, exclude_flags |= CL_PARAMS; if (include_flags) - print_specific_help (include_flags, exclude_flags, 0, opts, - lang_mask); + { + gcc_assert (target_option_override_hook); + target_option_override_hook (); + print_specific_help (include_flags, exclude_flags, 0, opts, lang_mask); + } } /* Handle target- and language-independent options. Return zero to diff --git a/gcc/opts.h b/gcc/opts.h index 8f594b46e33..9a837305af1 100644 --- a/gcc/opts.h +++ b/gcc/opts.h @@ -419,8 +419,9 @@ extern bool target_handle_option (struct gcc_options *opts, extern void finish_options (struct gcc_options *opts, struct gcc_options *opts_set, location_t loc); -extern void print_help (struct gcc_options *opts, unsigned int lang_mask, const - char *help_option_argument); +extern void print_help (struct gcc_options *opts, unsigned int lang_mask, + const char *help_option_argument, + void (*target_option_override_hook) (void)); extern void default_options_optimization (struct gcc_options *opts, struct gcc_options *opts_set, struct cl_decoded_option *decoded_options, diff --git a/gcc/opts-global.c b/gcc/opts-global.c index b1a8429dc3c..ec960c87c9a 100644 --- a/gcc/opts-global.c +++ b/gcc/opts-global.c @@ -328,7 +328,7 @@ decode_options (struct gcc_options *opts, struct gcc_options *opts_set, const char *arg; FOR_EACH_VEC_ELT (help_option_arguments, i, arg) - print_help (opts, lang_mask, arg); + print_help (opts, lang_mask, arg, target_option_override_hook); } /* Hold command-line options associated with stack limitation. */ diff --git a/gcc/opts.c b/gcc/opts.c index 499eb900643..6d07fa6e17e 100644 --- a/gcc/opts.c +++ b/gcc/opts.c @@ -1579,7 +1579,8 @@ print_specific_help (unsigned int include_flags, unsigned int exclude_flags, unsigned int any_flags, struct gcc_options *opts, - unsigned int lang_mask) + unsigned int lang_mask, + void (*target_option_override_hook) (void)) { unsigned int all_langs_mask = (1U << cl_lang_count) - 1; const char * description = NULL; @@ -1664,6 +1665,14 @@ print_specific_help (unsigned int include_flags, } } + static bool call_override_once_p = false; + if (!call_override_once_p) + { + gcc_assert (target_option_override_hook); + target_option_override_hook (); + call_override_once_p = true; + } + printf ("%s%s:\n", description, descrip_extra); print_filtered_help (include_flags, exclude_flags, any_flags, opts->x_help_columns, opts, lang_mask); @@ -2017,7 +2026,8 @@ check_alignment_argument (location_t loc, const char *flag, const char *name) void print_help (struct gcc_options *opts, unsigned int lang_mask, - const char *help_option_argument) + const char *help_option_argument, + void (*target_option_override_hook) (void)) { const char *a = help_option_argument; unsigned int include_flags = 0; @@ -2146,8 +2156,8 @@ print_help (struct gcc_options *opts, unsigned int lang_mask, exclude_flags |= CL_PARAMS; if (include_flags) - print_specific_help (include_flags, exclude_flags, 0, opts, - lang_mask); + print_specific_help (include_flags, exclude_flags, 0, opts, lang_mask, + target_option_override_hook); } /* Handle target- and language-independent options. Return zero to @@ -2186,18 +2196,19 @@ common_handle_option (struct gcc_options *opts, undoc_mask = ((opts->x_verbose_flag | opts->x_extra_warnings) ? 0 : CL_UNDOCUMENTED); - target_option_override_hook (); /* First display any single language specific options. */ for (i = 0; i < cl_lang_count; i++) - print_specific_help - (1U << i, (all_langs_mask & (~ (1U << i))) | undoc_mask, 0, opts, - lang_mask); + print_specific_help (1U << i, + (all_langs_mask & (~(1U << i))) | undoc_mask, 0, + opts, lang_mask, target_option_override_hook); /* Next display any multi language specific options. */ - print_specific_help (0, undoc_mask, all_langs_mask, opts, lang_mask); + print_specific_help (0, undoc_mask, all_langs_mask, opts, lang_mask, + target_option_override_hook); /* Then display any remaining, non-language options. */ for (i = CL_MIN_OPTION_CLASS; i <= CL_MAX_OPTION_CLASS; i <<= 1) if (i != CL_DRIVER) - print_specific_help (i, undoc_mask, 0, opts, lang_mask); + print_specific_help (i, undoc_mask, 0, opts, lang_mask, + target_option_override_hook); opts->x_exit_after_options = true; break; } @@ -2206,8 +2217,8 @@ common_handle_option (struct gcc_options *opts, if (lang_mask == CL_DRIVER) break; - target_option_override_hook (); - print_specific_help (CL_TARGET, CL_UNDOCUMENTED, 0, opts, lang_mask); + print_specific_help (CL_TARGET, CL_UNDOCUMENTED, 0, opts, lang_mask, + target_option_override_hook); opts->x_exit_after_options = true; break; diff --git a/gcc/opts.h b/gcc/opts.h index 8f594b46e33..9a837305af1 100644 --- a/gcc/opts.h +++ b/gcc/opts.h @@ -419,8 +419,9 @@ extern bool target_handle_option (struct gcc_options *opts, extern void finish_options (struct gcc_options *opts, struct gcc_options *opts_set, location_t loc); -extern void print_help (struct gcc_options *opts, unsigned int lang_mask, const - char *help_option_argument); +extern void print_help (struct gcc_options *opts, unsigned int lang_mask, + const char *help_option_argument, + void (*target_option_override_hook) (void)); extern void default_options_optimization (struct gcc_options *opts, struct gcc_options *opts_set, struct cl_decoded_option *decoded_options,
Hi! On Fri, Aug 07, 2020 at 10:44:10AM +0800, Kewen.Lin wrote: > > I think this makes a lot of sense. > > > >> btw, not sure whether it's a good idea to move target_option_override_hook > >> call into print_specific_help and use one function local static > >> variable to control it's called once for all kinds of help dumping > >> (possible combination), then can remove the calls in function > >> common_handle_option. > > > > I cannot easily imagine what that will look like... it could easily be > > worse than what you have here (callbacks aren't so nice, but there are > > worse things). > > > > I attached opts_alt2.diff to be more specific for this, both alt1 and alt2 > follow the existing callback scheme, alt2 aims to avoid possible multiple > times target_option_override_hook calls when we have several --help= or > similar, but I guess alt1 is also fine since the hook should be allowed to > be called more than once. It could take quadratic time in alt1... Mostly a theoretical problem I think, but still. All options look fine to me, but you need someone else to approve this ;-) One thing: > @@ -1664,6 +1665,14 @@ print_specific_help (unsigned int include_flags, > } > } > > + static bool call_override_once_p = false; > + if (!call_override_once_p) > + { > + gcc_assert (target_option_override_hook); > + target_option_override_hook (); > + call_override_once_p = true; > + } That assert is pretty random, nothing else using the hook assert it isn't zero; it immediately and always calls the hook right away, so you will get a nice ICE with backtrace if it is zero *anyway*. Cheers, Segher
On 8/7/20 12:04 AM, Segher Boessenkool wrote:
> I think this makes a lot of sense.
Hello.
I also support the patch (as Segher, I can't approve it).
Martin
diff --git a/gcc/opts-global.c b/gcc/opts-global.c index b1a8429dc3c..ec960c87c9a 100644 --- a/gcc/opts-global.c +++ b/gcc/opts-global.c @@ -328,7 +328,7 @@ decode_options (struct gcc_options *opts, struct gcc_options *opts_set, const char *arg; FOR_EACH_VEC_ELT (help_option_arguments, i, arg) - print_help (opts, lang_mask, arg); + print_help (opts, lang_mask, arg, target_option_override_hook); } /* Hold command-line options associated with stack limitation. */ diff --git a/gcc/opts.c b/gcc/opts.c index 499eb900643..df184f909e6 100644 --- a/gcc/opts.c +++ b/gcc/opts.c @@ -2017,7 +2017,8 @@ check_alignment_argument (location_t loc, const char *flag, const char *name) void print_help (struct gcc_options *opts, unsigned int lang_mask, - const char *help_option_argument) + const char *help_option_argument, + void (*target_option_override_hook) (void)) { const char *a = help_option_argument; unsigned int include_flags = 0; @@ -2145,9 +2146,11 @@ print_help (struct gcc_options *opts, unsigned int lang_mask, if (!(include_flags & CL_PARAMS)) exclude_flags |= CL_PARAMS; - if (include_flags) + if (include_flags) { + target_option_override_hook (); print_specific_help (include_flags, exclude_flags, 0, opts, lang_mask); + } } /* Handle target- and language-independent options. Return zero to diff --git a/gcc/opts.h b/gcc/opts.h index 8f594b46e33..9a837305af1 100644 --- a/gcc/opts.h +++ b/gcc/opts.h @@ -419,8 +419,9 @@ extern bool target_handle_option (struct gcc_options *opts, extern void finish_options (struct gcc_options *opts, struct gcc_options *opts_set, location_t loc); -extern void print_help (struct gcc_options *opts, unsigned int lang_mask, const - char *help_option_argument); +extern void print_help (struct gcc_options *opts, unsigned int lang_mask, + const char *help_option_argument, + void (*target_option_override_hook) (void)); extern void default_options_optimization (struct gcc_options *opts, struct gcc_options *opts_set, struct cl_decoded_option *decoded_options,