[Bug tree-optimization/85175] [8 regression] false-positive -Wformat-overflow= warning with gcc-8 -Os

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85175

--- Comment #6 from Martin Sebor  ---
I wonder if adding an attribute to constrain the range of a variable on
declaration would be a solution.  That way the first warning on your list, for
example, could be avoided by annotating id along the lines of:

  struct acpi_processor {
acpi_handle handle;
u32 acpi_id;
phys_cpuid_t phys_id;   /* CPU hardware ID such as APIC ID for x86 */
u32 id __attribute__ ((range (0, CONFIG_NR_CPUS))); /* CPU logical
ID allocated by OS */
...
  };

and have GCC use that range on each read access to id.  This could also improve
code generation.  (Things would of course break badly if id were assigned an
out-of-bounds value; GCC could help by warning on such assignments if/when it
caught them.)

This might be something to consider for GCC 9 (it's too late for GCC 8).

[Bug tree-optimization/85175] [8 regression] false-positive -Wformat-overflow= warning with gcc-8 -Os

[arnd at linaro dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85175

--- Comment #5 from Arnd Bergmann  ---
Improving the optimizer will definitely help this one, but not the other
instances I found. Here's a list of the remaining warnings that got introduced
in the linux kernel by r257857 for reference:

https://elixir.bootlin.com/linux/v4.16/source/drivers/acpi/acpi_processor.c#L330
invalid_logical_cpuid() has checked the 'id' variable to be a positive number
(i.e. not an error value), so it's assumed to be in the range of [0,
2147483647] by the compiler, while we know it it's in the range of [0,
CONFIG_NR_CPUS] when the id is positive. I'd work around that by adding a '<
CONFIG_NR_CPUS' check in invalid_logical_cpuid.

https://elixir.bootlin.com/linux/v4.16/source/drivers/gpu/drm/imx/imx-ldb.c#L634
The one referenced reported here, ideally handled better by gcc

https://elixir.bootlin.com/linux/v4.16/source/drivers/usb/gadget/function/rndis.c#L900
We check the 'id' for a positive number, negative values are error codes. 
rndis_get_nr() otherwise returns the first available number, so we can assume
it's a low number (you won't have billions of USB network interfaces), but
making the buffer larger is a safer fix.

https://elixir.bootlin.com/linux/v4.16/source/drivers/usb/gadget/udc/fsl_udc_core.c#L2497
We know that max_ep is a small number, but gcc cannot know this, so the loop
index has a lower bound but no upper bound. Would work around this by
increasing the buffer size from 14 to 16 bytes.

https://elixir.bootlin.com/linux/v4.16/source/sound/pci/cmipci.c#L3157
We check for an upper bound but not a lower bound on a signed integer that we
know contains a positive number. The warning seems reasonable here, and I would
make the variable unsigned.

https://elixir.bootlin.com/linux/v4.16/source/drivers/scsi/ch.c#L937
ch->minor is known to be less than 128 CH_MAX_DEVS here, but gcc cannot see
this. Again, idr_alloc() returns a negative code in case of an error, so gcc
sees that the variable has a lower bound, but not the upper bound. Would work
around this using a %hhi modifier.

https://elixir.bootlin.com/linux/v4.16/source/drivers/power/supply/sbs-battery.c#L559
sbs_read_word_data() returns either a negative error code or a positive 16-bit
number. Would work around that using the %hx modifier.

https://elixir.bootlin.com/linux/v4.16/source/net/bluetooth/hci_core.c#L3093
idr_alloc() once more returns a negative number on error, or a positive number
that may have an upper bound (the third argument to idr_alloc). Here we should
specify the upper bound (1), but gcc won't see it and still warn, so we
also need a way to tell it that 'id' is a four-digit number. Using the %hi
format won't work as that is still five digits.

To summarize: For this particular project (linux kernel), these added tend to
be slightly annoying false-positives, but we can work around that with a
handful of simple patches. In five of the eight cases, gcc sees a limited range
of [0, 2147483647] because of an explicit check for an error code. I don't know
how common that code pattern is in other projects, but for us it would be more
logical to treat this like the unbounded range in -Wformat-overflow=2 rather
than a range with a known bound for -Wformat-overflow=1.

