[PATCH/RFC] options: Make --help= to emit values post-overrided

[PATCH/RFC] options: Make --help= to emit values post-overrided

[Kewen.Lin]

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

Hi,

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?

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.


BR,
Kewen
-----

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.


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

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,

Re: [PATCH/RFC] options: Make --help= to emit values post-overrided

[Segher Boessenkool]

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

Re: [PATCH/RFC] options: Make --help= to emit values post-overrided

[Kewen.Lin]

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

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

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

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,

[-- Attachment #3: opts_alt2.diff --]
[-- Type: text/plain, Size: 4565 bytes --]

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,

Re: [PATCH/RFC] options: Make --help= to emit values post-overrided

[Segher Boessenkool]

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

[PATCH] options: Make --help= to emit values post-overrided

[Kewen.Lin]

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

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.
> 
> 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 ;-)

Yeah, CC Joseph.  Thanks!

> 
> 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*.
> 

Good point!  Removed it.

Bootstrapped/regtested on powerpc64le-linux-gnu P8.

BR,
Kewen

*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.

*opts_alt2.diff*

gcc/ChangeLog:

	* opts-global.c (decode_options): Adjust call to print_help.
	* opts.c (print_specific_help): Add one function point argument
	target_option_override_hook and call it once.
	(print_help): Add one function point argument
	target_option_override_hook and pass it in print_specific_help call.
	(common_handle_option): Adjust calls to print_specific_help, refactor
	calls to target_option_override_hook.
	* opts.h (print_help): Add one more argument to function declare.


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

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,

[-- Attachment #3: opts_alt2.diff --]
[-- Type: text/plain, Size: 4516 bytes --]

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..c61e7b7d519 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,13 @@ print_specific_help (unsigned int include_flags,
 	}
     }
 
+  static bool call_override_once_p = false;
+  if (!call_override_once_p)
+    {
+      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 +2025,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 +2155,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 +2195,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 +2216,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,

Re: [PATCH/RFC] options: Make --help= to emit values post-overrided

[Martin Liška]

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

Re: [PATCH] options: Make --help= to emit values post-overrided

[Richard Sandiford]

"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

Re: [PATCH] options: Make --help= to emit values post-overrided

[Kewen.Lin]

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

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=*.



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

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.  */

Re: [PATCH] options: Make --help= to emit values post-overrided

[Segher Boessenkool]

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

[PATCH v2] options: Make --help= to emit values post-overrided

[Kewen.Lin]

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

Hi Segher,

on 2020/8/15 上午6:01, Segher Boessenkool wrote:
> 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.  */
> 

This looks better, thanks for polishing!  The updated one is attached.

BR,
Kewen
-----
gcc/ChangeLog:

	* opts-global.c (decode_options): Call target_option_override_hook
	before it prints for --help=*.

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

diff --git a/gcc/opts-global.c b/gcc/opts-global.c
index b1a8429dc3c..69fe2b4f3b1 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 ())
+    {
+      /* 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);
+    }
 }
 
 /* Hold command-line options associated with stack limitation.  */

Re: [PATCH v2] options: Make --help= to emit values post-overrided

[Richard Sandiford]

"Kewen.Lin" <linkw@linux.ibm.com> writes:
> 	* opts-global.c (decode_options): Call target_option_override_hook
> 	before it prints for --help=*.

OK, thanks.

Richard

> diff --git a/gcc/opts-global.c b/gcc/opts-global.c
> index b1a8429dc3c..69fe2b4f3b1 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 ())
> +    {
> +      /* 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);
> +    }
>  }
>  
>  /* Hold command-line options associated with stack limitation.  */