[Fortran] PR92896 [10 Regression] Fix - Prevent character conversion in array constructor

Message ID ca92ab63-c072-ab69-66b5-38512208c6fa@codethink.co.uk
State New
Headers show
Series
  • [Fortran] PR92896 [10 Regression] Fix - Prevent character conversion in array constructor
Related show

Commit Message

Mark Eggleston Dec. 17, 2019, 3:41 p.m.
Prevent conversion of character data in array constructors.

Fix for PR fortran/92896 [10 Regression] [DEC] ICE in reduce_unary, at 
fortran/arith.c:1283.

This was caused by an unintended side affect of "Allow CHARACTER 
literals in assignments and data statements" (revision 277975). If the 
conversion occurs in a array constructor it is rejected.

Patch is attached.

OK for trunk?

Change logs:

gcc/fortran/ChangeLog

     Mark Eggleston  <mark.eggleston@codethink.com>

     PR fortran/92896
     * array.c (walk_array_constructor): Replace call to cfg_convert_type
     with call to gfc_convert_type_warn with new argument set to true.
     (check_element_type): Replace call to cfg_convert_type with call to
     gfc_convert_type_warn with new argument set to true.
     * gfortran.h: Add argument "array" to gfc_convert_type_warn default
     value set to false.
     *intrinsic.c (gfc_convert_type_warn): Update description of arguments.
     Add new argument to argument list. Add check for conversion to numeric
     or logical from character and array set to true, i.e. if conversion
     from character is in an array constructor reject it, goto bad.

gcc/testsuite/ChangeLog

     Mark Eggleston  <mark.eggleston@codethink.com>

     PR fortran/92896
     * gfortran.dg/no_char_conversion_in_array_constructor.f90: New test.

-- 
https://www.codethink.co.uk/privacy.html

Comments

Steve Kargl Dec. 17, 2019, 5:06 p.m. | #1
On Tue, Dec 17, 2019 at 03:41:41PM +0000, Mark Eggleston wrote:
> gcc/fortran/ChangeLog

> 

>      Mark Eggleston  <mark.eggleston@codethink.com>

> 

>      PR fortran/92896

>      * array.c (walk_array_constructor): Replace call to cfg_convert_type


s/cfg_convert_type/gfc_convert_type

>      with call to gfc_convert_type_warn with new argument set to true.

>      (check_element_type): Replace call to cfg_convert_type with call to

>      gfc_convert_type_warn with new argument set to true.

>      * gfortran.h: Add argument "array" to gfc_convert_type_warn default

>      value set to false.


Do all current uses of gfc_convert_type_warn need to be updated
to account for the new parameter?  That is, doesn't this introduce
a mismatch in the prototype and existing code?

-- 
Steve
Mark Eggleston Dec. 17, 2019, 5:28 p.m. | #2
On 17/12/2019 17:06, Steve Kargl wrote:
> On Tue, Dec 17, 2019 at 03:41:41PM +0000, Mark Eggleston wrote:

>> gcc/fortran/ChangeLog

>>

>>       Mark Eggleston  <mark.eggleston@codethink.com>

>>

>>       PR fortran/92896

>>       * array.c (walk_array_constructor): Replace call to cfg_convert_type

> s/cfg_convert_type/gfc_convert_type

>

>>       with call to gfc_convert_type_warn with new argument set to true.

>>       (check_element_type): Replace call to cfg_convert_type with call to

>>       gfc_convert_type_warn with new argument set to true.

>>       * gfortran.h: Add argument "array" to gfc_convert_type_warn default

>>       value set to false.

> Do all current uses of gfc_convert_type_warn need to be updated

> to account for the new parameter?  That is, doesn't this introduce

> a mismatch in the prototype and existing code?


I used a default value so all existing calls remain as they are and 
default to false. So no mismatch.

regards Mark

-- 
https://www.codethink.co.uk/privacy.html
Steve Kargl Dec. 17, 2019, 5:47 p.m. | #3
On Tue, Dec 17, 2019 at 05:28:05PM +0000, Mark Eggleston wrote:
> 

> On 17/12/2019 17:06, Steve Kargl wrote:

