Fix bogus -Wstringop-overflow warning in Ada

  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.
  

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
  

> 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.
  

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
>
>
  

> 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.
  

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
>
>
  

> 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?
  

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
>
>
>