bug#32189: 27.0.50; GCC 7 warning due to -Wformat-truncation=2

bug#32189: 27.0.50; GCC 7 warning due to -Wformat-truncation=2

[Ken Brown]

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

I'm getting the following warning when building the master branch on 
Cygwin with GCC 7.3:

In file included from /usr/include/stdio.h:800:0,
                  from ../lib/stdio.h:43,
                  from ../../master/src/w32cygwinx.c:22:
../../master/src/w32cygwinx.c: In function ‘Fw32_battery_status’:
../../master/src/w32cygwinx.c:116:26: warning: ‘%3.1f’ directive output 
may be truncated writing between 3 and 312 bytes into a region of size 
16 [-Wformat-truncation=]
     snprintf (buffer, 16, "%3.1f", h);
                           ^
../../master/src/w32cygwinx.c:116:4: note: ‘__builtin_snprintf’ output 
between 4 and 313 bytes into a destination of size 16
     snprintf (buffer, 16, "%3.1f", h);
     ^

The attached patch avoids the warning.  Is this a reasonable fix, or is
there a better way?


In GNU Emacs 27.0.50 (build 6, x86_64-unknown-cygwin, GTK+ Version 3.22.28)
  of 2018-07-17 built on moufang
Repository revision: 04599bb1b219b236356ba3393a23e1c1dd8c541b
Windowing system distributor 'The Cygwin/X Project', version 11.0.12000000


