Re: [patch, fortran] PR66310 Problems with intrinsic repeat for large number of copies

[Dominique d'Humières]

> Le 18 juil. 2016 à 02:02, Jerry DeLisle  a écrit :
> 
> Please test this revised patch. See my comments in the PR.
> 
> I think we should commit this one.
> 
> Jerry

Jerry,

As said on IRC I think the limit should be documented and a TODO comment added 
to gcc/fortran/gfortran.h.

While trying to bootstrap with the patch I got

/opt/gcc/build_w/./prev-gcc/xg++ -B/opt/gcc/build_w/./prev-gcc/ 
-B/opt/gcc/gcc7w/x86_64-apple-darwin15.5.0/bin/ -nostdinc++ 
-B/opt/gcc/build_w/prev-x86_64-apple-darwin15.5.0/libstdc++-v3/src/.libs 
-B/opt/gcc/build_w/prev-x86_64-apple-darwin15.5.0/libstdc++-v3/libsupc++/.libs  
-I/opt/gcc/build_w/prev-x86_64-apple-darwin15.5.0/libstdc++-v3/include/x86_64-apple-darwin15.5.0
  -I/opt/gcc/build_w/prev-x86_64-apple-darwin15.5.0/libstdc++-v3/include  
-I/opt/gcc/work/libstdc++-v3/libsupc++ 
-L/opt/gcc/build_w/prev-x86_64-apple-darwin15.5.0/libstdc++-v3/src/.libs 
-L/opt/gcc/build_w/prev-x86_64-apple-darwin15.5.0/libstdc++-v3/libsupc++/.libs 
-fno-PIE -c  -DIN_GCC_FRONTEND -g -O2   -gtoggle -DIN_GCC -fno-exceptions 
-fno-rtti -fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings 
-Wcast-qual -Wmissing-format-attribute -Woverloaded-virtual -pedantic 
-Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -Werror -fno-common 
 -DHAVE_CONFIG_H -I. -Ifortran -I../../work/gcc -I../../work/gcc/fortran 
-I../../work/gcc/../include -I./../intl -I../../work/gcc/../libcpp/include 
-I/opt/mp-new/include  -I../../work/gcc/../libdecnumber 
-I../../work/gcc/../libdecnumber/dpd -I../libdecnumber 
-I../../work/gcc/../libbacktrace -I/opt/mp-new/include  -o fortran/simplify.o 
-MT fortran/simplify.o -MMD -MP -MF fortran/.deps/simplify.TPo 
../../work/gcc/fortran/simplify.c
../../work/gcc/fortran/simplify.c: In function 'gfc_expr* 
gfc_simplify_repeat(gfc_expr*, gfc_expr*)':
../../work/gcc/fortran/simplify.c:5089:11: error: variable 'i' set but not used 
[-Werror=unused-but-set-variable]
   int i;
   ^
cc1plus: all warnings being treated as errors

i.e., the lines

   int i;

and

   i = gfc_validate_kind (BT_INTEGER, gfc_charlen_int_kind, false);

must be deleted. also the comment

  /* Compute the maximum value allowed for NCOPIES:
huge(cl) - 1 / len.  */

should be updated.

Last point I have found, the limit does not seem to take into account 
CHARACTER(KIND=4):

[Book15] f90/bug% cat pr66310_par_4.f90
   program p
  character(len=2,kind=4), parameter :: z = 'yz'
  print *, repeat(z, 2**25)
   end
[Book15] f90/bug% gfc pr66310_par_4.f90
f951(3881,0x7fff74e38000) malloc: *** mach_vm_map(size=18446744073441116160) 
failed (error code=3)
*** error: can't allocate region
*** set a breakpoint in malloc_error_break to debug

f951: out of memory allocating 18446744073441116160 bytes after a total of 0 
bytes

Thanks for working on this PR.

Dominique

Re: [patch, fortran] PR66310 Problems with intrinsic repeat for large number of copies

[Jerry DeLisle]

On 07/12/2016 06:26 AM, Dominique d'Humières wrote:
>>> 2016-07-11  Jerry DeLisle  
>>>
>>> PR fortran/66310
>>> * simplify.c (gfc_simplify_repeat): Set max repeat to huge - 1 to allow
>>> one byte for null terminating the resulting string constant.
>>
>> OK, thanks
> Please hold on. I still see several problem with the patch applied. One is
> 
>program p
>   character :: z = 'z'
>   print *, repeat(z, huge(1)-2**9)
>end

Please test this revised patch. See my comments in the PR.

I think we should commit this one.

Jerry
diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h
index 77831ab..dcacae8 100644
--- a/gcc/fortran/gfortran.h
+++ b/gcc/fortran/gfortran.h
@@ -59,6 +59,8 @@ not after.
 
 #define MAX_SUBRECORD_LENGTH 2147483639   /* 2**31-9 */
 
+/* Practical limit on size of character constants.  */
+#define MAX_CHAR_LENGTH 268435455/* 2**28-1  */
 
 #define gfc_is_whitespace(c) ((c==' ') || (c=='\t'))
 
diff --git a/gcc/fortran/simplify.c b/gcc/fortran/simplify.c
index 8096a92..4010753 100644
--- a/gcc/fortran/simplify.c
+++ b/gcc/fortran/simplify.c
@@ -5085,24 +5085,29 @@ gfc_simplify_repeat (gfc_expr *e, gfc_expr *n)
   /* Check that NCOPIES isn't too large.  */
   if (len)
 {
-  mpz_t max, mlen;
+  mpz_t max, mlen, limit;
   int i;
 
-  /* Compute the maximum value allowed for NCOPIES: huge(cl) / len.  */
+  /* Compute the maximum value allowed for NCOPIES:
+	huge(cl) - 1 / len.  */
   mpz_init (max);
   i = gfc_validate_kind (BT_INTEGER, gfc_charlen_int_kind, false);
 
+  /* Set the limit on size to avoid unsigned integer
+	 wrapping and resource limits.  */
+  mpz_init_set_si (limit, MAX_CHAR_LENGTH);
+
   if (have_length)
 	{
-	  mpz_tdiv_q (max, gfc_integer_kinds[i].huge,
-		  e->ts.u.cl->length->value.integer);
+	  mpz_tdiv_q (max, limit, e->ts.u.cl->length->value.integer);
 	}
   else
 	{
 	  mpz_init_set_si (mlen, len);
-	  mpz_tdiv_q (max, gfc_integer_kinds[i].huge, mlen);
+	  mpz_tdiv_q (max, limit, mlen);
 	  mpz_clear (mlen);
 	}
+  mpz_clear (limit);
 
   /* The check itself.  */
   if (mpz_cmp (ncopies, max) > 0)
diff --git a/gcc/testsuite/gfortran.dg/repeat_4.f90 b/gcc/testsuite/gfortran.dg/repeat_4.f90
index e5b5acc..a6ee75b 100644
--- a/gcc/testsuite/gfortran.dg/repeat_4.f90
+++ b/gcc/testsuite/gfortran.dg/repeat_4.f90
@@ -22,17 +22,17 @@ program test
 
   ! Check for too large NCOPIES argument and limit cases
   print *, repeat(t0, huge(0))
-  print *, repeat(t1, huge(0))
+  print *, repeat(t1, huge(0)) ! { dg-error "Argument NCOPIES of REPEAT intrinsic is too large " }
   print *, repeat(t2, huge(0)) ! { dg-error "Argument NCOPIES of REPEAT intrinsic is too large " }
   print *, repeat(s2, huge(0)) ! { dg-error "Argument NCOPIES of REPEAT intrinsic is too large " }
 
-  print *, repeat(t0, huge(0)/2)
-  print *, repeat(t1, huge(0)/2)
-  print *, repeat(t2, huge(0)/2)
+  print *, repeat(t0, huge(0)/8)
+  print *, repeat(t1, huge(0)/8)
+  print *, repeat(t2, huge(0)/16)
 
   print *, repeat(t0, huge(0)/2+1)
-  print *, repeat(t1, huge(0)/2+1)
-  print *, repeat(t2, huge(0)/2+1) ! { dg-error "Argument NCOPIES of REPEAT intrinsic is too large " }
-  print *, repeat(s2, huge(0)/2+1) ! { dg-error "Argument NCOPIES of REPEAT intrinsic is too large " }
+  print *, repeat(t1, huge(0)/8+1) ! { dg-error "Argument NCOPIES of REPEAT intrinsic is too large " }
+  print *, repeat(t2, huge(0)/16+1)! { dg-error "Argument NCOPIES of REPEAT intrinsic is too large " }
+  print *, repeat(s2, huge(0)/16+1)! { dg-error "Argument NCOPIES of REPEAT intrinsic is too large " }
 
 end program test

Re: [patch, fortran] PR66310 Problems with intrinsic repeat for large number of copies

[William Clodius]

A minor question. Why with Fortran’s counted strings are you appending a null 
character?

> On Jul 11, 2016, at 6:54 PM, Jerry DeLisle  wrote:
> 
> Attached patch sets a limit of huge-1 on the character count to avoid the
> integer wrap.
> 
> Regression tested with adjustment to test case.
> 
> OK for trunk?
> 
> Regards,
> 
> Jerry
> 
> 2016-07-11  Jerry DeLisle  
> 
>   PR fortran/66310
>   * simplify.c (gfc_simplify_repeat): Set max repeat to huge - 1 to allow
>   one byte for null terminating the resulting string constant.
> 
> diff --git a/gcc/fortran/simplify.c b/gcc/fortran/simplify.c
> index 95a8d10..03257ec 100644
> --- a/gcc/fortran/simplify.c
> +++ b/gcc/fortran/simplify.c
> @@ -5081,22 +5081,27 @@ gfc_simplify_repeat (gfc_expr *e, gfc_expr *n)
>   /* Check that NCOPIES isn't too large.  */
>   if (len)
> {
> -  mpz_t max, mlen;
> +  mpz_t max, mlen, limit;
>   int i;
> 
> -  /* Compute the maximum value allowed for NCOPIES: huge(cl) / len.  */
> +  /* Compute the maximum value allowed for NCOPIES:
> +   huge(cl) - 1 / len.  */
>   mpz_init (max);
> +  mpz_init (limit);
>   i = gfc_validate_kind (BT_INTEGER, gfc_charlen_int_kind, false);
> 
> +  /* Set the limit on size to huge-1 to avoid unsigned integer
> +wrapping in gfc_get_character_expr.  */
> +  mpz_sub_ui (limit, gfc_integer_kinds[i].huge, 1);
> +
>   if (have_length)
>{
> - mpz_tdiv_q (max, gfc_integer_kinds[i].huge,
> - e->ts.u.cl->length->value.integer);
> + mpz_tdiv_q (max, limit, e->ts.u.cl->length->value.integer);
>}
>   else
>{
>  mpz_init_set_si (mlen, len);
> - mpz_tdiv_q (max, gfc_integer_kinds[i].huge, mlen);
> + mpz_tdiv_q (max, limit, mlen);
>  mpz_clear (mlen);
>}
> 
> @@ -5104,6 +5109,7 @@ gfc_simplify_repeat (gfc_expr *e, gfc_expr *n)
>   if (mpz_cmp (ncopies, max) > 0)
>{
>  mpz_clear (max);
> + mpz_clear (limit);
>  mpz_clear (ncopies);
>  gfc_error ("Argument NCOPIES of REPEAT intrinsic is too large at %L",
> >where);
> @@ -5111,6 +5117,7 @@ gfc_simplify_repeat (gfc_expr *e, gfc_expr *n)
>}
> 
>   mpz_clear (max);
> +  mpz_clear (limit);
> }
>   mpz_clear (ncopies);
> 
> diff --git a/gcc/testsuite/gfortran.dg/repeat_4.f90
> b/gcc/testsuite/gfortran.dg/repeat_4.f90
> index e5b5acc..77483a1 100644
> --- a/gcc/testsuite/gfortran.dg/repeat_4.f90
> +++ b/gcc/testsuite/gfortran.dg/repeat_4.f90
> @@ -22,7 +22,8 @@ program test
> 
>   ! Check for too large NCOPIES argument and limit cases
>   print *, repeat(t0, huge(0))
> -  print *, repeat(t1, huge(0))
> +  print *, repeat(t1, huge(0)) ! { dg-error "Argument NCOPIES of REPEAT
> intrinsic is too large " }
> +  print *, repeat(t1, huge(0) - 1)
>   print *, repeat(t2, huge(0)) ! { dg-error "Argument NCOPIES of REPEAT
> intrinsic is too large " }
>   print *, repeat(s2, huge(0)) ! { dg-error "Argument NCOPIES of REPEAT
> intrinsic is too large " }
> 

Re: [patch, fortran] PR66310 Problems with intrinsic repeat for large number of copies

[Jerry DeLisle]

On 07/12/2016 06:26 AM, Dominique d'Humières wrote:
>>> 2016-07-11  Jerry DeLisle  
>>>
>>> PR fortran/66310
>>> * simplify.c (gfc_simplify_repeat): Set max repeat to huge - 1 to allow
>>> one byte for null terminating the resulting string constant.
>>
>> OK, thanks
> Please hold on. I still see several problem with the patch applied. One is
> 
>program p
>   character :: z = 'z'
>   print *, repeat(z, huge(1)-2**9)
>end
> 
> a.out(67209,0x7fff77e0b000) malloc: *** 
> mach_vm_map(size=18446744071562067968) failed (error code=3)
> *** error: can't allocate region
> *** set a breakpoint in malloc_error_break to debug
> Operating system error: Cannot allocate memory
> Memory allocation failure in realloc
> 
> on x86_64-apple-darwin15.5 with/without the patch. print *, repeat(z, 
> huge(1)-2**9-1) "works".
> 
> I’ll try to report the other problems in the PR later today.
> 
> Dominique
> 
> 

I am holding. On my system I get:

Operating system error: Cannot allocate memory
Memory allocation failure in xrealloc

Which is what I expect.  The error is in trying to allocate the buffer in
write_blockm which it simply can not do.  If you think about it, the numbers are
hugely ridiculous.

Can you try ncopies = (huge(1)-1)/4 and see what you get?

Jerry

Re: [patch, fortran] PR66310 Problems with intrinsic repeat for large number of copies

[Dominique d'Humières]

> > 2016-07-11  Jerry DeLisle  
> > 
> > PR fortran/66310
> > * simplify.c (gfc_simplify_repeat): Set max repeat to huge - 1 to allow
> > one byte for null terminating the resulting string constant.
>
> OK, thanks
Please hold on. I still see several problem with the patch applied. One is

   program p
  character :: z = 'z'
  print *, repeat(z, huge(1)-2**9)
   end

a.out(67209,0x7fff77e0b000) malloc: *** mach_vm_map(size=18446744071562067968) 
failed (error code=3)
*** error: can't allocate region
*** set a breakpoint in malloc_error_break to debug
Operating system error: Cannot allocate memory
Memory allocation failure in realloc

on x86_64-apple-darwin15.5 with/without the patch. print *, repeat(z, 
huge(1)-2**9-1) "works".

I’ll try to report the other problems in the PR later today.

Dominique

Re: [patch, fortran] PR66310 Problems with intrinsic repeat for large number of copies

[FX]

> 2016-07-11  Jerry DeLisle  
> 
>   PR fortran/66310
>   * simplify.c (gfc_simplify_repeat): Set max repeat to huge - 1 to allow
>   one byte for null terminating the resulting string constant.

OK, thanks