[committed] nios2: Fix format complaints and similar diagnostics

Message ID 87b9de50-88cd-e4ab-494e-262fd2ea283f@codesourcery.com
State New
Headers show
Series
  • [committed] nios2: Fix format complaints and similar diagnostics
Related show

Commit Message

Sandra Loosemore March 17, 2021, 10:01 p.m.
I've checked in this patch, after having received a report a few days 
ago that the nios2 back end was not building.  Most of the changes here 
are purely cosmetic.

Swapping out my nios2 maintainer hat for the documentation maintainer 
one, though: the warning insisting that "floating point" should be 
spelled "floating-point" is incorrect.  "Floating point" is correct as a 
noun and it's only appropriate to hyphenate the phrase when it's used as 
an adjective immediately before some other noun or noun phrase, like 
"floating-point processor".  However, removing that warning and trying 
to fix all the incorrect usages in the code seems like a can of worms, 
since there are already released versions of GCC that will complain 
about the correct spelling.  Since this bogus diagnostic isn't a 
regression and doesn't block builds, I'm not terribly enthusiastic about 
trying to fix it this late in stage 4 even if it's technically 
considered a documentation fix, so I've left that alone for now.

-Sandra

Comments

Richard Biener via Gcc-patches March 17, 2021, 10:40 p.m. | #1
On 3/17/21 4:01 PM, Sandra Loosemore wrote:
> I've checked in this patch, after having received a report a few days 

> ago that the nios2 back end was not building.  Most of the changes here 

> are purely cosmetic.

> 

> Swapping out my nios2 maintainer hat for the documentation maintainer 

> one, though: the warning insisting that "floating point" should be 

> spelled "floating-point" is incorrect.  "Floating point" is correct as a 

> noun and it's only appropriate to hyphenate the phrase when it's used as 

> an adjective immediately before some other noun or noun phrase, like 

> "floating-point processor".  However, removing that warning and trying 

> to fix all the incorrect usages in the code seems like a can of worms, 

> since there are already released versions of GCC that will complain 

> about the correct spelling.  Since this bogus diagnostic isn't a 

> regression and doesn't block builds, I'm not terribly enthusiastic about 

> trying to fix it this late in stage 4 even if it's technically 

> considered a documentation fix, so I've left that alone for now.


I probably missed the (adjective) part in the Spelling table in
the coding conventions.  It should be easy to adjust the warning
to keep quiet when "floating point" is the end of a sentence.
That would suppress it in the five or so instances where it's wrong.
Alternatively, to avoid the warning with older GCC(*) we could change
these five messages and add some text after the "floating point" part
to make it an adjective.

That said, I count 52 instances in gcc.pot where it looks appropriate.

Martin

[*] Does -Wformat-diag really trigger when using older GCC to build?
I thought it only triggered in stage 2 and 3 when using the same GCC
to rebuild itself.

> 

> -Sandra
Sandra Loosemore March 18, 2021, midnight | #2
On 3/17/21 4:40 PM, Martin Sebor wrote:

> [*] Does -Wformat-diag really trigger when using older GCC to build?

> I thought it only triggered in stage 2 and 3 when using the same GCC

> to rebuild itself.


I always end up hopelessly confused by anything involving configure 
scripts and makefiles, but....

Our scripts normally configure cross-compilers with 
--disable-build-format-warnings and there's a comment on that bit that 
these diagnostics are normally enabled when building non-stage-1 
compilers.  A cross compiler doesn't bootstrap so is never stage 1, I 
guess.  For the purposes of testing this patch I did an explicit 
--enable-build-format-warnings instead, anyway.

Jan-Benedict, who reported the problem to me off-list, said he just did

configure --target=nios2-linux-gnu

and

make all-gcc

using the host gcc including with his distro, identified as "Debian 
10.2.1-6".

-Sandra
Richard Biener via Gcc-patches March 21, 2021, 9:34 p.m. | #3
On 3/17/21 6:00 PM, Sandra Loosemore wrote:
> On 3/17/21 4:40 PM, Martin Sebor wrote:

> 

>> [*] Does -Wformat-diag really trigger when using older GCC to build?

>> I thought it only triggered in stage 2 and 3 when using the same GCC

>> to rebuild itself.

> 

> I always end up hopelessly confused by anything involving configure 

> scripts and makefiles, but....

> 

> Our scripts normally configure cross-compilers with 