> > On Tue, Dec 17, 2019 at 03:41:41PM +0000, Mark Eggleston wrote:

> >> gcc/fortran/ChangeLog

> >>

> >>       Mark Eggleston  <mark.eggleston@codethink.com>

> >>

> >>       PR fortran/92896

> >>       * array.c (walk_array_constructor): Replace call to cfg_convert_type

> > s/cfg_convert_type/gfc_convert_type

> >

> >>       with call to gfc_convert_type_warn with new argument set to true.

> >>       (check_element_type): Replace call to cfg_convert_type with call to

> >>       gfc_convert_type_warn with new argument set to true.

> >>       * gfortran.h: Add argument "array" to gfc_convert_type_warn default

> >>       value set to false.

> > Do all current uses of gfc_convert_type_warn need to be updated

> > to account for the new parameter?  That is, doesn't this introduce

> > a mismatch in the prototype and existing code?

> 

> I used a default value so all existing calls remain as they are and 

> default to false. So no mismatch.

> 


% cat a.h
#ifndef _STDBOOL_H_
#include <stdbool.h>
#endif
float foo(int, float, bool tmp = false);
% cat a.c
#include "a.h"
void
bar(float x)
{
  int n;
  n = 1;
  x = foo(n, x);
}
% /usr/home/sgk/work/x/bin/gcc -Wall -c a.c
In file included from a.c:2:
a.h:1:32: error: expected ';', ',' or ')' before '=' token
    1 | float foo(int, float, bool tmp = false);
      |                                ^
a.c: In function 'bar':
a.c:8:7: warning: implicit declaration of function 'foo' [-Wimplicit-function-declaration]
    8 |   x = foo(n, x);
      |       ^~~

-- 
Steve
Janne Blomqvist Dec. 17, 2019, 6:04 p.m. | #4
On Tue, Dec 17, 2019 at 7:47 PM Steve Kargl
<sgk@troutmask.apl.washington.edu> wrote:
>

> On Tue, Dec 17, 2019 at 05:28:05PM +0000, Mark Eggleston wrote:

> >

> > On 17/12/2019 17:06, Steve Kargl wrote:

> > > On Tue, Dec 17, 2019 at 03:41:41PM +0000, Mark Eggleston wrote:

> > >> gcc/fortran/ChangeLog

> > >>

> > >>       Mark Eggleston  <mark.eggleston@codethink.com>

> > >>

> > >>       PR fortran/92896

> > >>       * array.c (walk_array_constructor): Replace call to cfg_convert_type

> > > s/cfg_convert_type/gfc_convert_type

> > >

> > >>       with call to gfc_convert_type_warn with new argument set to true.

> > >>       (check_element_type): Replace call to cfg_convert_type with call to

> > >>       gfc_convert_type_warn with new argument set to true.

> > >>       * gfortran.h: Add argument "array" to gfc_convert_type_warn default

> > >>       value set to false.

> > > Do all current uses of gfc_convert_type_warn need to be updated

> > > to account for the new parameter?  That is, doesn't this introduce

> > > a mismatch in the prototype and existing code?

> >

> > I used a default value so all existing calls remain as they are and

> > default to false. So no mismatch.

> >

>

> % cat a.h

> #ifndef _STDBOOL_H_

> #include <stdbool.h>

> #endif

> float foo(int, float, bool tmp = false);

> % cat a.c

> #include "a.h"

> void

> bar(float x)

> {

>   int n;

>   n = 1;

>   x = foo(n, x);

> }

> % /usr/home/sgk/work/x/bin/gcc -Wall -c a.c

> In file included from a.c:2:

> a.h:1:32: error: expected ';', ',' or ')' before '=' token

>     1 | float foo(int, float, bool tmp = false);

>       |                                ^

> a.c: In function 'bar':

> a.c:8:7: warning: implicit declaration of function 'foo' [-Wimplicit-function-declaration]

>     8 |   x = foo(n, x);

>       |       ^~~


Well, frontends are nowadays C++, so

a) No need to include stdbool.h, bool is a builtin type.