[Bug tree-optimization/85175] [8 regression] false-positive -Wformat-overflow= warning with gcc-8 -Os

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85175

--- Comment #4 from Martin Sebor  ---
In general, sizing buffers for the minimum/maximum value of each directive's
argument and choosing the appropriate length modifiers (e.g., %hhi for char or
%hi for short) avoids these problems.

The thinking behind the current behavior at level 1 is that an expression in
some known range is a sign that the programmer may have made an effort to
constrain the value, while an expression for which we have no range information
may have been constrained and we just don't see it.  When the known range
doesn't constrain the value sufficiently, the warning triggers.  An unknown
range doesn't.  The "fix" for a warning in the former case should in most cases
be to simply constrain the variable either by using an appropriate length
modifier in the directive or just before the call (e.g., via some form of an
assertion or an equivalent of '((x < min || x > max) ? __builtin_unreachable ()
: (void)0).   This works quite well except when the buffer is just big enough
and when the warning exposes weaknesses in optimizations and/or unforeseen
interactions with other transformations (usually jump-threading or the
sanitizers).  Rather than weakening the warnings our approach to these problems
has been to improve the optimizers.  I'm hoping this case can be dealt with the
same way but I'm not familiar enough with the EVRP machinery yet to tell if
that will be possible.

As for -Wformat-overflow=2, an expression in an unknown range is treated as if
it had the maximum range of its type.

(I suppose we should also have level 3 where ranges are ignored and buffers are
expected to be sized only according to argument type.  I realize it's not what
you're looking for but it would make it straightforward to write safe,
warning-free code regardless of optimizations.)

[Bug tree-optimization/85175] [8 regression] false-positive -Wformat-overflow= warning with gcc-8 -Os