> --disable-build-format-warnings and there's a comment on that bit that 

> these diagnostics are normally enabled when building non-stage-1 

> compilers.  A cross compiler doesn't bootstrap so is never stage 1, I 

> guess.  For the purposes of testing this patch I did an explicit 

> --enable-build-format-warnings instead, anyway.

> 

> Jan-Benedict, who reported the problem to me off-list, said he just did

> 

> configure --target=nios2-linux-gnu

> 

> and

> 

> make all-gcc

> 

> using the host gcc including with his distro, identified as "Debian 

> 10.2.1-6".



I see.  I routinely notice -Wformat warnings when building
cross-compilers (my system compiler is GCC 8.3) so I've been
assuming they're to be expected.  For instance this one:

/src/gcc/master/gcc/gimple-fold.c: In function ‘bool 
gimple_fold_builtin_strncpy(gimple_stmt_iterator*, tree, tree, tree)’:
/src/gcc/master/gcc/gimple-fold.c:1921:4: warning: format ‘%G’ expects 
argument of type ‘gcall*’, but argument 4 has type ‘gimple*’ [-Wformat=]
     "%G%qD destination unchanged after copying no bytes "
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     "from a string of length %E",
     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     stmt, fndecl, slen);
     ~~~~

Since, as you say, cross-compilers don't build stages 2 and later,
disabling these warnings for their builds would seem like the only
solution.  Either that, or building them with the current GCC.

Either way, I'll try to remember to relax the warning for GCC 12
as I mentioned.

Martin

Patch

commit 5074c6fa38cef1abb9a355d717b41441a44c4e6a
Author: Sandra Loosemore <sandra@codesourcery.com>
Date:   Wed Mar 17 14:37:05 2021 -0700

    nios2: Fix format complaints and similar diagnostics.
    
    The nios2 back end has not been building with newer versions of host
    GCC due to several complaints about diagnostic formatting, along with
    a couple other warnings.  This patch fixes the errors seen when
    building with a host compiler from current mainline head.  I also made
    a pass through all the error messages in this file to make them use
    more consistent formatting, even where the host compiler was not
    specifically complaining.
    
    	gcc/
    	* config/nios2/nios2.c (nios2_custom_check_insns): Clean up
    	error message format issues.
    	(nios2_option_override): Likewise.
    	(nios2_expand_fpu_builtin): Likewise.
    	(nios2_init_custom_builtins): Adjust to avoid bogus strncpy
    	truncation warning.
    	(nios2_expand_custom_builtin): More error message format fixes.
    	(nios2_expand_rdwrctl_builtin): Likewise.
    	(nios2_expand_rdprs_builtin): Likewise.
    	(nios2_expand_eni_builtin): Likewise.
    	(nios2_expand_builtin): Likewise.
    	(nios2_register_custom_code): Likewise.
    	(nios2_valid_target_attribute_rec): Likewise.
    	(nios2_add_insn_asm): Fix uninitialized variable warning.

diff --git a/gcc/config/nios2/nios2.c b/gcc/config/nios2/nios2.c
index 3ff4ff1..bf5e2be 100644
--- a/gcc/config/nios2/nios2.c
+++ b/gcc/config/nios2/nios2.c
@@ -1179,8 +1179,8 @@  nios2_custom_check_insns (void)
 	for (j = 0; j < ARRAY_SIZE (nios2_fpu_insn); j++)
 	  if (N2FPU_DOUBLE_REQUIRED_P (j) && ! N2FPU_ENABLED_P (j))
 	    {
-	      error ("switch %<-mcustom-%s%> is required for double "
-		     "precision floating point", N2FPU_NAME (j));
+	      error ("switch %<-mcustom-%s%> is required for "
+		     "double-precision floating-point", N2FPU_NAME (j));
 	      errors = true;
 	    }
 	break;
@@ -1188,7 +1188,8 @@  nios2_custom_check_insns (void)
 
   if (errors || custom_code_conflict)
     fatal_error (input_location,
-		 "conflicting use of %<-mcustom%> switches, target attributes, "
+		 "conflicting use of %<-mcustom%> switches, "
+		 "target attributes, "
 		 "and/or %<__builtin_custom_%> functions");
 }
 