b) optional arguments are a thing (they are also used elsewhere in the
Fortran frontend).


-- 
Janne Blomqvist
Steve Kargl Dec. 17, 2019, 6:11 p.m. | #5
On Tue, Dec 17, 2019 at 08:04:30PM +0200, Janne Blomqvist wrote:
> 

> Well, frontends are nowadays C++, so

> 

> a) No need to include stdbool.h, bool is a builtin type.

> 

> b) optional arguments are a thing (they are also used elsewhere in the

> Fortran frontend).

> 


Ah, yes.  The creep of C++ into gfortran code continues.

-- 
Steve
"All Things Must Pass", G. Harrison
Mark Eggleston Dec. 18, 2019, 8:36 a.m. | #6
On 17/12/2019 17:47, Steve Kargl wrote:
> On Tue, Dec 17, 2019 at 05:28:05PM +0000, Mark Eggleston wrote:

>> On 17/12/2019 17:06, Steve Kargl wrote:

>>> On Tue, Dec 17, 2019 at 03:41:41PM +0000, Mark Eggleston wrote:

>>>> gcc/fortran/ChangeLog

>>>>

>>>>        Mark Eggleston  <mark.eggleston@codethink.com>

>>>>

>>>>        PR fortran/92896

>>>>        * array.c (walk_array_constructor): Replace call to cfg_convert_type

>>> s/cfg_convert_type/gfc_convert_type

>>>

>>>>        with call to gfc_convert_type_warn with new argument set to true.

>>>>        (check_element_type): Replace call to cfg_convert_type with call to

>>>>        gfc_convert_type_warn with new argument set to true.

>>>>        * gfortran.h: Add argument "array" to gfc_convert_type_warn default

>>>>        value set to false.

>>> Do all current uses of gfc_convert_type_warn need to be updated

>>> to account for the new parameter?  That is, doesn't this introduce

>>> a mismatch in the prototype and existing code?

>> I used a default value so all existing calls remain as they are and

>> default to false. So no mismatch.

>>

> % cat a.h

> #ifndef _STDBOOL_H_

> #include <stdbool.h>

> #endif

> float foo(int, float, bool tmp = false);

> % cat a.c

> #include "a.h"

> void

> bar(float x)

> {

>    int n;

>    n = 1;

>    x = foo(n, x);

> }

> % /usr/home/sgk/work/x/bin/gcc -Wall -c a.c

> In file included from a.c:2:

> a.h:1:32: error: expected ';', ',' or ')' before '=' token

>      1 | float foo(int, float, bool tmp = false);

>        |                                ^

> a.c: In function 'bar':

> a.c:8:7: warning: implicit declaration of function 'foo' [-Wimplicit-function-declaration]

>      8 |   x = foo(n, x);

>        |       ^~~

>

That's compiled with the C compiler, as I understand it the Fortran FE 
is in written in C++ and compiled using the C++ compiler even though the 
file extensions are .c.

regards,

Mark

-- 
https://www.codethink.co.uk/privacy.html
Mark Eggleston Dec. 18, 2019, 8:47 a.m. | #7
On 17/12/2019 18:04, Janne Blomqvist wrote:
> On Tue, Dec 17, 2019 at 7:47 PM Steve Kargl

> <sgk@troutmask.apl.washington.edu> wrote:

>> On Tue, Dec 17, 2019 at 05:28:05PM +0000, Mark Eggleston wrote:

>>> On 17/12/2019 17:06, Steve Kargl wrote:

>>>> On Tue, Dec 17, 2019 at 03:41:41PM +0000, Mark Eggleston wrote:

>>>>> gcc/fortran/ChangeLog

>>>>>

>>>>>        Mark Eggleston  <mark.eggleston@codethink.com>

>>>>>

>>>>>        PR fortran/92896

>>>>>        * array.c (walk_array_constructor): Replace call to cfg_convert_type

>>>> s/cfg_convert_type/gfc_convert_type

>>>>

>>>>>        with call to gfc_convert_type_warn with new argument set to true.

>>>>>        (check_element_type): Replace call to cfg_convert_type with call to

>>>>>        gfc_convert_type_warn with new argument set to true.

