[PATCH][C] Fixup pointer-int-sum

[PATCH][C] Fixup pointer-int-sum

[Richard Guenther]

This tries to make sense of the comments and code in the code
doing the index - size multiplication in pointer-int-sum.  It
also fixes a bogus integer-constant conversion which results
in not properly canonicalized integer constants.

The comment in the code claims the index - size multiplication
is carried out signed, which doesn't match the code which does
it unsigned (the commend dates back to rev. 6733 where we _did_
carry out the multiplication in a signed type, using
c_common_type_for_size (TYPE_PRECISION (sizetype), 0)).  The
following patch makes us preserve the signedness of intop
so that for signed intop the multiplication will be known to
not overflow (what is actually the C semantics - is the
multiplication allowed to overflow for unsigned intop?  If not
I guess the orginal code of always choosing a signed type was
more correct and we should go back to it instead?)

The comment also claims there is a sign-extension of t
to sizetype - that's not true either, it's just a sign-change.

Joseph, do we want an unconditional (un-)signed multiplication
(before the patch it's unsigned), or what the patch does?

Bootstrapped and tested on x86_64-unknown-linux-gnu.  I'll also
test the unconditionally signed variant.

Thanks,
Richard.

2011-07-07  Richard Guenther  <rguenther@suse.de>

	* c-common.c (pointer_int_sum): Do the index times size
	multiplication in the signedness of index.  Properly
	strip overflow flags.

Index: gcc/c-family/c-common.c
===================================================================
--- gcc/c-family/c-common.c	(revision 175962)
+++ gcc/c-family/c-common.c	(working copy)
@@ -3737,23 +3737,22 @@ pointer_int_sum (location_t loc, enum tr
 
   /* Convert the integer argument to a type the same size as sizetype
      so the multiply won't overflow spuriously.  */
-  if (TYPE_PRECISION (TREE_TYPE (intop)) != TYPE_PRECISION (sizetype)
-      || TYPE_UNSIGNED (TREE_TYPE (intop)) != TYPE_UNSIGNED (sizetype))
+  if (TYPE_PRECISION (TREE_TYPE (intop)) != TYPE_PRECISION (sizetype))
     intop = convert (c_common_type_for_size (TYPE_PRECISION (sizetype),
-					     TYPE_UNSIGNED (sizetype)), intop);
+					     TYPE_UNSIGNED (TREE_TYPE (intop))),
+		     intop);
 
   /* Replace the integer argument with a suitable product by the object size.
-     Do this multiplication as signed, then convert to the appropriate type
-     for the pointer operation and disregard an overflow that occured only
-     because of the sign-extension change in the latter conversion.  */
+     Do this multiplication in a widened intop type, then convert to the
+     appropriate type for the pointer operation and disregard an overflow
+     that occured only because of the sign-change in the latter conversion.  */
   {
     tree t = build_binary_op (loc,
 			      MULT_EXPR, intop,
 			      convert (TREE_TYPE (intop), size_exp), 1);
     intop = convert (sizetype, t);
     if (TREE_OVERFLOW_P (intop) && !TREE_OVERFLOW (t))
-      intop = build_int_cst_wide (TREE_TYPE (intop), TREE_INT_CST_LOW (intop),
-				  TREE_INT_CST_HIGH (intop));
+      intop = double_int_to_tree (sizetype, tree_to_double_int (intop));
   }
 
   /* Create the sum or difference.  */

Re: [PATCH][C] Fixup pointer-int-sum

[Joseph S. Myers]

On Thu, 7 Jul 2011, Richard Guenther wrote:

> not overflow (what is actually the C semantics - is the
> multiplication allowed to overflow for unsigned intop?  If not

Overflow is not allowed.  Formally the multiplication is as-if to infinite 
precision, and then there is undefined behavior if the result of the 
addition (to infinite precision) is outside the array pointed to - 
wrapping around by some multiple of the whole address space is not 
allowed.

In practice, as previously discussed objects half or more of the address 
space do not work reliably because of the problems doing pointer 
subtraction, so always using a signed type shouldn't break anything that 
actually worked reliably (though how unreliable things were with large 
malloced objects - which unfortunately glibc's malloc can provide - if the 
source code didn't use pointer subtraction, I don't know).

In GCC's terms half or more of the address space generally means half the 
range of size_t.  (m32c has ptrdiff_t wider than size_t in some cases.  On 
such unusual architectures it ought to be possible to have objects whose 
size is up to SIZE_MAX bytes and have pointer addition and subtraction 
work reliably, which would suggest using ptrdiff_t for arithmetic in such 
cases, but the code checking sizes for arrays of constant size uses the 
signed type corresponding to size_t, so you could only get a larger object 
through malloc or VLAs.)

The patch is OK.  Unconditionally signed is also OK, though I don't see 
any advantage over this version.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH][C] Fixup pointer-int-sum

[Richard Guenther]

On Thu, 7 Jul 2011, Joseph S. Myers wrote:

> On Thu, 7 Jul 2011, Richard Guenther wrote:
> 
> > not overflow (what is actually the C semantics - is the
> > multiplication allowed to overflow for unsigned intop?  If not
> 
> Overflow is not allowed.  Formally the multiplication is as-if to infinite 
> precision, and then there is undefined behavior if the result of the 
> addition (to infinite precision) is outside the array pointed to - 
> wrapping around by some multiple of the whole address space is not 
> allowed.
> 
> In practice, as previously discussed objects half or more of the address 
> space do not work reliably because of the problems doing pointer 
> subtraction, so always using a signed type shouldn't break anything that 
> actually worked reliably (though how unreliable things were with large 
> malloced objects - which unfortunately glibc's malloc can provide - if the 
> source code didn't use pointer subtraction, I don't know).
> 
> In GCC's terms half or more of the address space generally means half the 
> range of size_t.  (m32c has ptrdiff_t wider than size_t in some cases.  On 
> such unusual architectures it ought to be possible to have objects whose 
> size is up to SIZE_MAX bytes and have pointer addition and subtraction 
> work reliably, which would suggest using ptrdiff_t for arithmetic in such 
> cases, but the code checking sizes for arrays of constant size uses the 
> signed type corresponding to size_t, so you could only get a larger object 
> through malloc or VLAs.)
> 
> The patch is OK.  Unconditionally signed is also OK, though I don't see 
> any advantage over this version.

Ok, I'll defer the decision to the time I have settled on a final
solution to get rid of the (unsigned) sizetype offset operand
for POINTER_PLUS_EXPR.  The least invasive idea is to introduce a
new signed ptrofftype to replace all sizetype conversions at places
we build POINTER_PLUS_EXPRs.  That would favor unconditionally
signed.  The moderate invasive idea is to allow both a signed
and an unsigned ptrofftype (but still force a common precision),
with all the fun that arises from combining (ptr p+ off1) p+ off2
with different signs for the offset operand ...

Thanks,
Richard.