Re: [PATCH] Fix bogus -Wstringop-overflow warning
Hello,
On Fri, Oct 14 2022, Martin Liška wrote:
> On 10/14/22 08:12, Richard Biener wrote:
>> On Fri, Oct 14, 2022 at 12:54 AM Eric Botcazou via Gcc-patches
>> wrote:
>>>
Not a fan as it could potentially hide a real issue, but I don't really
have a better solution.
>>>
>>> Thanks.
>>>
I pondered suggesting "access" affect type identity, but the cases where
that's really important are probably better handled by the "fn spec"
attribute, leaving "access" strictly impacting diagnostics.
>>>
>>> I can expand a bit here, because I tried to change the "access" attribute
>>> that
>>> way and this badly breaks the C compiler, for example:
>>>
>>> int foo (int n, char m[1][n]);
>>>
>>> int foo (int n, char m[1][n]) {}
>>>
>>> no longer compiles with an error about different function types.
>>
>> Note in discussion with IPA folks we agreed that IPA cloning that modifies
>> arguments either has to remove access attributes, adjust them or refrain
>> from cloning.
>>
>> Martin - has anything been done to this respect?
>
> I think it's more for Martin Jambor who's the IPA specialist when it comes
> to parameter manipulation.
>
They are being dropped since 2af63f0f53a
Adjusting them accordingly is an item buried quite deep in my TODO list.
>>
>> I suppose there's also a way to figure if a clone has arguments
>> changed in any way?
Look whether clone_info::get (node) exists and its param_adjustments is
non-NULL.
In theory the param_adjustments could contain description of the very
same signature the original function has but in practice it does not
currently happen and is unlikely to happen ever.
Martin
Re: [PATCH] Fix bogus -Wstringop-overflow warning
On 10/14/22 08:12, Richard Biener wrote:
> On Fri, Oct 14, 2022 at 12:54 AM Eric Botcazou via Gcc-patches
> wrote:
>>
>>> Not a fan as it could potentially hide a real issue, but I don't really
>>> have a better solution.
>>
>> Thanks.
>>
>>> I pondered suggesting "access" affect type identity, but the cases where
>>> that's really important are probably better handled by the "fn spec"
>>> attribute, leaving "access" strictly impacting diagnostics.
>>
>> I can expand a bit here, because I tried to change the "access" attribute
>> that
>> way and this badly breaks the C compiler, for example:
>>
>> int foo (int n, char m[1][n]);
>>
>> int foo (int n, char m[1][n]) {}
>>
>> no longer compiles with an error about different function types.
>
> Note in discussion with IPA folks we agreed that IPA cloning that modifies
> arguments either has to remove access attributes, adjust them or refrain
> from cloning.
>
> Martin - has anything been done to this respect?
I think it's more for Martin Jambor who's the IPA specialist when it comes
to parameter manipulation.
Martin
>
> I suppose there's also a way to figure if a clone has arguments
> changed in any way?
>
> Thanks,
> Richard.
>
>> --
>> Eric Botcazou
>>
>>
Re: [PATCH] Fix bogus -Wstringop-overflow warning
On Fri, Oct 14, 2022 at 12:54 AM Eric Botcazou via Gcc-patches
wrote:
>
> > Not a fan as it could potentially hide a real issue, but I don't really
> > have a better solution.
>
> Thanks.
>
> > I pondered suggesting "access" affect type identity, but the cases where
> > that's really important are probably better handled by the "fn spec"
> > attribute, leaving "access" strictly impacting diagnostics.
>
> I can expand a bit here, because I tried to change the "access" attribute that
> way and this badly breaks the C compiler, for example:
>
> int foo (int n, char m[1][n]);
>
> int foo (int n, char m[1][n]) {}
>
> no longer compiles with an error about different function types.
Note in discussion with IPA folks we agreed that IPA cloning that modifies
arguments either has to remove access attributes, adjust them or refrain
from cloning.
Martin - has anything been done to this respect?
I suppose there's also a way to figure if a clone has arguments
changed in any way?
Thanks,
Richard.
> --
> Eric Botcazou
>
>
Re: [PATCH] Fix bogus -Wstringop-overflow warning
> Not a fan as it could potentially hide a real issue, but I don't really
> have a better solution.
Thanks.
> I pondered suggesting "access" affect type identity, but the cases where
> that's really important are probably better handled by the "fn spec"
> attribute, leaving "access" strictly impacting diagnostics.
I can expand a bit here, because I tried to change the "access" attribute that
way and this badly breaks the C compiler, for example:
int foo (int n, char m[1][n]);
int foo (int n, char m[1][n]) {}
no longer compiles with an error about different function types.
--
Eric Botcazou
Re: [PATCH] Fix bogus -Wstringop-overflow warning
On 10/13/22 06:06, Eric Botcazou via Gcc-patches wrote: Hi, if you compile the attached testcase with -O2 -fno-inline -Wall, you get: In function 'process_array3': cc1: warning: 'process_array4' accessing 4 bytes in a region of size 3 [- Wstringop-overflow=] cc1: note: referencing argument 1 of type 'char[4]' t.c:6:6: note: in a call to function 'process_array4' 6 | void process_array4 (char a[4], int n) | ^~ cc1: warning: 'process_array4' accessing 4 bytes in a region of size 3 [- Wstringop-overflow=] cc1: note: referencing argument 1 of type 'char[4]' t.c:6:6: note: in a call to function 'process_array4' That's because the ICF IPA pass has identified the two functions and turned process_array3 into a wrapper of process_array4. This looks sensible to me given that the only difference between them is an "access" attribute on their type describing the access size of the parameter and the "access" attribute does not affect type identity (struct attribute_spec.affects_type_identity). Hence the proposed fix, tested on x86-64/Linux, OK for the mainline? 2022-10-13 Eric Botcazou * gimple-ssa-warn-access.cc (pass_waccess::check_call): Return early for calls made from thunks. 2022-10-13 Eric Botcazou * gcc.dg/Wstringop-overflow-89.c: New test. Not a fan as it could potentially hide a real issue, but I don't really have a better solution. I pondered suggesting "access" affect type identity, but the cases where that's really important are probably better handled by the "fn spec" attribute, leaving "access" strictly impacting diagnostics. OK jeff
Re: [PATCH] Fix bogus -Wstringop-overflow warning in Ada
On Thu, Aug 18, 2022 at 4:46 PM Eric Botcazou 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 > > >
Re: [PATCH] Fix bogus -Wstringop-overflow warning in Ada
> 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
Re: [PATCH] Fix bogus -Wstringop-overflow warning in Ada
On Thu, Aug 18, 2022 at 9:54 AM Eric Botcazou 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 > >
Re: [PATCH] Fix bogus -Wstringop-overflow warning in Ada
> 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
Re: [PATCH] Fix bogus -Wstringop-overflow warning in Ada
On Wed, Aug 17, 2022 at 3:38 PM Eric Botcazou 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::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::generic_wide_int(const T&) [with T = tree_node*; storage
> = wide_int_ref_storage]'
> /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' used in nested name specifier
> 1024 | : storage_ref (wi::int_traits ::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
>
>
Re: [PATCH] Fix bogus -Wstringop-overflow warning in Ada
> 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::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::generic_wide_int(const T&) [with T = tree_node*; storage
= wide_int_ref_storage]'
/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' used in nested name specifier
1024 | : storage_ref (wi::int_traits ::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
Re: [PATCH] Fix bogus -Wstringop-overflow warning in Ada
On Tue, Aug 16, 2022 at 3:57 PM Eric Botcazou via Gcc-patches
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
>
> * pointer-query.cc (handle_array_ref): Fix handling of low bound.
>
>
> 2022-08-16 Eric Botcazou
>
> * 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