[-- Attachment #2: 0001-Pacify-GCC-7-with-Wformat-truncation-2.patch --]
[-- Type: text/plain, Size: 1231 bytes --]

From f79ad94f8dcca82aa1523ef89ff88aab1b2d3fe4 Mon Sep 17 00:00:00 2001
From: Ken Brown <kbrown@cornell.edu>
Date: Tue, 17 Jul 2018 15:12:35 -0400
Subject: [PATCH] Pacify GCC 7 with -Wformat-truncation=2

* src/w32cygwinx.c (Fw32_battery_status): Increase size of
'buffer'.  (Bug#xxxxx)
---
 src/w32cygwinx.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/w32cygwinx.c b/src/w32cygwinx.c
index 8d3ae164cf..f7bbf17a16 100644
--- a/src/w32cygwinx.c
+++ b/src/w32cygwinx.c
@@ -104,19 +104,19 @@ The following %-sequences are provided:
 	{
 	  long m;
 	  double h;
-	  char buffer[16];
-	  snprintf (buffer, 16, "%ld", seconds_left);
+	  char buffer[400];
+	  snprintf (buffer, 400, "%ld", seconds_left);
 	  seconds = build_string (buffer);
 
 	  m = seconds_left / 60;
-	  snprintf (buffer, 16, "%ld", m);
+	  snprintf (buffer, 400, "%ld", m);
 	  minutes = build_string (buffer);
 
 	  h = seconds_left / 3600.0;
-	  snprintf (buffer, 16, "%3.1f", h);
+	  snprintf (buffer, 400, "%3.1f", h);
 	  hours = build_string (buffer);
 
-	  snprintf (buffer, 16, "%ld:%02ld", m / 60, m % 60);
+	  snprintf (buffer, 400, "%ld:%02ld", m / 60, m % 60);
 	  remain = build_string (buffer);
 	}
 
-- 
2.17.0

bug#32189: 27.0.50; GCC 7 warning due to -Wformat-truncation=2

[Eli Zaretskii]

> From: Ken Brown <kbrown@cornell.edu>
> Date: Tue, 17 Jul 2018 15:26:34 -0400
> 
> I'm getting the following warning when building the master branch on 
> Cygwin with GCC 7.3:
> 
> In file included from /usr/include/stdio.h:800:0,
>                   from ../lib/stdio.h:43,
>                   from ../../master/src/w32cygwinx.c:22:
> ../../master/src/w32cygwinx.c: In function ‘Fw32_battery_status’:
> ../../master/src/w32cygwinx.c:116:26: warning: ‘%3.1f’ directive output 
> may be truncated writing between 3 and 312 bytes into a region of size 
> 16 [-Wformat-truncation=]
>      snprintf (buffer, 16, "%3.1f", h);
>                            ^
> ../../master/src/w32cygwinx.c:116:4: note: ‘__builtin_snprintf’ output 
> between 4 and 313 bytes into a destination of size 16
>      snprintf (buffer, 16, "%3.1f", h);
>      ^

Do we really need to use -Wformat-truncation?  Is it a useful warning
switch?  The above sounds like useless noise, because the code
explicitly _asks_ for truncation.  What do people think about this?

> The attached patch avoids the warning.  Is this a reasonable fix, or is
> there a better way?

I think if we keep the switch, a better fix is to do this:

     snprintf (buffer, 16, "%ld", h % 1000000);

     m = seconds_left / 60;
     snprintf (buffer, 16, "%ld", m % 20000);

etc., you get the point.

Thanks.

bug#32189: 27.0.50; GCC 7 warning due to -Wformat-truncation=2

[Ken Brown]

On 7/18/2018 11:09 AM, Eli Zaretskii wrote:
>> From: Ken Brown <kbrown@cornell.edu>
>> Date: Tue, 17 Jul 2018 15:26:34 -0400
>>
>> I'm getting the following warning when building the master branch on
>> Cygwin with GCC 7.3:
>>
>> In file included from /usr/include/stdio.h:800:0,
>>                    from ../lib/stdio.h:43,
>>                    from ../../master/src/w32cygwinx.c:22:
>> ../../master/src/w32cygwinx.c: In function ‘Fw32_battery_status’:
>> ../../master/src/w32cygwinx.c:116:26: warning: ‘%3.1f’ directive output
>> may be truncated writing between 3 and 312 bytes into a region of size
>> 16 [-Wformat-truncation=]
>>       snprintf (buffer, 16, "%3.1f", h);
>>                             ^
>> ../../master/src/w32cygwinx.c:116:4: note: ‘__builtin_snprintf’ output
>> between 4 and 313 bytes into a destination of size 16
>>       snprintf (buffer, 16, "%3.1f", h);
>>       ^
> 
> Do we really need to use -Wformat-truncation?  Is it a useful warning
> switch?  The above sounds like useless noise, because the code
> explicitly _asks_ for truncation.  What do people think about this?

Moreover, the warning isn't very smart; see below.

>> The attached patch avoids the warning.  Is this a reasonable fix, or is
>> there a better way?
> 
> I think if we keep the switch, a better fix is to do this:
> 
>       snprintf (buffer, 16, "%ld", h % 1000000);
> 
>       m = seconds_left / 60;
>       snprintf (buffer, 16, "%ld", m % 20000);
> 
> etc., you get the point.

This doesn't work with GCC 7.  (Maybe it would work with GCC 8; the 
release notes say that it is better at avoiding false positives.)  For 
integer specifiers like "%ld", the only thing I've found that works 
without enlarging the buffer is to cast the argument to a smaller 
integer type.  For float specifiers like "%3.1f", even using a small 
type doesn't seem to work.  For example:

$ cat test.c
#include <stdio.h>
int
main ()
{
   char buffer[16];
   short a;
   snprintf (buffer, 16, "%3.1f", a);
}

$ gcc -Wformat-truncation=2 test.c
test.c: In function ‘main’:
test.c:8:26: warning: ‘%3.1f’ directive output may be truncated writing 
between 3 and 312 bytes into a region of size 16 [-Wformat-truncation=]
    snprintf (buffer, 16, "%3.1f", a);
                           ^~~~~
test.c:8:3: note: ‘snprintf’ output between 4 and 313 bytes into a 
destination of size 16
    snprintf (buffer, 16, "%3.1f", a);
    ^~~~~~~~~~~~~~~~~~~~~

Ken

bug#32189: 27.0.50; GCC 7 warning due to -Wformat-truncation=2

[Eli Zaretskii]

> Cc: 32189@debbugs.gnu.org
> From: Ken Brown <kbrown@cornell.edu>
> Date: Wed, 18 Jul 2018 15:42:55 -0400
> 
> On 7/18/2018 11:09 AM, Eli Zaretskii wrote:
> > Do we really need to use -Wformat-truncation?  Is it a useful warning
> > switch?  The above sounds like useless noise, because the code
> > explicitly _asks_ for truncation.  What do people think about this?
> 
> Moreover, the warning isn't very smart; see below.

My vote is for removing this warning until it becomes smarter.  Any
objections, anyone?

bug#32189: 27.0.50; GCC 7 warning due to -Wformat-truncation=2

[Philipp Stephani]

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

Ken Brown <kbrown@cornell.edu> schrieb am Mi., 18. Juli 2018 um 21:44 Uhr:

>
>
> This doesn't work with GCC 7.  (Maybe it would work with GCC 8; the
> release notes say that it is better at avoiding false positives.)  For
> integer specifiers like "%ld", the only thing I've found that works
> without enlarging the buffer is to cast the argument to a smaller
> integer type.