@@ -1378,11 +1379,11 @@  nios2_option_override (void)
   if (flag_pic)
     {
       if (nios2_gpopt_option != gpopt_none)
-	error ("%<-mgpopt%> not supported with PIC.");
+	error ("%<-mgpopt%> not supported with PIC");
       if (nios2_gprel_sec)
-	error ("%<-mgprel-sec=%> not supported with PIC.");
+	error ("%<-mgprel-sec=%> not supported with PIC");
       if (nios2_r0rel_sec)
-	error ("%<-mr0rel-sec=%> not supported with PIC.");
+	error ("%<-mr0rel-sec=%> not supported with PIC");
     }
 
   /* Process -mgprel-sec= and -m0rel-sec=.  */
@@ -1390,13 +1391,13 @@  nios2_option_override (void)
     {
       if (regcomp (&nios2_gprel_sec_regex, nios2_gprel_sec, 
 		   REG_EXTENDED | REG_NOSUB))
-	error ("%<-mgprel-sec=%> argument is not a valid regular expression.");
+	error ("%<-mgprel-sec=%> argument is not a valid regular expression");
     }
   if (nios2_r0rel_sec)
     {
       if (regcomp (&nios2_r0rel_sec_regex, nios2_r0rel_sec, 
 		   REG_EXTENDED | REG_NOSUB))
-	error ("%<-mr0rel-sec=%> argument is not a valid regular expression.");
+	error ("%<-mr0rel-sec=%> argument is not a valid regular expression");
     }
 
   /* If we don't have mul, we don't have mulx either!  */
@@ -3574,8 +3575,9 @@  nios2_expand_fpu_builtin (tree exp, unsigned int code, rtx target)
 
   if (N2FPU_N (code) < 0)
     fatal_error (input_location,
-		 "Cannot call %<__builtin_custom_%s%> without specifying switch"
-		 " %<-mcustom-%s%>", N2FPU_NAME (code), N2FPU_NAME (code));
+		 "cannot call %<__builtin_custom_%s%> without specifying "
+		 "switch %<-mcustom-%s%>",
+		 N2FPU_NAME (code), N2FPU_NAME (code));
   if (has_target_p)
     create_output_operand (&ops[opno++], target, dst_mode);
   else
@@ -3641,10 +3643,10 @@  nios2_init_custom_builtins (int start_code)
 	    = build_function_type_list (ret_type, integer_type_node,
 					op[rhs1].type, op[rhs2].type,
 					NULL_TREE);
-	  snprintf (builtin_name + n, 32 - n, "%sn%s%s",
-		    op[lhs].c, op[rhs1].c, op[rhs2].c);
 	  /* Save copy of parameter string into custom_builtin_name[].  */
