public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Martin Sebor <msebor@gmail.com>
To: Jeff Law <law@redhat.com>, Gcc Patch List <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] avoid using upper bound of width and precision in -Wformat-overlow (PR 79692)
Date: Wed, 01 Mar 2017 23:48:00 -0000	[thread overview]
Message-ID: <8c32721d-1d60-4d4c-e71f-260705f85977@gmail.com> (raw)
In-Reply-To: <623e38f3-8d0f-0866-87ce-d005ad0613f8@redhat.com>

> So in some cases you use
>
> +  /* For an indeterminate precision the lower bound must be assumed
> +     to be zero.  */
> +  if (prec[0] < 0 && prec[1] >= 0)
>
> Note prec[1] >= 0
>
> In other cases you have:
>
> +    /* The minimum output with unknown precision is a single byte
> +       (e.g., "0") but the more likely output is 3 bytes ("0.0").  */
> +    if (dir.prec[0] < 0 && dir.prec[1] > 0)
>
> Note  dir.prec[1] > 0
>
> Shouldn't one of those be changed to be consistent with the other?

Thanks for the careful review!  The two tests determine two different
things so I'm not sure they need to be consistent. But considering
your question made me realize that the first conditional isn't
completely correct:

+  /* For an indeterminate precision the lower bound must be assumed
+     to be zero.  */
+  if (prec[0] < 0 && prec[1] >= 0)
+    prec[0] = 0;

Precisions in a negative-positive range with an upper bound of less
than 6 must be assumed to have an upper bound of 6 because that's
the default when precision is negative.  E.g., given
snprintf (0, 0, "%*f", p, 1.23456789) where p is in [-1, 0] results
either in:

   1.234568

when (p == -1) holds, or in:

   1

when (p == 0) holds.  So while the lower bound in the if statement
above must be set to zero, the upper bound may need to be adjusted
as well.

The patch I just committed in r245822 fixes that (and also changes
the conditional so that consistency is no longer an issue).

However, while reviewing the rest of the floating point handling
code it became clear that this is a bug that affects both floating
point formatting functions (i.e., the one that handles constants
as well as the non-constant one).  I also noticed some other minor
glitches in this area that should be fixed.  I'm testing another
patch that resolves those problem as well.

> Similarly in known_width_and_precision.  Please review the patch to
> ensure that we're as consistent as possible for these tests.

In known_width_and_precision the different inequalities are
deliberate.  Because the actual width is an absolute value
of the specified argument the range of unknown width is
[0, INT_MAX + 1] (printf("%*i", INT_MIN, i) specifies a width
of -INT_MIN, or INT_MAX + 1 (without overflow).  But because
negative precisions are ignored, the largest upper bound is
INT_MAX.

Martin

      reply	other threads:[~2017-03-01 23:48 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-25 22:56 Martin Sebor
2017-02-27 16:25 ` Jeff Law
2017-03-01 23:48   ` Martin Sebor [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8c32721d-1d60-4d4c-e71f-260705f85977@gmail.com \
    --to=msebor@gmail.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=law@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).