You can't do that; %ld requires a long argument, and casting results in
undefined behavior.


> For float specifiers like "%3.1f", even using a small
> type doesn't seem to work.  For example:
>
> $ cat test.c
> #include <stdio.h>
> int
> main ()
> {
>    char buffer[16];
>    short a;
>    snprintf (buffer, 16, "%3.1f", a);
> }
>

This is undefined behavior, as %f requires a double argument.

[-- Attachment #2: Type: text/html, Size: 1318 bytes --]

bug#32189: 27.0.50; GCC 7 warning due to -Wformat-truncation=2

[Ken Brown]

On 7/19/2018 2:10 AM, Philipp Stephani wrote:
> Ken Brown <kbrown@cornell.edu <mailto:kbrown@cornell.edu>> schrieb am 
> Mi., 18. Juli 2018 um 21:44 Uhr:

>     $ cat test.c
>     #include <stdio.h>
>     int
>     main ()
>     {
>         char buffer[16];
>         short a;
>         snprintf (buffer, 16, "%3.1f", a);
>     }
> 
> 
> This is undefined behavior, as %f requires a double argument.

Then cast the argument to double; this doesn't change the warning, which 
claims that the output could be 312 bytes.

Ken

bug#32189: 27.0.50; GCC 7 warning due to -Wformat-truncation=2

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Thu, 19 Jul 2018 08:10:20 +0200
> Cc: Eli Zaretskii <eliz@gnu.org>, 32189@debbugs.gnu.org
> 
>  This doesn't work with GCC 7.  (Maybe it would work with GCC 8; the 
>  release notes say that it is better at avoiding false positives.)  For 
>  integer specifiers like "%ld", the only thing I've found that works 
>  without enlarging the buffer is to cast the argument to a smaller 
>  integer type. 
> 
> You can't do that; %ld requires a long argument, and casting results in undefined behavior.

We can assign instead, and since the value is known to be small
enough, doing that won't lose bits.

bug#32189: 27.0.50; GCC 7 warning due to -Wformat-truncation=2

[Philipp Stephani]

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

Eli Zaretskii <eliz@gnu.org> schrieb am Mi., 18. Juli 2018 um 17:10 Uhr:

> > From: Ken Brown <kbrown@cornell.edu>
> > Date: Tue, 17 Jul 2018 15:26:34 -0400
> >
> > I'm getting the following warning when building the master branch on
> > Cygwin with GCC 7.3:
> >
> > In file included from /usr/include/stdio.h:800:0,
> >                   from ../lib/stdio.h:43,
> >                   from ../../master/src/w32cygwinx.c:22:
> > ../../master/src/w32cygwinx.c: In function ‘Fw32_battery_status’:
> > ../../master/src/w32cygwinx.c:116:26: warning: ‘%3.1f’ directive output
> > may be truncated writing between 3 and 312 bytes into a region of size
> > 16 [-Wformat-truncation=]
> >      snprintf (buffer, 16, "%3.1f", h);
> >                            ^
> > ../../master/src/w32cygwinx.c:116:4: note: ‘__builtin_snprintf’ output
> > between 4 and 313 bytes into a destination of size 16
> >      snprintf (buffer, 16, "%3.1f", h);
> >      ^
>
> Do we really need to use -Wformat-truncation?  Is it a useful warning
> switch?  The above sounds like useless noise, because the code
> explicitly _asks_ for truncation.  What do people think about this?
>