>>>>>        * gfortran.h: Add argument "array" to gfc_convert_type_warn default

>>>>>        value set to false.

>>>> Do all current uses of gfc_convert_type_warn need to be updated

>>>> to account for the new parameter?  That is, doesn't this introduce

>>>> a mismatch in the prototype and existing code?

>>> I used a default value so all existing calls remain as they are and

>>> default to false. So no mismatch.

>>>

>> % cat a.h

>> #ifndef _STDBOOL_H_

>> #include <stdbool.h>

>> #endif

>> float foo(int, float, bool tmp = false);

>> % cat a.c

>> #include "a.h"

>> void

>> bar(float x)

>> {

>>    int n;

>>    n = 1;

>>    x = foo(n, x);

>> }

>> % /usr/home/sgk/work/x/bin/gcc -Wall -c a.c

>> In file included from a.c:2:

>> a.h:1:32: error: expected ';', ',' or ')' before '=' token

>>      1 | float foo(int, float, bool tmp = false);

>>        |                                ^

>> a.c: In function 'bar':

>> a.c:8:7: warning: implicit declaration of function 'foo' [-Wimplicit-function-declaration]

>>      8 |   x = foo(n, x);

>>        |       ^~~

> Well, frontends are nowadays C++, so

>

> a) No need to include stdbool.h, bool is a builtin type.

>

> b) optional arguments are a thing (they are also used elsewhere in the

> Fortran frontend).

>

It is a bit confusing that the Fortran FE source files have the .c 
extension implying C when they are C++ and are compiled using C++.

After that aside back to the original question, OK to commit?

regards,

Mark

-- 
https://www.codethink.co.uk/privacy.html
Steve Kargl Dec. 18, 2019, 7:14 p.m. | #8
On Wed, Dec 18, 2019 at 08:47:31AM +0000, Mark Eggleston wrote:
>

> It is a bit confusing that the Fortran FE source files have the .c 

> extension implying C when they are C++ and are compiled using C++.


It is not puzzling at all.  gfortran was added to GCC some 15
years ago.  gfortran was originally written in C and is mostly
still written in C.

When asking people, who know Fortran, to get involved in gfortran
development, the #1 excuse why they don't ----> "I don't know C".
Now, throw C++ on top of C, and it is even harder to find new hands.
C++ creep into gfortran is, IMNSHO, a bad thing.

As to the patch, unless someone else objects, I suppose it's ok.

-- 
Steve

Patch

From f2bd94410ad637444b0014a776ada2df98859cc5 Mon Sep 17 00:00:00 2001
From: Mark Eggleston <markeggleston@codethink.com>
Date: Tue, 17 Dec 2019 13:05:46 +0000
Subject: [PATCH] Prevent conversion of character data in array constructors.

Fix for PR fortran/92896 [10 Regression] [DEC] ICE in reduce_unary, at
fortran/arith.c:1283.

This was caused by an unintended side affect of "Allow CHARACTER literals
in assignments and data statements" (revision 277975). If the conversion
occurs in a array constructor it is rejected.
---
 gcc/fortran/array.c                                       |  7 ++++---
 gcc/fortran/gfortran.h                                    |  3 ++-
 gcc/fortran/intrinsic.c                                   | 15 +++++++++++++--
 .../no_char_conversion_in_array_constructor.f90           | 10 ++++++++++
 4 files changed, 29 insertions(+), 6 deletions(-)
 create mode 100644 gcc/testsuite/gfortran.dg/no_char_conversion_in_array_constructor.f90

diff --git a/gcc/fortran/array.c b/gcc/fortran/array.c
index 36223d2233d..88ee16669bf 100644
--- a/gcc/fortran/array.c
+++ b/gcc/fortran/array.c
@@ -1189,9 +1189,10 @@  walk_array_constructor (gfc_typespec *ts, gfc_constructor_base head)
 	  if (m == MATCH_ERROR)
 	    return m;
 	}
-      else if (!gfc_convert_type (e, ts, 1) && e->ts.type != BT_UNKNOWN)
+      else if (!gfc_convert_type_warn (e, ts, 1, 1, true)
+	       && e->ts.type != BT_UNKNOWN)
 	return MATCH_ERROR;
