Fix -Wformat-diag for s390x target.

Hello.

We've got -Wformat-diag for some time and I think we should start using it
in -Werror for GCC bootstrap. The following patch removes last pieces of the warning
for s390x target.

Ready to be installed?
Thanks,
Martin


gcc/ChangeLog:

	* config/s390/s390-c.c (s390_expand_overloaded_builtin): Wrap
	keyword in quotes.
	(s390_resolve_overloaded_builtin): Remove trailing dot.
	* config/s390/s390.c (s390_const_operand_ok): Use - for range.
	(s390_expand_builtin): Remove trailing dot.
	(s390_emit_prologue): Likewise, use semicolon.
	(s390_option_override_internal): Update keyword.
	* varasm.c (do_assemble_alias): Wrap keyword in quotes.
---
  gcc/config/s390/s390-c.c |  9 +++++----
  gcc/config/s390/s390.c   | 28 ++++++++++++++--------------
  gcc/varasm.c             |  2 +-
  3 files changed, 20 insertions(+), 19 deletions(-)

On Wed, Jan 12, 2022 at 10:05 AM Martin Liška <mliska@suse.cz> wrote:
>
> Hello.
>
> We've got -Wformat-diag for some time and I think we should start using it
> in -Werror for GCC bootstrap. The following patch removes last pieces of the warning
> for s390x target.
>
> Ready to be installed?
> Thanks,
> Martin
>
>
> gcc/ChangeLog:
>
>         * config/s390/s390-c.c (s390_expand_overloaded_builtin): Wrap
>         keyword in quotes.
>         (s390_resolve_overloaded_builtin): Remove trailing dot.
>         * config/s390/s390.c (s390_const_operand_ok): Use - for range.
>         (s390_expand_builtin): Remove trailing dot.
>         (s390_emit_prologue): Likewise, use semicolon.
>         (s390_option_override_internal): Update keyword.
>         * varasm.c (do_assemble_alias): Wrap keyword in quotes.
> ---
>   gcc/config/s390/s390-c.c |  9 +++++----
>   gcc/config/s390/s390.c   | 28 ++++++++++++++--------------
>   gcc/varasm.c             |  2 +-
>   3 files changed, 20 insertions(+), 19 deletions(-)
>
> diff --git a/gcc/config/s390/s390-c.c b/gcc/config/s390/s390-c.c
> index 600018421df..10bc6ac8900 100644
> --- a/gcc/config/s390/s390-c.c
> +++ b/gcc/config/s390/s390-c.c
> @@ -484,7 +484,8 @@ s390_expand_overloaded_builtin (location_t loc,
>       case S390_OVERLOADED_BUILTIN_s390_vec_step:
>         if (TREE_CODE (TREE_TYPE ((*arglist)[0])) != VECTOR_TYPE)
>         {
> -         error_at (loc, "builtin vec_step can only be used on vector types.");
> +         error_at (loc, "builtin %qs can only be used on vector types",
> +                   "vec_step ");

extra space in "vec_step "?

>           return error_mark_node;
>         }
>         return build_int_cst (NULL_TREE,
> @@ -905,7 +906,7 @@ s390_resolve_overloaded_builtin (location_t loc,
>         if (ob_flags & B_INT)
>         {
>           error_at (loc,
> -                   "builtin %qF is for GCC internal use only.",
> +                   "builtin %qF is for GCC internal use only",
>                     ob_fndecl);
>           return error_mark_node;
>         }
> @@ -913,7 +914,7 @@ s390_resolve_overloaded_builtin (location_t loc,
>       }
>
>     if (ob_flags & B_DEP)
> -    warning_at (loc, 0, "builtin %qF is deprecated.", ob_fndecl);
> +    warning_at (loc, 0, "builtin %qF is deprecated", ob_fndecl);
>
>     if (!TARGET_VX && (ob_flags & B_VX))
>       {
> @@ -1021,7 +1022,7 @@ s390_resolve_overloaded_builtin (location_t loc,
>       }
>
>     if (bflags_overloaded_builtin_var[last_match_index] & B_DEP)
> -    warning_at (loc, 0, "%qs matching variant is deprecated.",
> +    warning_at (loc, 0, "%qs matching variant is deprecated",
>                 IDENTIFIER_POINTER (DECL_NAME (ob_fndecl)));
>
>     /* Overloaded variants which have MAX set as low level builtin are
> diff --git a/gcc/config/s390/s390.c b/gcc/config/s390/s390.c
> index 056002e4a4a..bf96cbf7588 100644
> --- a/gcc/config/s390/s390.c
> +++ b/gcc/config/s390/s390.c
> @@ -766,7 +766,7 @@ s390_const_operand_ok (tree arg, int argnum, int op_flags, tree decl)
>                      argnum, decl, values);
>             }
>           else
> -           error ("constant argument %d for builtin %qF is out of range (0..%wu)",
> +           error ("constant argument %d for builtin %qF is out of range (0-%wu)",
>                    argnum, decl, (HOST_WIDE_INT_1U << bitwidth) - 1);
>
>           return false;
> @@ -783,7 +783,7 @@ s390_const_operand_ok (tree arg, int argnum, int op_flags, tree decl)
>           || tree_to_shwi (arg) > ((HOST_WIDE_INT_1 << (bitwidth - 1)) - 1))
>         {
>           error ("constant argument %d for builtin %qF is out of range "
> -                "(%wd..%wd)", argnum, decl,
> +                "(%wd-%wd)", argnum, decl,
>                  -(HOST_WIDE_INT_1 << (bitwidth - 1)),
>                  (HOST_WIDE_INT_1 << (bitwidth - 1)) - 1);
>           return false;
> @@ -832,25 +832,25 @@ s390_expand_builtin (tree exp, rtx target, rtx subtarget ATTRIBUTE_UNUSED,
>         if ((bflags & B_HTM) && !TARGET_HTM)
>         {
>           error ("builtin %qF is not supported without %<-mhtm%> "
> -                "(default with %<-march=zEC12%> and higher).", fndecl);
> +                "(default with %<-march=zEC12%> and higher)", fndecl);
>           return const0_rtx;
>         }
>         if (((bflags & B_VX) || (bflags & B_VXE)) && !TARGET_VX)
>         {
>           error ("builtin %qF requires %<-mvx%> "
> -                "(default with %<-march=z13%> and higher).", fndecl);
> +                "(default with %<-march=z13%> and higher)", fndecl);
>           return const0_rtx;
>         }
>
>         if ((bflags & B_VXE) && !TARGET_VXE)
>         {
> -         error ("Builtin %qF requires z14 or higher.", fndecl);
> +         error ("Builtin %qF requires z14 or higher", fndecl);
>           return const0_rtx;
>         }
>
>         if ((bflags & B_VXE2) && !TARGET_VXE2)
>         {
> -         error ("Builtin %qF requires z15 or higher.", fndecl);
> +         error ("Builtin %qF requires z15 or higher", fndecl);
>           return const0_rtx;
>         }
>
> @@ -11464,8 +11464,8 @@ s390_emit_prologue (void)
>             {
>               warning (0, "frame size of function %qs is %wd"
>                        " bytes exceeding user provided stack limit of "
> -                      "%d bytes.  "
> -                      "An unconditional trap is added.",
> +                      "%d bytes; "
> +                      "an unconditional trap is added",
>                        current_function_name(), cfun_frame_layout.frame_size,
>                        s390_stack_size);
>               emit_insn (gen_trap ());
> @@ -11479,9 +11479,9 @@ s390_emit_prologue (void)
>               if (stack_guard >= s390_stack_size)
>                 {
>                   warning (0, "frame size of function %qs is %wd"
> -                          " bytes which is more than half the stack size. "
> -                          "The dynamic check would not be reliable. "
> -                          "No check emitted for this function.",
> +                          " bytes which is more than half the stack size; "
> +                          "the dynamic check would not be reliable; "
> +                          "no check emitted for this function",
>                            current_function_name(),
>                            cfun_frame_layout.frame_size);
>                 }
> @@ -15557,11 +15557,11 @@ s390_option_override_internal (struct gcc_options *opts,
>         if (TARGET_HARD_DFP_P (opts_set->x_target_flags))
>         {
>           if (!TARGET_CPU_DFP_P (opts))
> -           error ("hardware decimal floating point instructions"
> +           error ("hardware decimal floating-point instructions"
>                    " not available on %s",
>                    processor_table[(int)opts->x_s390_arch].name);
>           if (!TARGET_ZARCH_P (opts->x_target_flags))
> -           error ("hardware decimal floating point instructions"
> +           error ("hardware decimal floating-point instructions"
>                    " not available in ESA/390 mode");
>         }
>         else
> @@ -15573,7 +15573,7 @@ s390_option_override_internal (struct gcc_options *opts,
>       {
>         if (TARGET_HARD_DFP_P (opts_set->x_target_flags)
>           && TARGET_HARD_DFP_P (opts->x_target_flags))
> -       error ("%<-mhard-dfp%> can%'t be used in conjunction with "
> +       error ("%<-mhard-dfp%> cannot be used in conjunction with "
>                "%<-msoft-float%>");
>
>         opts->x_target_flags &= ~MASK_HARD_DFP;
> diff --git a/gcc/varasm.c b/gcc/varasm.c
> index 76574be191f..0cbec5fd7fd 100644
> --- a/gcc/varasm.c
> +++ b/gcc/varasm.c
> @@ -6190,7 +6190,7 @@ do_assemble_alias (tree decl, tree target)
>         if (!TARGET_SUPPORTS_WEAK)
>         {
>           error_at (DECL_SOURCE_LOCATION (decl),
> -                   "weakref is not supported in this configuration");
> +                   "%qs is not supported in this configuration", "weakref ");
>           return;
>         }
>   #endif
> --
> 2.34.1
>

On 1/12/22 02:03, Martin Liška wrote:
> Hello.
> 
> We've got -Wformat-diag for some time and I think we should start using it
> in -Werror for GCC bootstrap. The following patch removes last pieces of 
> the warning
> for s390x target.
> 
> Ready to be installed?
> Thanks,
> Martin
> 
> 
> gcc/ChangeLog:
> 
>      * config/s390/s390-c.c (s390_expand_overloaded_builtin): Wrap
>      keyword in quotes.
>      (s390_resolve_overloaded_builtin): Remove trailing dot.
>      * config/s390/s390.c (s390_const_operand_ok): Use - for range.
>      (s390_expand_builtin): Remove trailing dot.
>      (s390_emit_prologue): Likewise, use semicolon.
>      (s390_option_override_internal): Update keyword.
>      * varasm.c (do_assemble_alias): Wrap keyword in quotes.
> ---
>   gcc/config/s390/s390-c.c |  9 +++++----
>   gcc/config/s390/s390.c   | 28 ++++++++++++++--------------
>   gcc/varasm.c             |  2 +-
>   3 files changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/gcc/config/s390/s390-c.c b/gcc/config/s390/s390-c.c
> index 600018421df..10bc6ac8900 100644
> --- a/gcc/config/s390/s390-c.c
> +++ b/gcc/config/s390/s390-c.c
> @@ -484,7 +484,8 @@ s390_expand_overloaded_builtin (location_t loc,
>       case S390_OVERLOADED_BUILTIN_s390_vec_step:
>         if (TREE_CODE (TREE_TYPE ((*arglist)[0])) != VECTOR_TYPE)
>       {
> -      error_at (loc, "builtin vec_step can only be used on vector 
> types.");
> +      error_at (loc, "builtin %qs can only be used on vector types",
> +            "vec_step ");

I'd have expected the warning to also trigger for the missing
hyphen in "builtin" (as per the GCC coding conventions) but it
looks like the code only looks for "builtin function".  I must
have done that because of the large number of misspellings.

Regardless, if the name of the function is __builtin_vec_step
(or __builtin_s390_vec_step?) then quoting the entire name of
the function as is conventionally done in the rest of GCC would
be one solution.  Rather than hardcoding the name as a string
there should be a way to do that by passing the right tree to
%qD (or %qF like below).  Based on my reading of the rest of
the file I wonder if this might be the decl:

s390_builtin_decls[bt_for_overloaded_builtin_var[S390_OVERLOADED_BUILTIN_s390_vec_step]]

Alternatively, if the preferred name to call the function with
is vec_step then simply vec_step without the "builtin" would
suffice.

(As an aside, there's a spurious space at the end of "vec_step "
above.)

>         return error_mark_node;
>       }
>         return build_int_cst (NULL_TREE,
> @@ -905,7 +906,7 @@ s390_resolve_overloaded_builtin (location_t loc,
>         if (ob_flags & B_INT)
>       {
>         error_at (loc,
> -            "builtin %qF is for GCC internal use only.",
> +            "builtin %qF is for GCC internal use only",
>               ob_fndecl);
>         return error_mark_node;
>       }
> @@ -913,7 +914,7 @@ s390_resolve_overloaded_builtin (location_t loc,
>       }
> 
>     if (ob_flags & B_DEP)
> -    warning_at (loc, 0, "builtin %qF is deprecated.", ob_fndecl);
> +    warning_at (loc, 0, "builtin %qF is deprecated", ob_fndecl);
> 
>     if (!TARGET_VX && (ob_flags & B_VX))
>       {
> @@ -1021,7 +1022,7 @@ s390_resolve_overloaded_builtin (location_t loc,
>       }
> 
>     if (bflags_overloaded_builtin_var[last_match_index] & B_DEP)
> -    warning_at (loc, 0, "%qs matching variant is deprecated.",
> +    warning_at (loc, 0, "%qs matching variant is deprecated",
>           IDENTIFIER_POINTER (DECL_NAME (ob_fndecl)));
> 
>     /* Overloaded variants which have MAX set as low level builtin are
> diff --git a/gcc/config/s390/s390.c b/gcc/config/s390/s390.c
> index 056002e4a4a..bf96cbf7588 100644
> --- a/gcc/config/s390/s390.c
> +++ b/gcc/config/s390/s390.c
> @@ -766,7 +766,7 @@ s390_const_operand_ok (tree arg, int argnum, int 
> op_flags, tree decl)
>                argnum, decl, values);
>           }
>         else
> -        error ("constant argument %d for builtin %qF is out of range 
> (0..%wu)",
> +        error ("constant argument %d for builtin %qF is out of range 
> (0-%wu)",
>              argnum, decl, (HOST_WIDE_INT_1U << bitwidth) - 1);
> 
>         return false;
> @@ -783,7 +783,7 @@ s390_const_operand_ok (tree arg, int argnum, int 
> op_flags, tree decl)
>         || tree_to_shwi (arg) > ((HOST_WIDE_INT_1 << (bitwidth - 1)) - 1))
>       {
>         error ("constant argument %d for builtin %qF is out of range "
> -         "(%wd..%wd)", argnum, decl,
> +         "(%wd-%wd)", argnum, decl,
>            -(HOST_WIDE_INT_1 << (bitwidth - 1)),
>            (HOST_WIDE_INT_1 << (bitwidth - 1)) - 1);
>         return false;
> @@ -832,25 +832,25 @@ s390_expand_builtin (tree exp, rtx target, rtx 
> subtarget ATTRIBUTE_UNUSED,
>         if ((bflags & B_HTM) && !TARGET_HTM)
>       {
>         error ("builtin %qF is not supported without %<-mhtm%> "
> -         "(default with %<-march=zEC12%> and higher).", fndecl);
> +         "(default with %<-march=zEC12%> and higher)", fndecl);
>         return const0_rtx;
>       }
>         if (((bflags & B_VX) || (bflags & B_VXE)) && !TARGET_VX)
>       {
>         error ("builtin %qF requires %<-mvx%> "
> -         "(default with %<-march=z13%> and higher).", fndecl);
> +         "(default with %<-march=z13%> and higher)", fndecl);
>         return const0_rtx;
>       }
> 
>         if ((bflags & B_VXE) && !TARGET_VXE)
>       {
> -      error ("Builtin %qF requires z14 or higher.", fndecl);
> +      error ("Builtin %qF requires z14 or higher", fndecl);

I'd expect the leading capital B to be flagged by the warning
after this change.  It's silent about leading capitals in full
sentences that end in a period (I think that's the GNU FORTRAN
style) but it complains about them otherwise.

Martin

>         return const0_rtx;
>       }
> 
>         if ((bflags & B_VXE2) && !TARGET_VXE2)
>       {
> -      error ("Builtin %qF requires z15 or higher.", fndecl);
> +      error ("Builtin %qF requires z15 or higher", fndecl);
>         return const0_rtx;
>       }
> 
> @@ -11464,8 +11464,8 @@ s390_emit_prologue (void)
>           {
>             warning (0, "frame size of function %qs is %wd"
>                  " bytes exceeding user provided stack limit of "
> -               "%d bytes.  "
> -               "An unconditional trap is added.",
> +               "%d bytes; "
> +               "an unconditional trap is added",
>                  current_function_name(), cfun_frame_layout.frame_size,
>                  s390_stack_size);
>             emit_insn (gen_trap ());
> @@ -11479,9 +11479,9 @@ s390_emit_prologue (void)
>             if (stack_guard >= s390_stack_size)
>           {
>             warning (0, "frame size of function %qs is %wd"
> -               " bytes which is more than half the stack size. "
> -               "The dynamic check would not be reliable. "
> -               "No check emitted for this function.",
> +               " bytes which is more than half the stack size; "
> +               "the dynamic check would not be reliable; "
> +               "no check emitted for this function",
>                  current_function_name(),
>                  cfun_frame_layout.frame_size);
>           }
> @@ -15557,11 +15557,11 @@ s390_option_override_internal (struct 
> gcc_options *opts,
>         if (TARGET_HARD_DFP_P (opts_set->x_target_flags))
>       {
>         if (!TARGET_CPU_DFP_P (opts))
> -        error ("hardware decimal floating point instructions"
> +        error ("hardware decimal floating-point instructions"
>              " not available on %s",
>              processor_table[(int)opts->x_s390_arch].name);
>         if (!TARGET_ZARCH_P (opts->x_target_flags))
> -        error ("hardware decimal floating point instructions"
> +        error ("hardware decimal floating-point instructions"
>              " not available in ESA/390 mode");
>       }
>         else
> @@ -15573,7 +15573,7 @@ s390_option_override_internal (struct 
> gcc_options *opts,
>       {
>         if (TARGET_HARD_DFP_P (opts_set->x_target_flags)
>         && TARGET_HARD_DFP_P (opts->x_target_flags))
> -    error ("%<-mhard-dfp%> can%'t be used in conjunction with "
> +    error ("%<-mhard-dfp%> cannot be used in conjunction with "
>              "%<-msoft-float%>");
> 
>         opts->x_target_flags &= ~MASK_HARD_DFP;
> diff --git a/gcc/varasm.c b/gcc/varasm.c
> index 76574be191f..0cbec5fd7fd 100644
> --- a/gcc/varasm.c
> +++ b/gcc/varasm.c
> @@ -6190,7 +6190,7 @@ do_assemble_alias (tree decl, tree target)
>         if (!TARGET_SUPPORTS_WEAK)
>       {
>         error_at (DECL_SOURCE_LOCATION (decl),
> -            "weakref is not supported in this configuration");
> +            "%qs is not supported in this configuration", "weakref ");
>         return;
>       }
>   #endif

On 1/12/22 10:03, Martin Liška wrote:
> Ready to be installed?

I'm going to install the patch as it's pre-approved by Jeff.

Martin