[arnd at linaro dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85175

--- Comment #3 from Arnd Bergmann  ---
(In reply to Martin Sebor from comment #2)

> So with the change above GCC is doing a better but imperfect job of
> determining the range.  Changing the variable to unsigned constrains the
> lower bound to zero and eliminates the warning even at -Os.

Ok, so this is actually the same thing I saw with seven other files in the
kernel (out of several hundred randconfig kernel builds on three
architectures). In each case, we now have a signed integer range that appears
to be constrained on one side after r257857, e.g. [-2147483647, 68] or [1,
2147483647] based on a partial range check, but the warning is still a false
positive in the end.

The one such warning we gained in the kernel that makes sense was this one (not
sent yet):

commit c18e88d296264d76f1a242ae95a43681cf198078
Author: Arnd Bergmann 
Date:   Tue Apr 3 09:45:35 2018 +0200

bluetooth: fix hci name overflow

gcc-8 warns that the index of the hci device could overflow the eight
character array:

net/bluetooth/hci_core.c: In function 'hci_register_dev':
net/bluetooth/hci_core.c:3093:26: error: '%d' directive writing between 1
and 10 bytes into a region of size 5 [-Werror=format-overflow=]
  sprintf(hdev->name, "hci%d", id);
  ^~
net/bluetooth/hci_core.c:3093:22: note: directive argument in the range [0,
2147483647]
  sprintf(hdev->name, "hci%d", id);
  ^~~
net/bluetooth/hci_core.c:3093:2: note: 'sprintf' output between 5 and 14
bytes into a destination of size 8
  sprintf(hdev->name, "hci%d", id);

This uses snprintf() to enforce a valid string, and limits the range of
the integer to 0... In practice this should not matter as we would
not be able connect more than  bluetooth hci's simultaneously.

Signed-off-by: Arnd Bergmann 

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 40d260f2bea5..9e2ad444d799 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3075,13 +3075,14 @@ int hci_register_dev(struct hci_dev *hdev)

/* Do not allow HCI_AMP devices to register at index 0,
 * so the index can be used as the AMP controller ID.
+* Ensure the name fits into eight characters id < 1.
 */
switch (hdev->dev_type) {
case HCI_PRIMARY:
-   id = ida_simple_get(_index_ida, 0, 0, GFP_KERNEL);
+   id = ida_simple_get(_index_ida, 0, 1, GFP_KERNEL);
break;
case HCI_AMP:
-   id = ida_simple_get(_index_ida, 1, 0, GFP_KERNEL);
+   id = ida_simple_get(_index_ida, 1, 1, GFP_KERNEL);
break;
default:
return -EINVAL;
@@ -3090,7 +3091,7 @@ int hci_register_dev(struct hci_dev *hdev)
if (id < 0)
return id;

-   sprintf(hdev->name, "hci%d", id);
+   snprintf(hdev->name, sizeof(hdev->name), "hci%d", id);
hdev->id = id;

BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);

The other new warnings are either the same kind as this one (the compiler
should be able to figure it out), or the sort where the compiler is technically
right about the string overflow based on the types, but we can easily prove
that the range is more limited like in the ida_simple_get() case with correct
limits (this is an extern function that returns an unused number within a
strict range).

Would it be sensible to only warn with -Wformat-overflow=1 on a signed integer
if the overflow happens with an actual known limit, but not if the limit is
euqal to the minimum/maximum representable numbers? The documentation for
-Wformat-overflow=2 isn't completely clear on what behavior was intended here
for the =1 and =2 case if the range is only bounded on one side.

[Bug tree-optimization/85175] [8 regression] false-positive -Wformat-overflow= warning with gcc-8 -Os

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85175

--- Comment #2 from Martin Sebor  ---
The warning appeared with r257857:

r257857 | law | 2018-02-20 13:59:22 -0500 (Tue, 20 Feb 2018) | 8 lines

PR middle-end/82123
PR tree-optimization/81592
PR middle-end/79257
* gimple-ssa-sprintf.c (format_integer): Query EVRP range analyzer
for range data rather than using global data.

The difference in the dump between the revision prior to it and r257857 is:

  Directive 2 at offset 2: "%d"
Result: 1, 1, 11, 11 (3, 3, 13, 13)

and

  Directive 2 at offset 2: "%d"
Result: 1, 11, 11, 11 (3, 13, 13, 13)

I.e., GCC now thinks the likely output of the %d directive is 11 bytes (the
second number on the Result: line).  Prior to r257857 the range type of the
argument was VR_VARYING for which the warning doesn't trigger.  With r257857
the range is  [-2147483647, 3] of which the warning uses the larger bound to
trigger.

So with the change above GCC is doing a better but imperfect job of determining
the range.  Changing the variable to unsigned constrains the lower bound to
zero and eliminates the warning even at -Os.

[Bug tree-optimization/85175] [8 regression] false-positive -Wformat-overflow= warning with gcc-8 -Os

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85175

Jakub Jelinek  changed:

   What|Removed |Added

 Status|UNCONFIRMED |NEW
   Last reconfirmed||2018-04-03
 CC||jakub at gcc dot gnu.org
 Ever confirmed|0   |1

--- Comment #1 from Jakub Jelinek  ---
The problem is that i is addressable, so it is not in SSA form.  With -O2, the
ch pass copies the loop header and so we end up with a PHI and in vrp2 (not
vrp1!) we finally manage to compute the value range for the sprintf argument
SSA_NAME;
it is not precise, because we compute
  # RANGE [-2147483647, 3]
  # i.2_15 = PHI <0(2), _2(3)>
because we really don't track what the addressable i contains and have:
  i.1_1 = i;
  # RANGE [-2147483647, 2147483647]
  _2 = i.1_1 + 1;
  i = _2;
  if (_2 <= 3)
With -Os, there is no loop header copying and
  i.2_3 = i;
  if (i.2_3 <= 3)
goto ; [89.00%]
is used directly in the sprintf, so there is no second SSA_NAME we could attach
at least the limited [-2147483647, 3] range to.

So, either the planned on-demand VR computation could help here, or perhaps
some range splitting on GIMPLE, e.g. we could figure out that the taking of the
address is dominated by large enough block (even loop here) where we could
promote the variable into SSA form and only store it into the addressable var
before the address is taken.  If we do something like that, we'd need to add
debug stmts.

[Bug tree-optimization/85175] [8 regression] false-positive -Wformat-overflow= warning with gcc-8 -Os

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85175

Richard Biener  changed:

   What|Removed |Added

   Target Milestone|--- |8.0