| Message ID | 43e70605-d708-373c-c069-f8bbd3f9a8b0@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | options: Make --help= to emit values post-overrided | expand |
"Kewen.Lin" <linkw@linux.ibm.com> writes: > Hi Segher, > > on 2020/8/7 锟斤拷锟斤拷10:42, Segher Boessenkool wrote: >> 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. Yeah. I guess ideally (and independently of this patch) we'd have a flag_checking assert that override_options is idempotent, but that might be tricky to implement. It looks like there's a subtle (pre-existing) difference in what --help and --help= do. --help already calls target_option_override_hook, but does it at the point that the option occurs. --help= instead queues the help until we've finished processing other arguments, and would therefore take later options into account. I don't know that one is obviously better than the other though. > […] > *opts_alt1.diff* > > gcc/ChangeLog: > > * opts-global.c (decode_options): Adjust call to print_help. > * opts.c (print_help): Add one function point argument > target_option_override_hook and call it before print_specific_help. > * opts.h (print_help): Add one more argument to function declare. I think personally I'd prefer an option (3): call target_option_override_hook directly in decode_options, if help_option_arguments is nonempty. Like you say, decode_options appears to be the only caller of print_help. Thanks, Richard
Hi Richard, Thanks for the comments! on 2020/8/13 上午12:10, Richard Sandiford wrote: > "Kewen.Lin" <linkw@linux.ibm.com> writes: >> Hi Segher, >> >> on 2020/8/7 锟斤拷锟斤拷10:42, Segher Boessenkool wrote: >>> 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. > > Yeah. I guess ideally (and independently of this patch) we'd have a > flag_checking assert that override_options is idempotent, but that > might be tricky to implement. > > It looks like there's a subtle (pre-existing) difference in what --help > and --help= do. --help already calls target_option_override_hook, > but does it at the point that the option occurs. --help= instead > queues the help until we've finished processing other arguments, > and would therefore take later options into account. > Yes, it is. > I don't know that one is obviously better than the other though. > >> […] >> *opts_alt1.diff* >> >> gcc/ChangeLog: >> >> * opts-global.c (decode_options): Adjust call to print_help. >> * opts.c (print_help): Add one function point argument >> target_option_override_hook and call it before print_specific_help. >> * opts.h (print_help): Add one more argument to function declare. > > I think personally I'd prefer an option (3): call > target_option_override_hook directly in decode_options, > if help_option_arguments is nonempty. Like you say, > decode_options appears to be the only caller of print_help. > Good idea! The related patch is attached, different from opts_alt{1,2} it could still call target_option_override_hook even if we won't call print_specific_help eventually for some special cases like lang_mask is CL_DRIVER or include_flags is empty. But I think it's fine. Also bootstrapped/regtested on powerpc64le-linux-gnu P8. BR, Kewen ----- gcc/ChangeLog: * opts-global.c (decode_options): Call target_option_override_hook before it prints for --help=*. diff --git a/gcc/opts-global.c b/gcc/opts-global.c index b1a8429dc3c..fc332871cb8 100644 --- a/gcc/opts-global.c +++ b/gcc/opts-global.c @@ -327,8 +327,14 @@ decode_options (struct gcc_options *opts, struct gcc_options *opts_set, unsigned i; const char *arg; - FOR_EACH_VEC_ELT (help_option_arguments, i, arg) - print_help (opts, lang_mask, arg); + if (!help_option_arguments.is_empty ()) + { + /* Consider post-overrided values for --help=*. */ + target_option_override_hook (); + + FOR_EACH_VEC_ELT (help_option_arguments, i, arg) + print_help (opts, lang_mask, arg); + } } /* Hold command-line options associated with stack limitation. */
Hi! On Fri, Aug 14, 2020 at 01:42:24PM +0800, Kewen.Lin wrote: > > I think personally I'd prefer an option (3): call > > target_option_override_hook directly in decode_options, > > if help_option_arguments is nonempty. Like you say, > > decode_options appears to be the only caller of print_help. > > Good idea! The related patch is attached, different from opts_alt{1,2} > it could still call target_option_override_hook even if we won't call > print_specific_help eventually for some special cases like lang_mask is > CL_DRIVER or include_flags is empty. But I think it's fine. > --- a/gcc/opts-global.c > +++ b/gcc/opts-global.c > @@ -327,8 +327,14 @@ decode_options (struct gcc_options *opts, struct gcc_options *opts_set, > unsigned i; > const char *arg; > > - FOR_EACH_VEC_ELT (help_option_arguments, i, arg) > - print_help (opts, lang_mask, arg); > + if (!help_option_arguments.is_empty ()) > + { > + /* Consider post-overrided values for --help=*. */ I'd say /* Make sure --help=* see the overridden values. */ > + target_option_override_hook (); > + > + FOR_EACH_VEC_ELT (help_option_arguments, i, arg) > + print_help (opts, lang_mask, arg); > + } > } The patch looks just fine to me. But, not my call :-) Segher
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..a83b2f837dd 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,10 @@ 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); + { + 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,