-  }
+    }
   return MATCH_YES;
 }
 
@@ -1390,7 +1391,7 @@  check_element_type (gfc_expr *expr, bool convert)
     return 0;
 
   if (convert)
-    return gfc_convert_type(expr, &constructor_ts, 1) ? 0 : 1;
+    return gfc_convert_type_warn (expr, &constructor_ts, 1, 1, true) ? 0 : 1;
 
   gfc_error ("Element in %s array constructor at %L is %s",
 	     gfc_typename (&constructor_ts), &expr->where,
diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h
index f4a2b99bdc4..218daee0805 100644
--- a/gcc/fortran/gfortran.h
+++ b/gcc/fortran/gfortran.h
@@ -3188,7 +3188,8 @@  void gfc_intrinsic_done_1 (void);
 char gfc_type_letter (bt, bool logical_equals_int = false);
 gfc_symbol * gfc_get_intrinsic_sub_symbol (const char *);
 bool gfc_convert_type (gfc_expr *, gfc_typespec *, int);
-bool gfc_convert_type_warn (gfc_expr *, gfc_typespec *, int, int);
+bool gfc_convert_type_warn (gfc_expr *, gfc_typespec *, int, int,
+			    bool array = false);
 bool gfc_convert_chartype (gfc_expr *, gfc_typespec *);
 int gfc_generic_intrinsic (const char *);
 int gfc_specific_intrinsic (const char *);
diff --git a/gcc/fortran/intrinsic.c b/gcc/fortran/intrinsic.c
index 76b53bb7117..c913f5ab152 100644
--- a/gcc/fortran/intrinsic.c
+++ b/gcc/fortran/intrinsic.c
@@ -5096,10 +5096,15 @@  gfc_convert_type (gfc_expr *expr, gfc_typespec *ts, int eflag)
      1 Generate a gfc_error()
      2 Generate a gfc_internal_error().
 
-   'wflag' controls the warning related to conversion.  */
+   'wflag' controls the warning related to conversion.
+
+   'array' indicates whether the conversion is in an array constructor.
+   Non-standard conversion from character to numeric not allowed if true.
+*/
 
 bool
-gfc_convert_type_warn (gfc_expr *expr, gfc_typespec *ts, int eflag, int wflag)
+gfc_convert_type_warn (gfc_expr *expr, gfc_typespec *ts, int eflag, int wflag,
+		       bool array)
 {
   gfc_intrinsic_sym *sym;
   gfc_typespec from_ts;
@@ -5142,6 +5147,12 @@  gfc_convert_type_warn (gfc_expr *expr, gfc_typespec *ts, int eflag, int wflag)
       && gfc_compare_types (&expr->ts, ts))
     return true;
 
+  /* If array is true then conversion is in an array constructor where
+     non-standard conversion is not allowed.  */
+  if (array && from_ts.type == BT_CHARACTER
+      && (gfc_numeric_ts (ts) || ts->type == BT_LOGICAL))
+    goto bad;
+
   sym = find_conv (&expr->ts, ts);
   if (sym == NULL)
     goto bad;
diff --git a/gcc/testsuite/gfortran.dg/no_char_conversion_in_array_constructor.f90 b/gcc/testsuite/gfortran.dg/no_char_conversion_in_array_constructor.f90
new file mode 100644
index 00000000000..7dc4624b8b1
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/no_char_conversion_in_array_constructor.f90
@@ -0,0 +1,10 @@ 
+! { dg-do compile }
+! { dg-options "-fdec-char-conversions" }
+
+program p
+   print *, -[integer :: 1, [integer(8) :: '2']] ! { dg-error "Cannot convert" }
+   print *, -[real :: 1, [real(8) :: '2']]       ! { dg-error "Cannot convert" }
+   print *, -[complex :: 1, [complex(8) :: '2']] ! { dg-error "Cannot convert" }
+   print *, [logical :: 1, [logical(8) :: '2']]  ! { dg-error "Cannot convert" }
+end
+
-- 
2.11.0