[patch, Fortran] Fix PR 60526, variable name has already been declared as a type

[patch, Fortran] Fix PR 60526, variable name has already been declared as a type

[Thomas Koenig]

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

Hello world,

For a type 'foo', we use a symtree 'Foo'. This led to accept-invalid
when a variable name was already declared as a type.  This rather
self-explanatory patch fixes that.

Regression-tested.  OK for trunk and 5?  (Do we still care about 4.9?)

Regards

	Thomas


2016-02-03  Thomas Koenig  <tkoenig@gcc.gnu.org>

         PR fortran/60526
         * decl.c (build_sym):  If the name has already been defined as a
         type, issue error and return false.

2016-02-03  Thomas Koenig  <tkoenig@gcc.gnu.org>

         PR fortran/60526
         * gfortran.dg/type_decl_4.f90:  New test.

[-- Attachment #2: p2.diff --]
[-- Type: text/x-patch, Size: 1039 bytes --]

Index: decl.c
===================================================================
--- decl.c	(Revision 232864)
+++ decl.c	(Arbeitskopie)
@@ -1215,10 +1215,32 @@ build_sym (const char *name, gfc_charlen *cl, bool
 {
   symbol_attribute attr;
   gfc_symbol *sym;
+  char *u_name;
+  int nlen;
+  gfc_symtree *st;
 
   if (gfc_get_symbol (name, NULL, &sym))
     return false;
 
+  /* Check if the name has already been defined as a type.  The
+     first letter of the symtree will be in upper case then.  */
+
+  nlen = strlen(name);
+
+  u_name = XCNEWVEC(char, nlen+1);
+  u_name[0] = TOUPPER(name[0]);
+  strncpy (u_name+1, name+1, nlen);
+
+  st = gfc_find_symtree (gfc_current_ns->sym_root, u_name);
+  free (u_name);
+
+  if (st != 0)
+    {
+      gfc_error ("Symbol %qs at %C also declared as a type at %L", name,
+		 &st->n.sym->declared_at);
+      return false;
+    }
+
   /* Start updating the symbol table.  Add basic type attribute if present.  */
   if (current_ts.type != BT_UNKNOWN
       && (sym->attr.implicit_type == 0

[-- Attachment #3: type_decl_4.f90 --]
[-- Type: text/x-fortran, Size: 225 bytes --]

! { dg-do compile }
program main
  type Xx ! { dg-error "Symbol 'xx' at .1. also declared as a type at .2." }
  end type Xx
  real :: Xx  ! { dg-error "Symbol 'xx' at .1. also declared as a type at .2." }
  
end program main

Re: [patch, Fortran] Fix PR 60526, variable name has already been declared as a type

[Andre Vehreschild]

Hi Thomas,

please note: This is not a review. I don't have the privileges to do so.

In preventing memory clutter I like to advise the use of:

char u_name[GFC_MAX_SYMBOL_LEN + 1]; 

and safe us all the dynamic memory allocation/free. Furthermore, how
about switching:

  strncpy (u_name, name, nlen+ 1);
  u_name[0] = TOUPPER(u_name[0]);

that way strncpy() can use its full power and copy aligned data using
longs, vector instructions or whatever. At least it has the potential.
with (u_)name+1 we always have an uneven start address and I doubt
strncpy can use an optimized copy algorithm. I know, that now we copy
one byte twice, but that shouldn't really matter.

Besides my ideas above the patch and test looks ok to me (I didn't do a
regtest though).

Regards,
	Andre

On Thu, 4 Feb 2016 23:03:13 +0100
Thomas Koenig <tkoenig@netcologne.de> wrote:

> Hello world,
> 
> For a type 'foo', we use a symtree 'Foo'. This led to accept-invalid
> when a variable name was already declared as a type.  This rather
> self-explanatory patch fixes that.
> 
> Regression-tested.  OK for trunk and 5?  (Do we still care about 4.9?)
> 
> Regards
> 
> 	Thomas
> 
> 
> 2016-02-03  Thomas Koenig  <tkoenig@gcc.gnu.org>
> 
>          PR fortran/60526
>          * decl.c (build_sym):  If the name has already been defined as a
>          type, issue error and return false.
> 
> 2016-02-03  Thomas Koenig  <tkoenig@gcc.gnu.org>
> 
>          PR fortran/60526
>          * gfortran.dg/type_decl_4.f90:  New test.


-- 
Andre Vehreschild * Email: vehre ad gmx dot de 

Re: [patch, Fortran] Fix PR 60526, variable name has already been declared as a type

[Thomas Koenig]

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

Hi Andre,

> In preventing memory clutter I like to advise the use of:
>
> char u_name[GFC_MAX_SYMBOL_LEN + 1];
>
> and safe us all the dynamic memory allocation/free.

We're really talking micro-optimizations here, but well ... ;-)

In the attached patch, I have replaced this with alloca.  I was going
to  use a VLA originally, but apparently C++ doesn't like that, at least
not in the version that we use within C++.

I think this is the right idiom for a throw-away variable like this.
I also found 15 instances of alloca in fortran, so it is OK to use.

> Furthermore, how
> about switching:
>
>    strncpy (u_name, name, nlen+ 1);
>    u_name[0] = TOUPPER(u_name[0]);
>
> that way strncpy() can use its full power and copy aligned data using
> longs,

I don't think this would have mattered for an array of char
(these are usually not aligned).  However, a pointer using alloca
should be aligned, so this could be used.

So, here's the new version.

OK for all trunk and 5?



>> 2016-02-03  Thomas Koenig  <tkoenig@gcc.gnu.org>
>>
>>           PR fortran/60526
>>           * decl.c (build_sym):  If the name has already been defined as a
>>           type, issue error and return false.
>>
>> 2016-02-03  Thomas Koenig  <tkoenig@gcc.gnu.org>
>>
>>           PR fortran/60526
>>           * gfortran.dg/type_decl_4.f90:  New test.
>
>


[-- Attachment #2: p3.diff --]
[-- Type: text/x-patch, Size: 1025 bytes --]

Index: decl.c
===================================================================
--- decl.c	(Revision 232864)
+++ decl.c	(Arbeitskopie)
@@ -1215,10 +1215,30 @@ build_sym (const char *name, gfc_charlen *cl, bool
 {
   symbol_attribute attr;
   gfc_symbol *sym;
+  int nlen;
+  char *u_name;
+  gfc_symtree *st;
 
   if (gfc_get_symbol (name, NULL, &sym))
     return false;
 
+  /* Check if the name has already been defined as a type.  The
+     first letter of the symtree will be in upper case then.  */
+
+  nlen = strlen(name);
+  u_name = (char *) alloca (nlen + 1);
+  strncpy (u_name, name, nlen + 1);
+  u_name[0] = TOUPPER(u_name[0]);
+
+  st = gfc_find_symtree (gfc_current_ns->sym_root, u_name);
+
+  if (st != 0)
+    {
+      gfc_error ("Symbol %qs at %C also declared as a type at %L", name,
+		 &st->n.sym->declared_at);
+      return false;
+    }
+
   /* Start updating the symbol table.  Add basic type attribute if present.  */
   if (current_ts.type != BT_UNKNOWN
       && (sym->attr.implicit_type == 0

Re: [patch, Fortran] Fix PR 60526, variable name has already been declared as a type

[Janne Blomqvist]

On Sat, Feb 6, 2016 at 10:20 PM, Thomas Koenig <tkoenig@netcologne.de> wrote:
> Hi Andre,
>
>> In preventing memory clutter I like to advise the use of:
>>
>> char u_name[GFC_MAX_SYMBOL_LEN + 1];
>>
>> and safe us all the dynamic memory allocation/free.
>
>
> We're really talking micro-optimizations here, but well ... ;-)

/me joins the micro-optimization bikeshed!

> In the attached patch, I have replaced this with alloca.  I was going
> to  use a VLA originally, but apparently C++ doesn't like that, at least
> not in the version that we use within C++.

IIRC VLA's were proposed for C++14, but were ultimately rejected.

The usual arguments against VLA's (and alloca, which is just another
way to spell the same thing, really) are that

- It reserves an extra register for the frame pointer.

- Functions using VLA's aren't inlined by GCC

- From a correctness perspective, unless you check the size somehow
beforehand, you can easily get a stack overflow crash.

So one solution to these correctness and performance issues is to have
a reasonably small statically sized buffer on the stack, and if the
size is bigger than that, fall back to heap allocation. E.g. something
like

void foo(..., size_t n) {
  char tmp[BUFSIZE];
  char *buf;
  if (n <= BUFSIZE)
    buf = tmp;
  else
    buf = xmalloc(n);
  // Use buf
 if (n > BUFSIZE)
    free(buf);
}

This is roughly what std::string does with the new C++11 compatible
libstdc++ ABI. As we're supposed to be C++ nowadays, why aren't we
using that?



-- 
Janne Blomqvist

Re: [patch, Fortran] Fix PR 60526, variable name has already been declared as a type

[Thomas Koenig]

Hi Janne,

[std::string]

> As we're supposed to be C++ nowadays, why aren't we
> using that?

My reason is simple: I know C and Fortran. I hardly know C++,
and I don't want to learn it for the sake of some largely
irrelevant micro-optimizations.

We should restrict C++ usage to those cases where there is
a clear and large benefit.

Regards

	Thomas

Re: [patch, Fortran] Fix PR 60526, variable name has already been declared as a type

[Andre Vehreschild]

Hi all,

it wasn't my intention to start such a big discussion for such a small
thing. Please stop this academic battle and get back to the issue at
hand:

Thomas is examining a symbol, that

- has come through the scanner, i.e., it adheres to the rules of
  gfortran, especially it is known to be smaller than GFC_MAX_SYMBOL_LEN

- is still a valid symbol. When gfortran's internal
  composition/modification of symbols breaks this rule, we are done
  earlier already. 

- its length is not changed in his routine.

I was only trying to prevent an alloc/free for this small use case,
which should fit on the stack quite easily. The proposed alloca() call
has according to the documentation of libc some availability issues on
certain platforms, too. Therefore why not going with the fixed size
stack array and adding a check for possible overflow to it and be done?

Regards,
	Andre

On Fri, 12 Feb 2016 09:02:27 +0100
Thomas Koenig <tkoenig@netcologne.de> wrote:

> Hi Janne,
> 
> [std::string]
> 
> > As we're supposed to be C++ nowadays, why aren't we
> > using that?  
> 
> My reason is simple: I know C and Fortran. I hardly know C++,
> and I don't want to learn it for the sake of some largely
> irrelevant micro-optimizations.
> 
> We should restrict C++ usage to those cases where there is
> a clear and large benefit.
> 
> Regards
> 
> 	Thomas


-- 
Andre Vehreschild * Email: vehre ad gmx dot de 

Re: [patch, Fortran] Fix PR 60526, variable name has already been declared as a type

[Janne Blomqvist]

On Fri, Feb 12, 2016 at 12:16 PM, Andre Vehreschild <vehre@gmx.de> wrote:
> Hi all,
>
> it wasn't my intention to start such a big discussion for such a small
> thing. Please stop this academic battle and get back to the issue at
> hand:
>
> Thomas is examining a symbol, that
>
> - has come through the scanner, i.e., it adheres to the rules of
>   gfortran, especially it is known to be smaller than GFC_MAX_SYMBOL_LEN
>
> - is still a valid symbol. When gfortran's internal
>   composition/modification of symbols breaks this rule, we are done
>   earlier already.
>
> - its length is not changed in his routine.
>
> I was only trying to prevent an alloc/free for this small use case,
> which should fit on the stack quite easily. The proposed alloca() call
> has according to the documentation of libc some availability issues on
> certain platforms, too. Therefore why not going with the fixed size
> stack array and adding a check for possible overflow to it and be done?

Yes, I agree. That sounds like the best approach in this case.



-- 
Janne Blomqvist

Re: [patch, Fortran] Fix PR 60526, variable name has already been declared as a type

[Thomas Koenig]

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

Am 12.02.2016 um 11:42 schrieb Janne Blomqvist:
> On Fri, Feb 12, 2016 at 12:16 PM, Andre Vehreschild <vehre@gmx.de> wrote:

>> The proposed alloca() call
>> has according to the documentation of libc some availability issues on
>> certain platforms, too.

These availablity issues cannot be too serious, or we would be having
trouble already:

ig25@linux-fd1f:~/Gcc/trunk/gcc/fortran> fgrep -H -n 'alloca (' *.c
cpp.c:839:      to_file_quoted = (unsigned char *) alloca (to_file_len * 
4 + 1);
cpp.c:1079:     (unsigned char *) alloca (to_file_len * 4 + 1);
error.c:898:  buffer = (char *) alloca (strlen (msg) + strlen (msg2) + 2);
module.c:6042:      filename = (char *) alloca (n);
module.c:6048:      filename = (char *) alloca (n);
module.c:6058:  filename_tmp = (char *) alloca (n + 1);
options.c:267:      source_path = (char *) alloca (i + 1);
primary.c:214:  buffer = (char *) alloca (length + 1);
primary.c:438:  buffer = (char *) alloca (length + 1);
primary.c:600:  buffer = (char *) alloca (count + 1);
scanner.c:321:  q = (char *) alloca (len + 1);
simplify.c:6381:  buffer = (unsigned char*)alloca (buffer_size);
target-memory.c:674:  buffer = (unsigned char*)alloca (len);
target-memory.c:781:  buffer = (unsigned char*)alloca (buffer_size);

>> Therefore why not going with the fixed size
>> stack array and adding a check for possible overflow to it and be done?
>
> Yes, I agree. That sounds like the best approach in this case.

OK, here is the final version of the patch.  I'd like to get this
committed so I can turn to PR 69742.

Regards

	Thomas


2016-02-03  Thomas Koenig  <tkoenig@gcc.gnu.org>

         PR fortran/60526
         * decl.c (build_sym):  If the name has already been defined as a
         type, issue error and return false.

2016-02-03  Thomas Koenig  <tkoenig@gcc.gnu.org>

         PR fortran/60526
         * gfortran.dg/type_decl_4.f90:  New test.


[-- Attachment #2: p4.diff --]
[-- Type: text/x-patch, Size: 1052 bytes --]

Index: decl.c
===================================================================
--- decl.c	(Revision 232864)
+++ decl.c	(Arbeitskopie)
@@ -1215,10 +1215,30 @@ build_sym (const char *name, gfc_charlen *cl, bool
 {
   symbol_attribute attr;
   gfc_symbol *sym;
+  int nlen;
+  char u_name[GFC_MAX_SYMBOL_LEN + 1];
+  gfc_symtree *st;
 
   if (gfc_get_symbol (name, NULL, &sym))
     return false;
 
+  /* Check if the name has already been defined as a type.  The
+     first letter of the symtree will be in upper case then.  */
+
+  nlen = strlen(name);
+  gcc_assert (nlen <= GFC_MAX_SYMBOL_LEN);
+  strncpy (u_name, name, nlen + 1);
+  u_name[0] = TOUPPER(u_name[0]);
+
+  st = gfc_find_symtree (gfc_current_ns->sym_root, u_name);
+
+  if (st != 0)
+    {
+      gfc_error ("Symbol %qs at %C also declared as a type at %L", name,
+		 &st->n.sym->declared_at);
+      return false;
+    }
+
   /* Start updating the symbol table.  Add basic type attribute if present.  */
   if (current_ts.type != BT_UNKNOWN
       && (sym->attr.implicit_type == 0

[-- Attachment #3: type_decl_4.f90 --]
[-- Type: text/x-fortran, Size: 225 bytes --]

! { dg-do compile }
program main
  type Xx ! { dg-error "Symbol 'xx' at .1. also declared as a type at .2." }
  end type Xx
  real :: Xx  ! { dg-error "Symbol 'xx' at .1. also declared as a type at .2." }
  
end program main

Re: [patch, Fortran] Fix PR 60526, variable name has already been declared as a type

[Janne Blomqvist]

On Sat, Feb 13, 2016 at 1:56 PM, Thomas Koenig <tkoenig@netcologne.de> wrote:
> Am 12.02.2016 um 11:42 schrieb Janne Blomqvist:
>>
>> On Fri, Feb 12, 2016 at 12:16 PM, Andre Vehreschild <vehre@gmx.de> wrote:
>
>
>>> The proposed alloca() call
>>> has according to the documentation of libc some availability issues on
>>> certain platforms, too.
>
>
> These availablity issues cannot be too serious, or we would be having
> trouble already:
>
> ig25@linux-fd1f:~/Gcc/trunk/gcc/fortran> fgrep -H -n 'alloca (' *.c
> cpp.c:839:      to_file_quoted = (unsigned char *) alloca (to_file_len * 4 +
> 1);
> cpp.c:1079:     (unsigned char *) alloca (to_file_len * 4 + 1);
> error.c:898:  buffer = (char *) alloca (strlen (msg) + strlen (msg2) + 2);
> module.c:6042:      filename = (char *) alloca (n);
> module.c:6048:      filename = (char *) alloca (n);
> module.c:6058:  filename_tmp = (char *) alloca (n + 1);
> options.c:267:      source_path = (char *) alloca (i + 1);
> primary.c:214:  buffer = (char *) alloca (length + 1);
> primary.c:438:  buffer = (char *) alloca (length + 1);
> primary.c:600:  buffer = (char *) alloca (count + 1);
> scanner.c:321:  q = (char *) alloca (len + 1);
> simplify.c:6381:  buffer = (unsigned char*)alloca (buffer_size);
> target-memory.c:674:  buffer = (unsigned char*)alloca (len);
> target-memory.c:781:  buffer = (unsigned char*)alloca (buffer_size);
>
>>> Therefore why not going with the fixed size
>>> stack array and adding a check for possible overflow to it and be done?
>>
>>
>> Yes, I agree. That sounds like the best approach in this case.
>
>
> OK, here is the final version of the patch.  I'd like to get this
> committed so I can turn to PR 69742.

Ok, thanks for the patch.


-- 
Janne Blomqvist

Re: [patch, Fortran] Fix PR 60526, variable name has already been declared as a type

[H.J. Lu]

On Sat, Feb 13, 2016 at 3:56 AM, Thomas Koenig <tkoenig@netcologne.de> wrote:
> Am 12.02.2016 um 11:42 schrieb Janne Blomqvist:
>>
>> On Fri, Feb 12, 2016 at 12:16 PM, Andre Vehreschild <vehre@gmx.de> wrote:
>
>
>>> The proposed alloca() call
>>> has according to the documentation of libc some availability issues on
>>> certain platforms, too.
>
>
> These availablity issues cannot be too serious, or we would be having
> trouble already:
>
> ig25@linux-fd1f:~/Gcc/trunk/gcc/fortran> fgrep -H -n 'alloca (' *.c
> cpp.c:839:      to_file_quoted = (unsigned char *) alloca (to_file_len * 4 +
> 1);
> cpp.c:1079:     (unsigned char *) alloca (to_file_len * 4 + 1);
> error.c:898:  buffer = (char *) alloca (strlen (msg) + strlen (msg2) + 2);
> module.c:6042:      filename = (char *) alloca (n);
> module.c:6048:      filename = (char *) alloca (n);
> module.c:6058:  filename_tmp = (char *) alloca (n + 1);
> options.c:267:      source_path = (char *) alloca (i + 1);
> primary.c:214:  buffer = (char *) alloca (length + 1);
> primary.c:438:  buffer = (char *) alloca (length + 1);
> primary.c:600:  buffer = (char *) alloca (count + 1);
> scanner.c:321:  q = (char *) alloca (len + 1);
> simplify.c:6381:  buffer = (unsigned char*)alloca (buffer_size);
> target-memory.c:674:  buffer = (unsigned char*)alloca (len);
> target-memory.c:781:  buffer = (unsigned char*)alloca (buffer_size);
>
>>> Therefore why not going with the fixed size
>>> stack array and adding a check for possible overflow to it and be done?
>>
>>
>> Yes, I agree. That sounds like the best approach in this case.
>
>
> OK, here is the final version of the patch.  I'd like to get this
> committed so I can turn to PR 69742.
>
> Regards
>
>         Thomas
>
>
>
> 2016-02-03  Thomas Koenig  <tkoenig@gcc.gnu.org>
>
>         PR fortran/60526
>         * decl.c (build_sym):  If the name has already been defined as a
>         type, issue error and return false.
>
> 2016-02-03  Thomas Koenig  <tkoenig@gcc.gnu.org>
>
>         PR fortran/60526
>         * gfortran.dg/type_decl_4.f90:  New test.
>

It breaks bootstrap on x86:

../../../src-trunk/libgfortran/intrinsics/selected_int_kind.f90:28:40:

   integer :: _gfortran_selected_int_kind
                                        1
Error: Symbol \u2018_gfortran_selected_int_kind\u2019 at (1) also
declared as a type at (2)
f951: Fatal Error: Option \u2018-fallow-leading-underscore\u2019 is
for use only by gfortran developers, and should not be used for
implicitly typed variables
compilation terminated.
Makefile:5679: recipe for target 'selected_int_kind.lo' failed
make[6]: *** [selected_int_kind.lo] Error 1


-- 
H.J.

Re: [patch, Fortran] Fix PR 60526, variable name has already been declared as a type

[Thomas Koenig]

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

Am 14.02.2016 um 15:38 schrieb H.J. Lu:

> It breaks bootstrap on x86:
>
> ../../../src-trunk/libgfortran/intrinsics/selected_int_kind.f90:28:40:
>
>     integer :: _gfortran_selected_int_kind

I have fixed this in two parts:

a) reverted the patch (r233411).  I still managed to catch the revision
    immediately following r233410.  Thanks H.J. for the prompt report!

b) committed a fixed patch (r233413)

The problem was that there is no upcase equivalent for _, so the
test for a symbol with an upcase first letter found the symbol itself.
That fix was obvios, see attached patch.

I tested the new patch by regression-testing and by rebuilding
libgfortran.

I was unable to write a test case because I could not find a
set of options to allow a leading underscore in a function name.

So, should we do something differently?  There are only seven
non-generated *.f90 files in libgfortran.  The chances of
breaking bootstrap this way are relatively low, and patch
reversion is easy enough, so I don't think we should regularly
rebuild libgfortran for this.

Regards

	Thomas


2016-02-14  Thomas Koenig  <tkoenig@gcc.gnu.org>

         PR fortran/60526
         * decl.c (build_sym):  If the name has already been defined as a
         type, it has a symtree with an upper case letter at the beginning.
         If such a symtree exists, issue an error and exit.  Don't do
         this if there is no corresponding upper case letter.


2016-02-14  Thomas Koenig  <tkoenig@gcc.gnu.org>

         PR fortran/60526
         * gfortran.dg/type_decl_4.f90:  Reinstated.


[-- Attachment #2: p5.diff --]
[-- Type: text/x-patch, Size: 1246 bytes --]

Index: decl.c
===================================================================
--- decl.c	(Revision 233411)
+++ decl.c	(Arbeitskopie)
@@ -1215,10 +1215,38 @@ build_sym (const char *name, gfc_charlen *cl, bool
 {
   symbol_attribute attr;
   gfc_symbol *sym;
+  int upper;
 
   if (gfc_get_symbol (name, NULL, &sym))
     return false;
 
+  /* Check if the name has already been defined as a type.  The
+     first letter of the symtree will be in upper case then.  Of
+     course, this is only necessary if the upper case letter is
+     actually different.  */
+
+  upper = TOUPPER(name[0]);
+  if (upper != name[0])
+    {
+      char u_name[GFC_MAX_SYMBOL_LEN + 1];
+      gfc_symtree *st;
+      int nlen;
+
+      nlen = strlen(name);
+      gcc_assert (nlen <= GFC_MAX_SYMBOL_LEN);
+      strncpy (u_name, name, nlen + 1);
+      u_name[0] = upper;
+
+      st = gfc_find_symtree (gfc_current_ns->sym_root, u_name);
+
+      if (st != 0)
+	{
+	  gfc_error ("Symbol %qs at %C also declared as a type at %L", name,
+		     &st->n.sym->declared_at);
+	  return false;
+	}
+    }
+
   /* Start updating the symbol table.  Add basic type attribute if present.  */
   if (current_ts.type != BT_UNKNOWN
       && (sym->attr.implicit_type == 0