-	  strncpy (custom_builtin_name[builtin_code], builtin_name + n, 5);
+	  snprintf (custom_builtin_name[builtin_code], 5, "%sn%s%s",
+		    op[lhs].c, op[rhs1].c, op[rhs2].c);
+	  strncpy (builtin_name + n, custom_builtin_name[builtin_code], 5);
 	  fndecl =
 	    add_builtin_function (builtin_name, builtin_ftype,
 				  start_code + builtin_code,
@@ -3682,7 +3684,7 @@  nios2_expand_custom_builtin (tree exp, unsigned int index, rtx target)
       if (argno == 0)
 	{
 	  if (!custom_insn_opcode (value, VOIDmode))
-	    error ("custom instruction opcode must be compile time "
+	    error ("custom instruction opcode must be a compile-time "
 		   "constant in the range 0-255 for %<__builtin_custom_%s%>",
 		   custom_builtin_name[index]);
 	}
@@ -3887,7 +3889,7 @@  nios2_expand_rdwrctl_builtin (tree exp, rtx target,
   struct expand_operand ops[MAX_RECOG_OPERANDS];
   if (!rdwrctl_operand (ctlcode, VOIDmode))
     {
-      error ("Control register number must be in range 0-31 for %s",
+      error ("control register number must be in range 0-31 for %s",
 	     d->name);
       return has_target_p ? gen_reg_rtx (SImode) : const0_rtx;
     }
@@ -3915,14 +3917,14 @@  nios2_expand_rdprs_builtin (tree exp, rtx target,
 
   if (!rdwrctl_operand (reg, VOIDmode))
     {
-      error ("Register number must be in range 0-31 for %s",
+      error ("register number must be in range 0-31 for %s",
 	     d->name);
       return gen_reg_rtx (SImode);
     }
 
   if (!rdprs_dcache_operand (imm, VOIDmode))
     {
-      error ("The immediate value must fit into a %d-bit integer for %s",
+      error ("immediate value must fit into a %d-bit integer for %s",
 	     (TARGET_ARCH_R2) ? 12 : 16, d->name);
       return gen_reg_rtx (SImode);
     }
@@ -3972,7 +3974,7 @@  nios2_expand_eni_builtin (tree exp, rtx target ATTRIBUTE_UNUSED,
 
   if (INTVAL (imm) != 0 && INTVAL (imm) != 1)
     {
-      error ("The ENI instruction operand must be either 0 or 1");
+      error ("the ENI instruction operand must be either 0 or 1");
       return const0_rtx;      
     }
   create_integer_operand (&ops[0], INTVAL (imm));
@@ -4000,7 +4002,7 @@  nios2_expand_builtin (tree exp, rtx target, rtx subtarget ATTRIBUTE_UNUSED,
 
       if (d->arch > nios2_arch_option)
 	{
-	  error ("Builtin function %s requires Nios II R%d",
+	  error ("built-in function %s requires Nios II R%d",
 		 d->name, (int) d->arch);
 	  /* Given it is invalid, just generate a normal call.  */
 	  return expand_call (exp, target, ignore);
@@ -4080,14 +4082,16 @@  nios2_register_custom_code (unsigned int N, enum nios2_ccs_code status,
       if (custom_code_status[N] == CCS_FPU && index != custom_code_index[N])
 	{
 	  custom_code_conflict = true;
-	  error ("switch %<-mcustom-%s%> conflicts with switch %<-mcustom-%s%>",
+	  error ("switch %<-mcustom-%s%> conflicts with "
+		 "switch %<-mcustom-%s%>",
 		 N2FPU_NAME (custom_code_index[N]), N2FPU_NAME (index));
 	}
       else if (custom_code_status[N] == CCS_BUILTIN_CALL)
 	{
 	  custom_code_conflict = true;
-	  error ("call to %<__builtin_custom_%s%> conflicts with switch "
-		 "%<-mcustom-%s%>", custom_builtin_name[custom_code_index[N]],
+	  error ("call to %<__builtin_custom_%s%> conflicts with "
+		 "switch %<-mcustom-%s%>",
+		 custom_builtin_name[custom_code_index[N]],
 		 N2FPU_NAME (index));
 	}
     }
@@ -4096,8 +4100,9 @@  nios2_register_custom_code (unsigned int N, enum nios2_ccs_code status,
       if (custom_code_status[N] == CCS_FPU)
 	{
 	  custom_code_conflict = true;
-	  error ("call to %<__builtin_custom_%s%> conflicts with switch "
-		 "%<-mcustom-%s%>", custom_builtin_name[index],
+	  error ("call to %<__builtin_custom_%s%> conflicts with "
+		 "switch %<-mcustom-%s%>",
+		 custom_builtin_name[index],
 		 N2FPU_NAME (custom_code_index[N]));
 	}
       else
@@ -4204,13 +4209,13 @@  nios2_valid_target_attribute_rec (tree args)
 	      char *end_eq = p;
 	      if (no_opt)
 		{
-		  error ("custom-fpu-cfg option does not support %<no-%>");
+		  error ("%<custom-fpu-cfg%> option does not support %<no-%>");
 		  return false;
 		}
 	      if (!eq)
 		{
-		  error ("custom-fpu-cfg option requires configuration"
-			 " argument");
+		  error ("%<custom-fpu-cfg%> option requires configuration "
+			 "argument");
 		  return false;
 		}
 	      /* Increment and skip whitespace.  */
@@ -4282,7 +4287,7 @@  nios2_valid_target_attribute_rec (tree args)
 	    }
 	  else
 	    {
-	      error ("%<%s%> is unknown", argstr);
+	      error ("invalid custom instruction option %qs", argstr);
 	      return false;
 	    }
 
@@ -4707,7 +4712,7 @@  nios2_add_insn_asm (rtx_insn *insn, rtx *operands)
 bool
 nios2_cdx_narrow_form_p (rtx_insn *insn)
 {
-  rtx pat, lhs, rhs1, rhs2;
+  rtx pat, lhs, rhs1 = NULL_RTX, rhs2 = NULL_RTX;
   enum attr_type type;
   if (!TARGET_HAS_CDX)
     return false;