[v2] driver: fix a problem with implementation of -falign-foo=0 [PR96247]

Thanks, Richard!

I think your suggestion is very good, so I made a new patch.

v2: at a high level handles -falign-foo=0 like -falign-foo
v1: at the target level overides the -falign-foo=0 option values

Obviously, v2 is better than v1. In addition, anthor option
to reject 0 that discussed in the email and PR96247
is not as good as the current patch either, I think.

I tested this patch on x86_64, it works well. OK for trunk?

Regards!
Hujp

---
 gcc/opts.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Add CC to Richard.

> Thanks, Richard!
> 
> I think your suggestion is very good, so I made a new patch.
> 
> v2: at a high level handles -falign-foo=0 like -falign-foo
> v1: at the target level overides the -falign-foo=0 option values
> 
> Obviously, v2 is better than v1. In addition, anthor option
> to reject 0 that discussed in the email and PR96247
> is not as good as the current patch either, I think.
> 
> I tested this patch on x86_64, it works well. OK for trunk?
> 
> Regards!
> Hujp
> 
> ---
>  gcc/opts.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/gcc/opts.c b/gcc/opts.c
> index 499eb900643..ed6102cd606 100644
> --- a/gcc/opts.c
> +++ b/gcc/opts.c
> @@ -2786,18 +2786,38 @@ common_handle_option (struct gcc_options
> *opts,
> 
>      case OPT_falign_loops_:
>        check_alignment_argument (loc, arg, "loops");
> +      // fix PR96247
> +      if (0 == atoi(arg)) {
> +        opts->x_flag_align_loops = true;
> +        opts->x_str_align_loops = NULL;
> +      }
>        break;
> 
>      case OPT_falign_jumps_:
>        check_alignment_argument (loc, arg, "jumps");
> +      // fix PR96247
> +      if (0 == atoi(arg)) {
> +        opts->x_flag_align_jumps = true;
> +        opts->x_str_align_jumps = NULL;
> +      }
>        break;
> 
>      case OPT_falign_labels_:
>        check_alignment_argument (loc, arg, "labels");
> +      // fix PR96247
> +      if (0 == atoi(arg)) {
> +        opts->x_flag_align_labels = true;
> +        opts->x_str_align_labels = NULL;
> +      }
>        break;
> 
>      case OPT_falign_functions_:
>        check_alignment_argument (loc, arg, "functions");
> +      // fix PR96247
> +      if (0 == atoi(arg)) {
> +        opts->x_flag_align_functions = true;
> +        opts->x_str_align_functions = NULL;
> +      }
>        break;
> 
>      case OPT_ftabstop_:
> --
> 2.17.1
> 
>

Hi!

Just some random comments...

On Thu, Jul 23, 2020 at 04:44:21PM +0800, Hu Jiangping wrote:
> +      // fix PR96247

      /* See PR96247.  */

> +      if (0 == atoi(arg)) {

Either

      if (atoi (arg) == 0)
        {
          blalalala

or

      if (!atoi (arg))
        {
          blalalala

(whichever reads best in this context).

> +      // fix PR96247

Repeating that many times isn't helping the reader...  It isn't
particularly useful even a single time, anyway?  It is clear what this
does, and if anyone wants to see history, we have Git.


Segher

Hu Jiangping <hujiangping@cn.fujitsu.com> writes:
> Thanks, Richard!
>
> I think your suggestion is very good, so I made a new patch.
>
> v2: at a high level handles -falign-foo=0 like -falign-foo
> v1: at the target level overides the -falign-foo=0 option values
>
> Obviously, v2 is better than v1. In addition, anthor option
> to reject 0 that discussed in the email and PR96247
> is not as good as the current patch either, I think.
>
> I tested this patch on x86_64, it works well. OK for trunk?

In addition to Segher's comments, I wonder if it would be better
to pass &opts->x_flag_align_foo and &opts->x_str_align_jumps to
check_alignment_argument and do the check there instead.
The condition for whether to do this would then be:

  align_result.length () == 1 && align_result[0] == 0

The reason for suggesting that is that it makes the parsing code
more self-consistent, rather than using atoi for this case only.

Looks good otherwise, thanks.

Richard

> In addition to Segher's comments, I wonder if it would be better
> to pass &opts->x_flag_align_foo and &opts->x_str_align_jumps to
> check_alignment_argument and do the check there instead.
> The condition for whether to do this would then be:
> 
>   align_result.length () == 1 && align_result[0] == 0
> 
> The reason for suggesting that is that it makes the parsing code
> more self-consistent, rather than using atoi for this case only.
> 
Thanks, Segher and Richard!

I'll make a new patch to do the check in check_alignment_argument,
and change the condition of the if statement as follows:

	align_result.length () >= 1 && align_result[0] == 0

for the input -falign-foo=n:m:n2:m2, according to documentation,
if n is zero, use the machine-dependent value. I think the implict
meaning is that even if m or n2 or m2 is specified, it should be
ignored. Any comments are appreciated!

> Looks good otherwise, thanks.
> 
> Richard
>