* [PATCH] Fix bogus -Wstringop-overflow warning in Ada
@ 2022-08-16 13:56 Eric Botcazou
2022-08-17 11:27 ` Richard Biener
0 siblings, 1 reply; 8+ messages in thread
From: Eric Botcazou @ 2022-08-16 13:56 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1109 bytes --]
Hi,
the following bogus warning:
In function 'lto26',
inlined from 'main' at /home/eric/gnat/bugs/V721-018/b~lto26.adb:237:7:
lto26.adb:11:13: warning: writing 1 byte into a region of size 0 [-Wstringop-
overflow=]
11 | Set (R, (7, 0, 84, Stream_Element (I), 0, 0, 0), 1);
| ^
lto26.adb: In function 'main':
lto26.adb:11:50: note: at offset -9223372036854775808 into destination object
'A.0' of size 7
11 | Set (R, (7, 0, 84, Stream_Element (I), 0, 0, 0), 1);
| ^
comes from a discrepancy between get_offset_range, which uses a signed type,
and handle_array_ref, which uses an unsigned one, to do offset computations.
Tested on x86-64/Linux, OK for the mainline?
2022-08-16 Eric Botcazou <ebotcazou@adacore.com>
* pointer-query.cc (handle_array_ref): Fix handling of low bound.
2022-08-16 Eric Botcazou <ebotcazou@adacore.com>
* gnat.dg/lto26.adb: New test.
* gnat.dg/lto26_pkg1.ads, gnat.dg/lto26_pkg1.adb: New helper.
* gnat.dg/lto26_pkg2.ads, gnat.dg/lto26_pkg2.adb: Likewise.
--
Eric Botcazou
[-- Attachment #2: p.diff --]
[-- Type: text/x-patch, Size: 1272 bytes --]
diff --git a/gcc/pointer-query.cc b/gcc/pointer-query.cc
index ae561731216..0f0100233c1 100644
--- a/gcc/pointer-query.cc
+++ b/gcc/pointer-query.cc
@@ -1796,14 +1796,19 @@ handle_array_ref (tree aref, gimple *stmt, bool addr, int ostype,
orng[0] = -orng[1] - 1;
}
- /* Convert the array index range determined above to a byte
- offset. */
+ /* Convert the array index range determined above to a byte offset. */
tree lowbnd = array_ref_low_bound (aref);
- if (!integer_zerop (lowbnd) && tree_fits_uhwi_p (lowbnd))
- {
- /* Adjust the index by the low bound of the array domain
- (normally zero but 1 in Fortran). */
- unsigned HOST_WIDE_INT lb = tree_to_uhwi (lowbnd);
+ if (TREE_CODE (lowbnd) == INTEGER_CST && !integer_zerop (lowbnd))
+ {
+ /* Adjust the index by the low bound of the array domain (0 in C/C++,
+ 1 in Fortran and anything in Ada) by applying the same processing
+ as in get_offset_range. */
+ const wide_int wlb = wi::to_wide (lowbnd);
+ signop sgn = SIGNED;
+ if (TYPE_UNSIGNED (TREE_TYPE (lowbnd))
+ && wlb.get_precision () < TYPE_PRECISION (sizetype))
+ sgn = UNSIGNED;
+ const offset_int lb = offset_int::from (wlb, sgn);
orng[0] -= lb;
orng[1] -= lb;
}
[-- Attachment #3: lto26_pkg1.adb --]
[-- Type: text/x-adasrc, Size: 223 bytes --]
with Lto26_Pkg2; use Lto26_Pkg2;
package body Lto26_Pkg1 is
procedure Set (R : Rec; A : Stream_Element_Array; C :Unsigned_8) is
procedure My_Build is new Build;
begin
My_Build (A, C);
end;
end Lto26_Pkg1;
[-- Attachment #4: lto26_pkg1.ads --]
[-- Type: text/x-adasrc, Size: 274 bytes --]
with Ada.Finalization;
with Ada.Streams; use Ada.Streams;
with Interfaces; use Interfaces;
package Lto26_Pkg1 is
type Rec is new Ada.Finalization.Limited_Controlled with null record;
procedure Set (R : Rec; A : Stream_Element_Array; C :Unsigned_8);
end Lto26_Pkg1;
[-- Attachment #5: lto26_pkg2.ads --]
[-- Type: text/x-adasrc, Size: 181 bytes --]
with Ada.Streams; use Ada.Streams;
with Interfaces; use Interfaces;
package Lto26_Pkg2 is
generic
procedure Build (A : Stream_Element_Array; C : Unsigned_8);
end Lto26_Pkg2;
[-- Attachment #6: lto26_pkg2.adb --]
[-- Type: text/x-adasrc, Size: 391 bytes --]
package body Lto26_Pkg2 is
procedure Build (A : Stream_Element_Array; C : Unsigned_8) is
Start : Stream_Element_Offset := A'First;
Next : Stream_Element_Offset;
Length : Unsigned_8;
begin
for I in 1 .. C loop
Length := Unsigned_8 (A (A'First));
Next := Start + Stream_Element_Offset (Length);
Start := Next;
end loop;
end;
end Lto26_Pkg2;
[-- Attachment #7: lto26.adb --]
[-- Type: text/x-adasrc, Size: 266 bytes --]
-- { dg-do run }
-- { dg-options "-O2 -flto" { target lto } }
with Ada.Streams; use Ada.Streams;
with Lto26_Pkg1; use Lto26_Pkg1;
procedure Lto26 is
R : Rec;
begin
for I in 1 .. 10 loop
Set (R, (7, 0, 84, Stream_Element (I), 0, 0, 0), 1);
end loop;
end;
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix bogus -Wstringop-overflow warning in Ada
2022-08-16 13:56 [PATCH] Fix bogus -Wstringop-overflow warning in Ada Eric Botcazou
@ 2022-08-17 11:27 ` Richard Biener
2022-08-17 13:38 ` Eric Botcazou
0 siblings, 1 reply; 8+ messages in thread
From: Richard Biener @ 2022-08-17 11:27 UTC (permalink / raw)
To: Eric Botcazou; +Cc: GCC Patches
On Tue, Aug 16, 2022 at 3:57 PM Eric Botcazou via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi,
>
> the following bogus warning:
>
> In function 'lto26',
> inlined from 'main' at /home/eric/gnat/bugs/V721-018/b~lto26.adb:237:7:
> lto26.adb:11:13: warning: writing 1 byte into a region of size 0 [-Wstringop-
> overflow=]
> 11 | Set (R, (7, 0, 84, Stream_Element (I), 0, 0, 0), 1);
> | ^
> lto26.adb: In function 'main':
> lto26.adb:11:50: note: at offset -9223372036854775808 into destination object
> 'A.0' of size 7
> 11 | Set (R, (7, 0, 84, Stream_Element (I), 0, 0, 0), 1);
> | ^
>
> comes from a discrepancy between get_offset_range, which uses a signed type,
> and handle_array_ref, which uses an unsigned one, to do offset computations.
>
> Tested on x86-64/Linux, OK for the mainline?
Hmm, can we instead do
if (!integer_zerop (lowbnd) && tree_fits_shwi_p (lowbnd))
{
const offset_int lb = offset_int::from (lowbnd, SIGNED);
...
? In particular interpreting the unsigned lowbnd as SIGNED when
not wlb.get_precision () < TYPE_PRECISION (sizetype), offset_int
should handle all positive and negative byte offsets since it can
also represent them measured in bits. Unfortunately the
wide_int classes do not provide the maximum precision they can
handle. That said, the check, if any, should guard the whole
orng adjustment, no? (in fact I wonder why we just ignore lowbnd
if it doesn't fit or is variable...)
>
>
> 2022-08-16 Eric Botcazou <ebotcazou@adacore.com>
>
> * pointer-query.cc (handle_array_ref): Fix handling of low bound.
>
>
> 2022-08-16 Eric Botcazou <ebotcazou@adacore.com>
>
> * gnat.dg/lto26.adb: New test.
> * gnat.dg/lto26_pkg1.ads, gnat.dg/lto26_pkg1.adb: New helper.
> * gnat.dg/lto26_pkg2.ads, gnat.dg/lto26_pkg2.adb: Likewise.
>
> --
> Eric Botcazou
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix bogus -Wstringop-overflow warning in Ada
2022-08-17 11:27 ` Richard Biener
@ 2022-08-17 13:38 ` Eric Botcazou
2022-08-18 7:30 ` Richard Biener
0 siblings, 1 reply; 8+ messages in thread
From: Eric Botcazou @ 2022-08-17 13:38 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
> Hmm, can we instead do
>
> if (!integer_zerop (lowbnd) && tree_fits_shwi_p (lowbnd))
> {
> const offset_int lb = offset_int::from (lowbnd, SIGNED);
> ...
>
> ?
Apparently not:
In file included from /home/eric/cvs/gcc/gcc/coretypes.h:460,
from /home/eric/cvs/gcc/gcc/pointer-query.cc:23:
/home/eric/cvs/gcc/gcc/wide-int.h: In instantiation of
'wide_int_ref_storage<SE, HDP>::wide_int_ref_storage(const T&) [with T =
tree_node*; bool SE = false; bool HDP = true]':
/home/eric/cvs/gcc/gcc/wide-int.h:782:15: required from
'generic_wide_int<T>::generic_wide_int(const T&) [with T = tree_node*; storage
= wide_int_ref_storage<false>]'
/home/eric/cvs/gcc/gcc/pointer-query.cc:1803:46: required from here
/home/eric/cvs/gcc/gcc/wide-int.h:1024:48: error: incomplete type
'wi::int_traits<tree_node*>' used in nested name specifier
1024 | : storage_ref (wi::int_traits <T>::decompose (scratch,
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~
1025 | wi::get_precision (x),
x))
|
~~~~~~~~~~~~~~~~~~~~~~~~~
And:
const offset_int lb = wi::to_offset (lowbnd);
compiles but does *not* fix the problem since it does a zero-extension.
[Either extension is fine, as long as it's the same in get_offset_range].
> In particular interpreting the unsigned lowbnd as SIGNED when
> not wlb.get_precision () < TYPE_PRECISION (sizetype), offset_int
> should handle all positive and negative byte offsets since it can
> also represent them measured in bits. Unfortunately the
> wide_int classes do not provide the maximum precision they can
> handle. That said, the check, if any, should guard the whole
> orng adjustment, no? (in fact I wonder why we just ignore lowbnd
> if it doesn't fit or is variable...)
Yes, tree_fits_uhwi_p (or tree_fits_shwi_p) is bogus here, but the test
against INTEGER_CST is used everywhere else and should be good enough.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix bogus -Wstringop-overflow warning in Ada
2022-08-17 13:38 ` Eric Botcazou
@ 2022-08-18 7:30 ` Richard Biener
2022-08-18 7:55 ` Eric Botcazou
0 siblings, 1 reply; 8+ messages in thread
From: Richard Biener @ 2022-08-18 7:30 UTC (permalink / raw)
To: Eric Botcazou; +Cc: GCC Patches
On Wed, Aug 17, 2022 at 3:38 PM Eric Botcazou <botcazou@adacore.com> wrote:
>
> > Hmm, can we instead do
> >
> > if (!integer_zerop (lowbnd) && tree_fits_shwi_p (lowbnd))
> > {
> > const offset_int lb = offset_int::from (lowbnd, SIGNED);
> > ...
> >
> > ?
>
> Apparently not:
>
> In file included from /home/eric/cvs/gcc/gcc/coretypes.h:460,
> from /home/eric/cvs/gcc/gcc/pointer-query.cc:23:
> /home/eric/cvs/gcc/gcc/wide-int.h: In instantiation of
> 'wide_int_ref_storage<SE, HDP>::wide_int_ref_storage(const T&) [with T =
> tree_node*; bool SE = false; bool HDP = true]':
> /home/eric/cvs/gcc/gcc/wide-int.h:782:15: required from
> 'generic_wide_int<T>::generic_wide_int(const T&) [with T = tree_node*; storage
> = wide_int_ref_storage<false>]'
> /home/eric/cvs/gcc/gcc/pointer-query.cc:1803:46: required from here
> /home/eric/cvs/gcc/gcc/wide-int.h:1024:48: error: incomplete type
> 'wi::int_traits<tree_node*>' used in nested name specifier
> 1024 | : storage_ref (wi::int_traits <T>::decompose (scratch,
> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~
> 1025 | wi::get_precision (x),
> x))
> |
> ~~~~~~~~~~~~~~~~~~~~~~~~~
>
> And:
>
> const offset_int lb = wi::to_offset (lowbnd);
>
> compiles but does *not* fix the problem since it does a zero-extension.
Meh. OK, eventually would need "indirection" through a wide-int then.
Like
offset_int::from (wi::to_wide (lowbnd), TYPE_SIGN (TREE_TYPE (lowbnd)))
?
> [Either extension is fine, as long as it's the same in get_offset_range].
I think it should extend according to sing of lowbnd? Or does Ada
use an unsigned lowbnd to represent a signed value here? At least
that's what I had issues with with your patch.
> > In particular interpreting the unsigned lowbnd as SIGNED when
> > not wlb.get_precision () < TYPE_PRECISION (sizetype), offset_int
> > should handle all positive and negative byte offsets since it can
> > also represent them measured in bits. Unfortunately the
> > wide_int classes do not provide the maximum precision they can
> > handle. That said, the check, if any, should guard the whole
> > orng adjustment, no? (in fact I wonder why we just ignore lowbnd
> > if it doesn't fit or is variable...)
>
> Yes, tree_fits_uhwi_p (or tree_fits_shwi_p) is bogus here, but the test
> against INTEGER_CST is used everywhere else and should be good enough.
>
> --
> Eric Botcazou
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix bogus -Wstringop-overflow warning in Ada
2022-08-18 7:30 ` Richard Biener
@ 2022-08-18 7:55 ` Eric Botcazou
2022-08-18 9:22 ` Richard Biener
0 siblings, 1 reply; 8+ messages in thread
From: Eric Botcazou @ 2022-08-18 7:55 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
> Meh. OK, eventually would need "indirection" through a wide-int then.
> Like
>
> offset_int::from (wi::to_wide (lowbnd), TYPE_SIGN (TREE_TYPE (lowbnd)))
That would be OK if get_offset_range did the same, but it does not since it
forces a sign-extension whatever the sign of a large type:
signop sgn = SIGNED;
/* Only convert signed integers or unsigned sizetype to a signed
offset and avoid converting large positive values in narrower
types to negative offsets. */
if (TYPE_UNSIGNED (type)
&& wr[0].get_precision () < TYPE_PRECISION (sizetype))
sgn = UNSIGNED;
> I think it should extend according to sing of lowbnd? Or does Ada
> use an unsigned lowbnd to represent a signed value here? At least
> that's what I had issues with with your patch.
It uses sizetype like everyone else and the signedness was forced on it
because of the POINTER_PLUS_EXPR thing, i.e. it was signed before.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix bogus -Wstringop-overflow warning in Ada
2022-08-18 7:55 ` Eric Botcazou
@ 2022-08-18 9:22 ` Richard Biener
2022-08-18 14:47 ` Eric Botcazou
0 siblings, 1 reply; 8+ messages in thread
From: Richard Biener @ 2022-08-18 9:22 UTC (permalink / raw)
To: Eric Botcazou; +Cc: GCC Patches
On Thu, Aug 18, 2022 at 9:54 AM Eric Botcazou <botcazou@adacore.com> wrote:
>
> > Meh. OK, eventually would need "indirection" through a wide-int then.
> > Like
> >
> > offset_int::from (wi::to_wide (lowbnd), TYPE_SIGN (TREE_TYPE (lowbnd)))
>
> That would be OK if get_offset_range did the same, but it does not since it
> forces a sign-extension whatever the sign of a large type:
>
> signop sgn = SIGNED;
> /* Only convert signed integers or unsigned sizetype to a signed
> offset and avoid converting large positive values in narrower
> types to negative offsets. */
> if (TYPE_UNSIGNED (type)
> && wr[0].get_precision () < TYPE_PRECISION (sizetype))
> sgn = UNSIGNED;
>
> > I think it should extend according to sing of lowbnd? Or does Ada
> > use an unsigned lowbnd to represent a signed value here? At least
> > that's what I had issues with with your patch.
>
> It uses sizetype like everyone else and the signedness was forced on it
> because of the POINTER_PLUS_EXPR thing, i.e. it was signed before.
Hmm :/ But that means we _should_ force a sign extension but only
from ptrofftype_p ()? That is, your test above should maybe read
signop sgn = TYPE_SIGN (type);
if (ptrofftype_p (type))
sgn = SIGNED;
assuming 'type' is the type of lowbnd
> --
> Eric Botcazou
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix bogus -Wstringop-overflow warning in Ada
2022-08-18 9:22 ` Richard Biener
@ 2022-08-18 14:47 ` Eric Botcazou
2022-08-19 7:30 ` Richard Biener
0 siblings, 1 reply; 8+ messages in thread
From: Eric Botcazou @ 2022-08-18 14:47 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
> Hmm :/ But that means we _should_ force a sign extension but only
> from ptrofftype_p ()? That is, your test above should maybe read
>
> signop sgn = TYPE_SIGN (type);
> if (ptrofftype_p (type))
> sgn = SIGNED;
>
> assuming 'type' is the type of lowbnd
Yes, that's essentially equivalent to what get_offset_range does, but I'm not
sure why having two slightly different ways of doing it would be better than a
single one here, Maybe replace the call to get_precision in both places with
TYPE_PRECSION (type) then?
--
Eric Botcazou
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix bogus -Wstringop-overflow warning in Ada
2022-08-18 14:47 ` Eric Botcazou
@ 2022-08-19 7:30 ` Richard Biener
0 siblings, 0 replies; 8+ messages in thread
From: Richard Biener @ 2022-08-19 7:30 UTC (permalink / raw)
To: Eric Botcazou; +Cc: GCC Patches
On Thu, Aug 18, 2022 at 4:46 PM Eric Botcazou <botcazou@adacore.com> wrote:
>
> > Hmm :/ But that means we _should_ force a sign extension but only
> > from ptrofftype_p ()? That is, your test above should maybe read
> >
> > signop sgn = TYPE_SIGN (type);
> > if (ptrofftype_p (type))
> > sgn = SIGNED;
> >
> > assuming 'type' is the type of lowbnd
>
> Yes, that's essentially equivalent to what get_offset_range does, but I'm not
> sure why having two slightly different ways of doing it would be better than a
> single one here, Maybe replace the call to get_precision in both places with
> TYPE_PRECSION (type) then?
I wasn't aware of the copy in get_offset_range. To cite:
wide_int wr[2];
if (!get_range (x, stmt, wr, rvals))
return false;
signop sgn = SIGNED;
/* Only convert signed integers or unsigned sizetype to a signed
offset and avoid converting large positive values in narrower
types to negative offsets. */
if (TYPE_UNSIGNED (type)
&& wr[0].get_precision () < TYPE_PRECISION (sizetype))
sgn = UNSIGNED;
r[0] = offset_int::from (wr[0], sgn);
r[1] = offset_int::from (wr[1], sgn);
I guess the main issue here is that the machinery converts to offset_int
prematurely and thus needs to do it even when it's not clear in what context
(POINTER_PLUS_EXPR offset or not) it is used. The code unfortunately
is a bit of a mess and I'm not too familiar with it. I'm OK with your
original patch, given the above it's consistent (even if maybe broken).
Thanks,
Richard.
>
> --
> Eric Botcazou
>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-08-19 7:30 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-16 13:56 [PATCH] Fix bogus -Wstringop-overflow warning in Ada Eric Botcazou
2022-08-17 11:27 ` Richard Biener
2022-08-17 13:38 ` Eric Botcazou
2022-08-18 7:30 ` Richard Biener
2022-08-18 7:55 ` Eric Botcazou
2022-08-18 9:22 ` Richard Biener
2022-08-18 14:47 ` Eric Botcazou
2022-08-19 7:30 ` Richard Biener
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).