The typical use case for snprintf is to make sure that the output never
gets truncated, so disabling -Wformat-truncation would be harmful in most
cases.
With truncation, the code will result in nonsensical output if the input
value was too large, e.g. 123.4 hours would result in "12" if the buffer
size were 3.
Rather than relying on truncation, the code should probably just use a
buffer that's large enough, or use %g instead of %f.

[-- Attachment #2: Type: text/html, Size: 2164 bytes --]

bug#32189: 27.0.50; GCC 7 warning due to -Wformat-truncation=2

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Thu, 19 Jul 2018 08:21:54 +0200
> Cc: Ken Brown <kbrown@cornell.edu>, 32189@debbugs.gnu.org
> 
>  Do we really need to use -Wformat-truncation?  Is it a useful warning
>  switch?  The above sounds like useless noise, because the code
>  explicitly _asks_ for truncation.  What do people think about this?
> 
> The typical use case for snprintf is to make sure that the output never gets truncated, so disabling
> -Wformat-truncation would be harmful in most cases.

That might be typical for your uses, but it doesn't mean it is
universally so.  It definitely isn't true for my uses: I use snprintf
to be sure a fixed buffer won't be overrun due to some programming
mistake.  And that is exactly what is going on in that function,
because the maximum expected values of the variables are known in
advance, and the buffer is large enough to accommodate them.

> With truncation, the code will result in nonsensical output if the input value was too large, e.g. 123.4 hours
> would result in "12" if the buffer size were 3.

Since truncation should never happen, except when there's some bug,
this is not really interesting.

> Rather than relying on truncation, the code should probably just use a buffer that's large enough, or use %g
> instead of %f. 

If we need to jump through hoops to stop useless noise about perfectly
valid code, we shouldn't have this warning in the first place.

bug#32189: 27.0.50; GCC 7 warning due to -Wformat-truncation=2

[Ken Brown]

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

On 7/19/2018 9:29 AM, Eli Zaretskii wrote:
> If we need to jump through hoops to stop useless noise about perfectly
> valid code, we shouldn't have this warning in the first place.

The attached patch suppresses the warning.

Ken

[-- Attachment #2: 0001-Avoid-warnings-with-GCC-7.patch --]
[-- Type: text/plain, Size: 940 bytes --]

From e5efe43d9fa38ef10d88a4eaf88c5e7b8be9d5c8 Mon Sep 17 00:00:00 2001
From: Ken Brown <kbrown@cornell.edu>
Date: Thu, 19 Jul 2018 09:52:19 -0400
Subject: [PATCH] Avoid warnings with GCC 7

* configure.ac (nw): Suppress the -Wformat-truncation=2 option
due to too many false alarms with GCC 7.  (Bug#32189)
---
 configure.ac | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configure.ac b/configure.ac
index b6918671e4..202c5621b1 100644
--- a/configure.ac
+++ b/configure.ac
@@ -954,6 +954,7 @@ AC_DEFUN
 
   nw="$nw -Wcast-align -Wcast-align=strict" # Emacs is tricky with pointers.
   nw="$nw -Wduplicated-branches"    # Too many false alarms
+  nw="$nw -Wformat-truncation=2"    # Too many false alarms
   nw="$nw -Wformat-overflow=2"      # False alarms due to GCC bug 80776
   nw="$nw -Wsystem-headers"         # Don't let system headers trigger warnings
   nw="$nw -Woverlength-strings"     # Not a problem these days
-- 
2.17.0

bug#32189: 27.0.50; GCC 7 warning due to -Wformat-truncation=2

[Eli Zaretskii]

> Cc: 32189@debbugs.gnu.org
> From: Ken Brown <kbrown@cornell.edu>
> Date: Thu, 19 Jul 2018 09:56:43 -0400
> 
> The attached patch suppresses the warning.
> 
> From e5efe43d9fa38ef10d88a4eaf88c5e7b8be9d5c8 Mon Sep 17 00:00:00 2001
> From: Ken Brown <kbrown@cornell.edu>
> Date: Thu, 19 Jul 2018 09:52:19 -0400
> Subject: [PATCH] Avoid warnings with GCC 7
> 
> * configure.ac (nw): Suppress the -Wformat-truncation=2 option
> due to too many false alarms with GCC 7.  (Bug#32189)
> ---
>  configure.ac | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/configure.ac b/configure.ac
> index b6918671e4..202c5621b1 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -954,6 +954,7 @@ AC_DEFUN
>  
>    nw="$nw -Wcast-align -Wcast-align=strict" # Emacs is tricky with pointers.
>    nw="$nw -Wduplicated-branches"    # Too many false alarms
> +  nw="$nw -Wformat-truncation=2"    # Too many false alarms
>    nw="$nw -Wformat-overflow=2"      # False alarms due to GCC bug 80776
>    nw="$nw -Wsystem-headers"         # Don't let system headers trigger warnings
>    nw="$nw -Woverlength-strings"     # Not a problem these days

Thanks, this LGTM, but I'd like to hear Paul's opinion about this
warning before we push (since this should probably go to the emacs-26
branch?).

bug#32189: 27.0.50; GCC 7 warning due to -Wformat-truncation=2

[Paul Eggert]

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

Eli Zaretskii wrote:
> Thanks, this LGTM, but I'd like to hear Paul's opinion about this
> warning before we push (since this should probably go to the emacs-26
> branch?).

Let's continue using -Wformat-truncation=2 since I recall it finding real bugs 
in Emacs in the past, and there's a better fix for this particular problem.

Some background. Generally I don't worry too much about warnings from older 
compilers, since one can just compile with --disable-gcc-warnings if the 
compiler is too old. It's better to use the latest GCC to find bugs, and not 
worry about pacifying older GCCs that are somewhat flaky in this area (as 
pacifying them would be a neverending project with little benefit).

Two comments for this particular case. First, Emacs generally prefers sprintf to 
snprintf, because truncation (which is what the latter does) is often just as 
serious a bug as buffer overflow (which is what the former does), and the GNU 
coding style is to avoid both bugs in which case sprintf is generally simpler 
and easier to use. I realize this goes against the common wisdom that snprintf 
is "safer" than sprintf, but the common wisdom is typically wrong for 
high-quality code. In the few places where Emacs does use snprintf, it either 
checks for truncated output and reports an error if so (which is lame, but at 
least there's an error check), or truncation is expected and is OK.

Second, the code in question uses snprintf followed by build_string, and can be 
simplified by using vformat_string instead. That would avoid the warnings and 
should make the code more reliable. Something like the attached patch, say. I 
haven't tested it since I don't use MS-Windows. Let's use something like this 
rather than shutting off the warnings.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Simplify-w32cygwinx.c-and-pacify-GCC-Bug-32189.patch --]
[-- Type: text/x-patch; name="0001-Simplify-w32cygwinx.c-and-pacify-GCC-Bug-32189.patch", Size: 2322 bytes --]

From 95ed210f17a7970764fb670e8b5d41b3d2ff3084 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 19 Jul 2018 16:12:17 -0700
Subject: [PATCH] Simplify w32cygwinx.c and pacify GCC (Bug#32189)

* src/w32cygwinx.c (format_string): New function.
(Fw32_battery_status): Use it.
---
 src/w32cygwinx.c | 37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/src/w32cygwinx.c b/src/w32cygwinx.c
index 8d3ae16..044e1fc 100644
--- a/src/w32cygwinx.c
+++ b/src/w32cygwinx.c
@@ -24,6 +24,16 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #include "lisp.h"
 #include "w32common.h"
 
+static Lisp_Object
+format_string (char const *format, ...)
+{
+  va_list args;
+  va_start (args, format);
+  Lisp_Object str = vformat_string (format, args);
+  va_end (args);
+  return str;
+}
+
 DEFUN ("w32-battery-status", Fw32_battery_status, Sw32_battery_status, 0, 0, 0,
        doc: /* Get power status information from Windows system.
 
@@ -92,32 +102,17 @@ The following %-sequences are provided:
       if (system_status.BatteryLifePercent > 100)
 	load_percentage = build_string ("N/A");
       else
-	{
-	  char buffer[16];
-	  snprintf (buffer, 16, "%d", system_status.BatteryLifePercent);
-	  load_percentage = build_string (buffer);
-	}
+	load_percentage = format_string ("%d", system_status.BatteryLifePercent);
 
       if (seconds_left < 0)
 	seconds = minutes = hours = remain = build_string ("N/A");
       else
 	{
-	  long m;
-	  double h;
-	  char buffer[16];
-	  snprintf (buffer, 16, "%ld", seconds_left);
-	  seconds = build_string (buffer);
-
-	  m = seconds_left / 60;
-	  snprintf (buffer, 16, "%ld", m);
-	  minutes = build_string (buffer);
-
-	  h = seconds_left / 3600.0;
-	  snprintf (buffer, 16, "%3.1f", h);
-	  hours = build_string (buffer);
-
-	  snprintf (buffer, 16, "%ld:%02ld", m / 60, m % 60);
-	  remain = build_string (buffer);
+	  long m = seconds_left / 60;
+	  seconds = format_string ("%ld", seconds_left);
+	  minutes = format_string ("%ld", m);
+	  hours = format_string ("%3.1f", seconds_left / 3600.0);
+	  remain = format_string ("%ld:%02ld", m / 60, m % 60);
 	}
 
       status = listn (CONSTYPE_HEAP, 8,
-- 
2.7.4

bug#32189: 27.0.50; GCC 7 warning due to -Wformat-truncation=2

[Eli Zaretskii]

> Cc: p.stephani2@gmail.com, 32189@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Thu, 19 Jul 2018 16:19:38 -0700
> 
> Let's continue using -Wformat-truncation=2 since I recall it finding real bugs 
> in Emacs in the past, and there's a better fix for this particular problem.
> 
> Some background. Generally I don't worry too much about warnings from older 
> compilers, since one can just compile with --disable-gcc-warnings if the 
> compiler is too old.

Well, GCC 7 is not exactly "old".  GCC 8 was released just a couple of
months ago, and GCC 7.3 is from January this year.  If you are saying
that this switch does a better job in later versions of GCC, maybe we
should disable it for GCC 7 alone?

> Second, the code in question uses snprintf followed by build_string, and can be 
> simplified by using vformat_string instead. That would avoid the warnings and 
> should make the code more reliable. Something like the attached patch, say. I 
> haven't tested it since I don't use MS-Windows. Let's use something like this 
> rather than shutting off the warnings.

Works for me (although vformat_string does exactly what a call to
snprintf does, just with a larger buffer).  Ken, could you please
check if this works in the Cygwin build?  If so, I think we are good
with this change.

And I was wrong: it should go to the master branch, not to emacs-26.

bug#32189: 27.0.50; GCC 7 warning due to -Wformat-truncation=2

[Ken Brown]

On 7/20/2018 2:56 AM, Eli Zaretskii wrote:
>> From: Paul Eggert <eggert@cs.ucla.edu>
>> Date: Thu, 19 Jul 2018 16:19:38 -0700
>> Second, the code in question uses snprintf followed by build_string, and can be
>> simplified by using vformat_string instead. That would avoid the warnings and
>> should make the code more reliable. Something like the attached patch, say. I
>> haven't tested it since I don't use MS-Windows. Let's use something like this
>> rather than shutting off the warnings.
> 
> Works for me (although vformat_string does exactly what a call to
> snprintf does, just with a larger buffer).  Ken, could you please
> check if this works in the Cygwin build?

Yes, although I'm getting the following warning after this change:

../../master/src/w32cygwinx.c: In function ‘format_string’:
../../master/src/w32cygwinx.c:32:3: warning: function ‘format_string’ 
might be a candidate for ‘gnu_printf’ format attribute 
[-Wsuggest-attribute=format]
    Lisp_Object str = vformat_string (format, args);
    ^~~~~~~~~~~

Ken

bug#32189: 27.0.50; GCC 7 warning due to -Wformat-truncation=2

[Eli Zaretskii]

> Cc: p.stephani2@gmail.com, 32189@debbugs.gnu.org
> From: Ken Brown <kbrown@cornell.edu>
> Date: Fri, 20 Jul 2018 09:49:39 -0400
> 
> ../../master/src/w32cygwinx.c: In function ‘format_string’:
> ../../master/src/w32cygwinx.c:32:3: warning: function ‘format_string’ 
> might be a candidate for ‘gnu_printf’ format attribute 
> [-Wsuggest-attribute=format]
>     Lisp_Object str = vformat_string (format, args);
>     ^~~~~~~~~~~

We do this with vformat_string:

  extern Lisp_Object vformat_string (const char *, va_list)
    ATTRIBUTE_FORMAT_PRINTF (1, 0);

So I guess this new function needs a similar treatment.

bug#32189: 27.0.50; GCC 7 warning due to -Wformat-truncation=2

[Ken Brown]

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

On 7/20/2018 10:17 AM, Eli Zaretskii wrote:
>> Cc: p.stephani2@gmail.com, 32189@debbugs.gnu.org
>> From: Ken Brown <kbrown@cornell.edu>
>> Date: Fri, 20 Jul 2018 09:49:39 -0400
>>
>> ../../master/src/w32cygwinx.c: In function ‘format_string’:
>> ../../master/src/w32cygwinx.c:32:3: warning: function ‘format_string’
>> might be a candidate for ‘gnu_printf’ format attribute
>> [-Wsuggest-attribute=format]
>>      Lisp_Object str = vformat_string (format, args);
>>      ^~~~~~~~~~~
> 
> We do this with vformat_string:
> 
>    extern Lisp_Object vformat_string (const char *, va_list)
>      ATTRIBUTE_FORMAT_PRINTF (1, 0);
> 
> So I guess this new function needs a similar treatment.

Amended patch attached.

Ken

[-- Attachment #2: 0001-Simplify-w32cygwinx.c-and-pacify-GCC-Bug-32189.patch --]
[-- Type: text/plain, Size: 2286 bytes --]

From 47bc76b73456c6b9ad02bb02123f2b1285434192 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 19 Jul 2018 16:12:17 -0700
Subject: [PATCH] Simplify w32cygwinx.c and pacify GCC (Bug#32189)

* src/w32cygwinx.c (format_string): New function.
(Fw32_battery_status): Use it.
---
 src/w32cygwinx.c | 37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/src/w32cygwinx.c b/src/w32cygwinx.c
index 8d3ae164cf..5d48c3a9e1 100644
--- a/src/w32cygwinx.c
+++ b/src/w32cygwinx.c
@@ -24,6 +24,16 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #include "lisp.h"
 #include "w32common.h"
 
+static Lisp_Object ATTRIBUTE_FORMAT_PRINTF (1, 2)
+format_string (char const *format, ...)
+{
+  va_list args;
+  va_start (args, format);
+  Lisp_Object str = vformat_string (format, args);
+  va_end (args);
+  return str;
+}
+
 DEFUN ("w32-battery-status", Fw32_battery_status, Sw32_battery_status, 0, 0, 0,
        doc: /* Get power status information from Windows system.
 
@@ -92,32 +102,17 @@ The following %-sequences are provided:
       if (system_status.BatteryLifePercent > 100)
 	load_percentage = build_string ("N/A");
       else
-	{
-	  char buffer[16];
-	  snprintf (buffer, 16, "%d", system_status.BatteryLifePercent);
-	  load_percentage = build_string (buffer);
-	}
+	load_percentage = format_string ("%d", system_status.BatteryLifePercent);
 
       if (seconds_left < 0)
 	seconds = minutes = hours = remain = build_string ("N/A");
       else
 	{
-	  long m;
-	  double h;
-	  char buffer[16];
-	  snprintf (buffer, 16, "%ld", seconds_left);
-	  seconds = build_string (buffer);
-
-	  m = seconds_left / 60;
-	  snprintf (buffer, 16, "%ld", m);
-	  minutes = build_string (buffer);
-
-	  h = seconds_left / 3600.0;
-	  snprintf (buffer, 16, "%3.1f", h);
-	  hours = build_string (buffer);
-
-	  snprintf (buffer, 16, "%ld:%02ld", m / 60, m % 60);
-	  remain = build_string (buffer);
+	  long m = seconds_left / 60;
+	  seconds = format_string ("%ld", seconds_left);
+	  minutes = format_string ("%ld", m);
+	  hours = format_string ("%3.1f", seconds_left / 3600.0);
+	  remain = format_string ("%ld:%02ld", m / 60, m % 60);
 	}
 
       status = listn (CONSTYPE_HEAP, 8,
-- 
2.17.0

bug#32189: 27.0.50; GCC 7 warning due to -Wformat-truncation=2

[Paul Eggert]

Ken Brown wrote:
> Amended patch attached.

Thanks for checking it. I installed it in master.

Eli Zaretskii wrote:

 > GCC 7 is not exactly "old".

True.

 > If you are saying
 > that this switch does a better job in later versions of GCC,

Yes, it does in some cases. I don't know whether it does in this particular 
case. Someone would have to check. But I wouldn't bother checking since we have 
a better fix now anyway (which is often the case for this particular warning).

 > maybe we should disable it for GCC 7 alone?

I'd rather not bother. Better to move on. The general rule for warnings should 
be to cater to the latest GCC, and not worry much about older ones. (I used to 
try doing things the other way and it was way more trouble than it was worth.) 
We need to cater to old compilers for correctness, but we don't need to do it 
for warnings.

bug#32189: 27.0.50; GCC 7 warning due to -Wformat-truncation=2

[Eli Zaretskii]

> Cc: p.stephani2@gmail.com, 32189@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 20 Jul 2018 07:37:18 -0700
> 
>  > maybe we should disable it for GCC 7 alone?
> 
> I'd rather not bother. Better to move on. The general rule for warnings should 
> be to cater to the latest GCC, and not worry much about older ones. (I used to 
> try doing things the other way and it was way more trouble than it was worth.) 
> We need to cater to old compilers for correctness, but we don't need to do it 
> for warnings.

Having warnings from recent versions of the compiler, which are likely
to be used by frequent contributors, runs the risk of making them
ignore warnings, which I think is not a good thing.

Anyway, we solved this one, so no need to argue more.

bug#32189: 27.0.50; GCC 7 warning due to -Wformat-truncation=2

[Ken Brown]

On 7/20/2018 10:37 AM, Paul Eggert wrote:
> Ken Brown wrote:
>> Amended patch attached.
> 
> Thanks for checking it. I installed it in master.

Thanks.  I'm closing the bug.  One last question: It seems that the new 
function 'format_string' could be useful in other places also.  (I may 
have a candidate very soon.)  Would it make sense to move its definition 
to eval.c, where vformat_string is defined, and declare it as external 
in lisp.h?

Ken

bug#32189: 27.0.50; GCC 7 warning due to -Wformat-truncation=2

[Paul Eggert]

On 07/20/2018 12:34 PM, Ken Brown wrote:
> Would it make sense to move its definition to eval.c, where 
> vformat_string is defined, and declare it as external in lisp.h?

Sure, if it's needed. In practice, I've found Fformat and 
Fformat_message to be more useful at the C level (among other things 
they handle more formats), and suggest using them if they're applicable. 
But if they're not, we can use format_string elsewhere -- though it 
would be helpful to note its limitations compared to Fformat_string, in 
a comment perhaps (you can see some of its limitations in the long 
comment at the start of doprnt.c, which the format_string comment could 
refer to).