[PATCH] PR fortran/67987 -- character lengths cannot be negative

[PATCH] PR fortran/67987 -- character lengths cannot be negative

[Steve Kargl]

[-- Attachment #1: Type: text/plain, Size: 1117 bytes --]

The attach patch enforces the Fortran Standard's requirement
that character length must be great than or equal to zero.
The fix submitted here supercedes the fix for PR fortran/31250,
which silently converted a negative string length to zero.
In removing the fix for 31250, a regression occurred, because
a substring reference where the 'start' value is larger than 
'end' value results in a zero length string.  gfortran was computing
the length, but not checking for a negative value.  This has
be fixed by actually doing the check and resetting legnth to
zero. 

Built and tested on x86_64-*-freebsd.  OK to commit? 

2015-10-16  Steven G. Kargl  <kargl@gcc.gnu.org>

	* decl.c (char_len_param_value): Check for negative character length.
	Unwrap a nearby unlong line.
	* resolve.c (gfc_resolve_substring_charlen): If a substring would have
	a negative character length, set it to zero per the Fortran standard.
	Unwrap a nearby unlong line.
	(resolve_charlen): Check for negative character length.

2015-10-16  Steven G. Kargl  <kargl@gcc.gnu.org>

	gfortran.dg/char_length_2.f90: Update the testcase.

-- 
Steve

[-- Attachment #2: pr67987.diff --]
[-- Type: text/x-diff, Size: 5113 bytes --]

Index: fortran/decl.c
===================================================================
--- fortran/decl.c	(revision 228667)
+++ fortran/decl.c	(working copy)
@@ -697,8 +697,7 @@ char_len_param_value (gfc_expr **expr, b
 
   if (gfc_match_char (':') == MATCH_YES)
     {
-      if (!gfc_notify_std (GFC_STD_F2003, "deferred type "
-			   "parameter at %C"))
+      if (!gfc_notify_std (GFC_STD_F2003, "deferred type parameter at %C"))
 	return MATCH_ERROR;
 
       *deferred = true;
@@ -708,11 +707,13 @@ char_len_param_value (gfc_expr **expr, b
 
   m = gfc_match_expr (expr);
 
-  if (m == MATCH_YES
-      && !gfc_expr_check_typed (*expr, gfc_current_ns, false))
+  if (m == MATCH_NO || m == MATCH_ERROR)
+    return m;
+
+  if (!gfc_expr_check_typed (*expr, gfc_current_ns, false))
     return MATCH_ERROR;
 
-  if (m == MATCH_YES && (*expr)->expr_type == EXPR_FUNCTION)
+  if ((*expr)->expr_type == EXPR_FUNCTION)
     {
       if ((*expr)->value.function.actual
 	  && (*expr)->value.function.actual->expr->symtree)
@@ -731,6 +732,16 @@ char_len_param_value (gfc_expr **expr, b
 	    }
 	}
     }
+
+  /* F2008, 4.4.3.1:  The length is a type parameter; its kind is processor
+     dependent and its value is greater than or equal to zero.  */
+  if ((*expr)->expr_type == EXPR_CONSTANT
+      && mpz_cmp_si ((*expr)->value.integer, 0) < 0)
+    {
+      gfc_error ("LEN at %C must be greater than or equal to 0");
+      return MATCH_ERROR;
+    }
+
   return m;
 
 syntax:
Index: fortran/resolve.c
===================================================================
--- fortran/resolve.c	(revision 228667)
+++ fortran/resolve.c	(working copy)
@@ -4562,8 +4562,7 @@ gfc_resolve_substring_charlen (gfc_expr 
     {
       if (e->ts.u.cl->length)
 	gfc_free_expr (e->ts.u.cl->length);
-      else if (e->expr_type == EXPR_VARIABLE
-		 && e->symtree->n.sym->attr.dummy)
+      else if (e->expr_type == EXPR_VARIABLE && e->symtree->n.sym->attr.dummy)
 	return;
     }
 
@@ -4596,12 +4595,19 @@ gfc_resolve_substring_charlen (gfc_expr 
       return;
     }
 
-  /* Length = (end - start +1).  */
+  /* Length = (end - start + 1).  */
   e->ts.u.cl->length = gfc_subtract (end, start);
   e->ts.u.cl->length = gfc_add (e->ts.u.cl->length,
 				gfc_get_int_expr (gfc_default_integer_kind,
 						  NULL, 1));
 
+  /* F2008, 6.4.1:  Both the starting point and the ending point shall
+     be within the range 1, 2, ..., n unless the starting point exceeds
+     the ending point, in which case the substring has length zero.  */
+
+  if (mpz_cmp_si (e->ts.u.cl->length->value.integer, 0) < 0)
+    mpz_set_si (e->ts.u.cl->length->value.integer, 0);
+
   e->ts.u.cl->length->ts.type = BT_INTEGER;
   e->ts.u.cl->length->ts.kind = gfc_charlen_int_kind;
 
@@ -10882,17 +10888,12 @@ resolve_charlen (gfc_charlen *cl)
 	}
     }
 
-  /* "If the character length parameter value evaluates to a negative
-     value, the length of character entities declared is zero."  */
   if (cl->length && !gfc_extract_int (cl->length, &i) && i < 0)
     {
-      if (warn_surprising)
-	gfc_warning_now (OPT_Wsurprising,
-			 "CHARACTER variable at %L has negative length %d,"
-			 " the length has been set to zero",
-			 &cl->length->where, i);
-      gfc_replace_expr (cl->length,
-			gfc_get_int_expr (gfc_default_integer_kind, NULL, 0));
+      gfc_error ("LEN at %L must be greater than or equal to 0", 
+		 &cl->length->where);
+      specification_expr = saved_specification_expr;
+      return false;
     }
 
   /* Check that the character length is not too large.  */
Index: testsuite/gfortran.dg/char_length_2.f90
===================================================================
--- testsuite/gfortran.dg/char_length_2.f90	(revision 228667)
+++ testsuite/gfortran.dg/char_length_2.f90	(working copy)
@@ -1,22 +1,12 @@
-! { dg-do link }
-! { dg-options "-Wsurprising" }
-! Tests the fix for PR 31250
-! CHARACTER lengths weren't reduced early enough for all checks of
-! them to be meaningful.  Furthermore negative string lengths weren't
-! dealt with correctly.
-CHARACTER(len=0) :: c1   ! This is OK.
-CHARACTER(len=-1) :: c2  ! { dg-warning "has negative length" }
+! { dg-do compile }
+! Original test came in with fix for PR 31250.
+! The fix for PR fortran/67987 supercedes 31250.
+!
 PARAMETER(I=-100)
-CHARACTER(len=I) :: c3   ! { dg-warning "has negative length" }
-CHARACTER(len=min(I,500)) :: c4  ! { dg-warning "has negative length" }
+CHARACTER(len=0) :: c1           ! This is OK.
+CHARACTER(len=-1) :: c2          ! { dg-error "must be greater than" }
+CHARACTER(len=I) :: c3           ! { dg-error "must be greater than" }
+CHARACTER(len=min(I,500)) :: c4  ! { dg-error "must be greater than" }
 CHARACTER(len=max(I,500)) :: d1  ! no warning
-CHARACTER(len=5) :: d2   ! no warning
-
-if (len(c1) .ne. 0) call link_error ()
-if (len(c2) .ne. len(c1)) call link_error ()
-if (len(c3) .ne. len(c2)) call link_error ()
-if (len(c4) .ne. len(c3)) call link_error ()
-
-if (len(d1) .ne. 500) call link_error ()
-if (len(d2) .ne. 5) call link_error ()
+CHARACTER(len=5) :: d2           ! no warning
 END

Re: [PATCH] PR fortran/67987 -- character lengths cannot be negative

[FX]

> The attach patch enforces the Fortran Standard's requirement
> that character length must be great than or equal to zero.

We've got to be careful about this. The standard (F2008) has this to say about character lengths:

4.4.3.1. "The number of characters in the string is called the length of the string. The length is a type parameter; its kind is processor dependent and its value is greater than or equal to zero.”

but

4.4.3.2. "If the character length parameter value evaluates to a negative value, the length of character entities declared is zero."


So while strings cannot have negative length, they can be declared with a length parameter value which is itself negative, leading to the string having zero length. Or, said otherwise:

  character(len=-2) :: s

is legal and declares a string of zero length, being thus equivalent to:

  character(len=0) :: s


Thus: not OK to commit.

FX

Re: [PATCH] PR fortran/67987 -- character lengths cannot be negative

[Steve Kargl]

[-- Attachment #1: Type: text/plain, Size: 1036 bytes --]

On Fri, Oct 16, 2015 at 10:17:34PM +0200, FX wrote:
> > The attach patch enforces the Fortran Standard's requirement
> > that character length must be great than or equal to zero.
> 
> 4.4.3.2. "If the character length parameter value evaluates to
> a negative value, the length of character entities declared is zero."
> 

Thanks.  I missed the above.  The above text goes back to 
at least F90 (see p. 41 on N693.pdf).  New diff attached.

2015-10-16  Steven G. Kargl  <kargl@gcc.gnu.org>

	PR fortran/67987
	* decl.c (char_len_param_value): Unwrap unlong line.  If LEN < 0,
	then force it to zero pre Fortran Standards. 
	* resolve.c (gfc_resolve_substring_charlen): Unwrap unlong line.
	If 'start' is larger than 'end', then length of string is negative,
	so explicitly set it to zero.
	(resolve_charlen): Remove -Wsurprising warning.  Update comment to
	text from F2008 standard.

2015-10-16  Steven G. Kargl  <kargl@gcc.gnu.org>

	PR fortran/67987
	* gfortran.dg/char_length_2.f90: Add declaration from PR to testcase.

-- 
Steve

[-- Attachment #2: pr67987.diff --]
[-- Type: text/x-diff, Size: 5173 bytes --]

Index: fortran/decl.c
===================================================================
--- fortran/decl.c	(revision 228667)
+++ fortran/decl.c	(working copy)
@@ -697,8 +697,7 @@ char_len_param_value (gfc_expr **expr, b
 
   if (gfc_match_char (':') == MATCH_YES)
     {
-      if (!gfc_notify_std (GFC_STD_F2003, "deferred type "
-			   "parameter at %C"))
+      if (!gfc_notify_std (GFC_STD_F2003, "deferred type parameter at %C"))
 	return MATCH_ERROR;
 
       *deferred = true;
@@ -708,11 +707,13 @@ char_len_param_value (gfc_expr **expr, b
 
   m = gfc_match_expr (expr);
 
-  if (m == MATCH_YES
-      && !gfc_expr_check_typed (*expr, gfc_current_ns, false))
+  if (m == MATCH_NO || m == MATCH_ERROR)
+    return m;
+
+  if (!gfc_expr_check_typed (*expr, gfc_current_ns, false))
     return MATCH_ERROR;
 
-  if (m == MATCH_YES && (*expr)->expr_type == EXPR_FUNCTION)
+  if ((*expr)->expr_type == EXPR_FUNCTION)
     {
       if ((*expr)->value.function.actual
 	  && (*expr)->value.function.actual->expr->symtree)
@@ -731,6 +732,15 @@ char_len_param_value (gfc_expr **expr, b
 	    }
 	}
     }
+
+  /* F2008, 4.4.3.1:  The length is a type parameter; its kind is processor
+     dependent and its value is greater than or equal to zero.
+     F2008, 4.4.3.2:  If the character length parameter value evaluates to
+     a negative value, the length of character entities declared is zero.  */
+  if ((*expr)->expr_type == EXPR_CONSTANT
+      && mpz_cmp_si ((*expr)->value.integer, 0) < 0)
+    mpz_set_si ((*expr)->value.integer, 0);
+
   return m;
 
 syntax:
Index: fortran/resolve.c
===================================================================
--- fortran/resolve.c	(revision 228667)
+++ fortran/resolve.c	(working copy)
@@ -4562,8 +4562,7 @@ gfc_resolve_substring_charlen (gfc_expr 
     {
       if (e->ts.u.cl->length)
 	gfc_free_expr (e->ts.u.cl->length);
-      else if (e->expr_type == EXPR_VARIABLE
-		 && e->symtree->n.sym->attr.dummy)
+      else if (e->expr_type == EXPR_VARIABLE && e->symtree->n.sym->attr.dummy)
 	return;
     }
 
@@ -4596,12 +4595,19 @@ gfc_resolve_substring_charlen (gfc_expr 
       return;
     }
 
-  /* Length = (end - start +1).  */
+  /* Length = (end - start + 1).  */
   e->ts.u.cl->length = gfc_subtract (end, start);
   e->ts.u.cl->length = gfc_add (e->ts.u.cl->length,
 				gfc_get_int_expr (gfc_default_integer_kind,
 						  NULL, 1));
 
+  /* F2008, 6.4.1:  Both the starting point and the ending point shall
+     be within the range 1, 2, ..., n unless the starting point exceeds
+     the ending point, in which case the substring has length zero.  */
+
+  if (mpz_cmp_si (e->ts.u.cl->length->value.integer, 0) < 0)
+    mpz_set_si (e->ts.u.cl->length->value.integer, 0);
+
   e->ts.u.cl->length->ts.type = BT_INTEGER;
   e->ts.u.cl->length->ts.kind = gfc_charlen_int_kind;
 
@@ -10882,18 +10888,11 @@ resolve_charlen (gfc_charlen *cl)
 	}
     }
 
-  /* "If the character length parameter value evaluates to a negative
-     value, the length of character entities declared is zero."  */
+  /* F2008, 4.4.3.2:  If the character length parameter value evaluates to
+     a negative value, the length of character entities declared is zero.  */
   if (cl->length && !gfc_extract_int (cl->length, &i) && i < 0)
-    {
-      if (warn_surprising)
-	gfc_warning_now (OPT_Wsurprising,
-			 "CHARACTER variable at %L has negative length %d,"
-			 " the length has been set to zero",
-			 &cl->length->where, i);
-      gfc_replace_expr (cl->length,
+    gfc_replace_expr (cl->length,
 			gfc_get_int_expr (gfc_default_integer_kind, NULL, 0));
-    }
 
   /* Check that the character length is not too large.  */
   k = gfc_validate_kind (BT_INTEGER, gfc_charlen_int_kind, false);
Index: testsuite/gfortran.dg/char_length_2.f90
===================================================================
--- testsuite/gfortran.dg/char_length_2.f90	(revision 228667)
+++ testsuite/gfortran.dg/char_length_2.f90	(working copy)
@@ -1,22 +1,13 @@
-! { dg-do link }
-! { dg-options "-Wsurprising" }
-! Tests the fix for PR 31250
-! CHARACTER lengths weren't reduced early enough for all checks of
-! them to be meaningful.  Furthermore negative string lengths weren't
-! dealt with correctly.
-CHARACTER(len=0) :: c1   ! This is OK.
-CHARACTER(len=-1) :: c2  ! { dg-warning "has negative length" }
+! { dg-do compile }
+! Original test came in with fix for PR 31250.
+! The last line tests the fix for PR fortran/67987.
+!
 PARAMETER(I=-100)
-CHARACTER(len=I) :: c3   ! { dg-warning "has negative length" }
-CHARACTER(len=min(I,500)) :: c4  ! { dg-warning "has negative length" }
-CHARACTER(len=max(I,500)) :: d1  ! no warning
-CHARACTER(len=5) :: d2   ! no warning
-
-if (len(c1) .ne. 0) call link_error ()
-if (len(c2) .ne. len(c1)) call link_error ()
-if (len(c3) .ne. len(c2)) call link_error ()
-if (len(c4) .ne. len(c3)) call link_error ()
-
-if (len(d1) .ne. 500) call link_error ()
-if (len(d2) .ne. 5) call link_error ()
+CHARACTER(len=0) :: c1
+CHARACTER(len=-1) :: c2
+CHARACTER(len=I) :: c3
+CHARACTER(len=min(I,500)) :: c4
+CHARACTER(len=max(I,500)) :: d1
+CHARACTER(len=5) :: d2
+character(-8) :: c = ' '
 END

Re: [PATCH] PR fortran/67987 -- character lengths cannot be negative

[FX]

> 2015-10-16  Steven G. Kargl  <kargl@gcc.gnu.org>
> 
> 	PR fortran/67987
> 	* decl.c (char_len_param_value): Unwrap unlong line.  If LEN < 0,
> 	then force it to zero pre Fortran Standards. 
> 	* resolve.c (gfc_resolve_substring_charlen): Unwrap unlong line.
> 	If 'start' is larger than 'end', then length of string is negative,
> 	so explicitly set it to zero.
> 	(resolve_charlen): Remove -Wsurprising warning.  Update comment to
> 	text from F2008 standard.
> 
> 2015-10-16  Steven G. Kargl  <kargl@gcc.gnu.org>
> 
> 	PR fortran/67987
> 	* gfortran.dg/char_length_2.f90: Add declaration from PR to testcase.

The patch is now mostly OK to me. Minor remarks:

  - I’m thinking you mean “force it to zero per [not pre] Fortran standards”
  - why remove the -Wsurprising warning? it seems a good case for -Wsurprising: legal code, but dubious anyway

OK after you ponder that second point.

FX

Re: [PATCH] PR fortran/67987 -- character lengths cannot be negative

[Steve Kargl]

On Fri, Oct 16, 2015 at 11:39:51PM +0200, FX wrote:
> > 2015-10-16  Steven G. Kargl  <kargl@gcc.gnu.org>
> > 
> > 	PR fortran/67987
> > 	* decl.c (char_len_param_value): Unwrap unlong line.  If LEN < 0,
> > 	then force it to zero pre Fortran Standards. 
> > 	* resolve.c (gfc_resolve_substring_charlen): Unwrap unlong line.
> > 	If 'start' is larger than 'end', then length of string is negative,
> > 	so explicitly set it to zero.
> > 	(resolve_charlen): Remove -Wsurprising warning.  Update comment to
> > 	text from F2008 standard.
> > 
> > 2015-10-16  Steven G. Kargl  <kargl@gcc.gnu.org>
> > 
> > 	PR fortran/67987
> > 	* gfortran.dg/char_length_2.f90: Add declaration from PR to testcase.
> 
> The patch is now mostly OK to me. Minor remarks:
> 
>   - I???m thinking you mean ???force it to zero per [not pre] Fortran standards???

yep.  Noticed that I was reading the fortran@ message it sent.

>   - why remove the -Wsurprising warning? it seems a good case for -Wsurprising: legal code, but dubious anyway
> 
> OK after you ponder that second point.
> 

F90 is over 26 years old.  There has been 3 major revisions that
have superceded F90 (F95, F03, and F08).  All of those revisions
include the text that you pointed out to me.  Why is it surprising
that a compiler conforms to the standard?  

"Simplify, simplify, simplify."  Henry David Thoreau

-- 
Steve

Re: [PATCH] PR fortran/67987 -- character lengths cannot be negative

[Steve Kargl]

On Fri, Oct 16, 2015 at 02:55:27PM -0700, Steve Kargl wrote:
> On Fri, Oct 16, 2015 at 11:39:51PM +0200, FX wrote:
> 
> >   - why remove the -Wsurprising warning? it seems a good case
> > for -Wsurprising: legal code, but dubious anyway
> > 
> > OK after you ponder that second point.
> > 
> 
> F90 is over 26 years old.  There has been 3 major revisions that
> have superceded F90 (F95, F03, and F08).  All of those revisions
> include the text that you pointed out to me.  Why is it surprising
> that a compiler conforms to the standard?  
> 
> "Simplify, simplify, simplify."  Henry David Thoreau
> 

Another reason to remove it.  It is no longer reached
for the first 2 of the 3 dg-warnings in char_length_2.f90. 

% head -13 char_length_2.f90

! { dg-do link }
! { dg-options "-Wsurprising" }
! Tests the fix for PR 31250
! CHARACTER lengths weren't reduced early enough for all checks of
! them to be meaningful.  Furthermore negative string lengths weren't
! dealt with correctly.
CHARACTER(len=0) :: c1   ! This is OK.
CHARACTER(len=-1) :: c2  ! { dg-warning "has negative length" }
PARAMETER(I=-100)
CHARACTER(len=I) :: c3   ! { dg-warning "has negative length" }
CHARACTER(len=min(I,500)) :: c4  ! { dg-warning "has negative length" }
CHARACTER(len=max(I,500)) :: d1  ! no warning
CHARACTER(len=5) :: d2   ! no warning

-- 
Steve

Re: [PATCH] PR fortran/67987 -- character lengths cannot be negative

[FX]

> F90 is over 26 years old.  There has been 3 major revisions that
> have superceded F90 (F95, F03, and F08).  All of those revisions
> include the text that you pointed out to me.  Why is it surprising
> that a compiler conforms to the standard?  
> 
> "Simplify, simplify, simplify."  Henry David Thoreau

OK, go ahead.