ICE in gfc_new_symbol() due to overlong symbol name

[Andrew Benson]

I just opened PR for this: 87103

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87103

The following code causes an ICE with gfortan 9.0.0 (r263855):

module a
type :: nameThatIsVeryLongButNotTooLongThatItShouldBeInvalid
end type nameThatIsVeryLongButNotTooLongThatItShouldBeInvalid
contains
subroutine b(self)
class(nameThatIsVeryLongButNotTooLongThatItShouldBeInvalid), intent(in) ::
self
select type (self)
class is (nameThatIsVeryLongButNotTooLongThatItShouldBeInvalid)
end select
end subroutine b
end module a

$ gfortran -v
Using built-in specs.
COLLECT_GCC=gfortran
COLLECT_LTO_WRAPPER=/home/abenson/Galacticus/Tools/libexec/gcc/x86_64-pc-
linux-gnu/9.0.0/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: ../gcc-trunk/configure --prefix=/home/abenson/Galacticus/Tools
--enable-languages=c,c++,fortran --disable-multilib : (reconfigured) ../gcc-
trunk/configure --prefix=/home/abenson/Galacticus/Tools --enable-languages=c,c+
+,fortran --disable-multilib : (reconfigured) ../gcc-trunk/configure --prefix=/
home/abenson/Galacticus/Tools --disable-multilib --enable-languages=c,c+
+,fortran,lto --no-create --no-recursion : (reconfigured) ../gcc-trunk/configure
--prefix=/home/abenson/Galacticus/Tools --disable-multilib --enable-
languages=c,c++,fortran,lto --no-create --no-recursion : (reconfigured) ../gcc-
trunk/configure --prefix=/home/abenson/Galacticus/Tools --disable-multilib --
enable-languages=c,c++,fortran,lto --no-create --no-recursion
Thread model: posix
gcc version 9.0.0 20180825 (experimental) (GCC)


$ gfortran -c tmp1.F90 -o tmp1.o
f951: internal compiler error: new_symbol(): Symbol name too long
0x7b9711 gfc_internal_error(char const*, ...)
../../gcc-trunk/gcc/fortran/error.c:1362
0x84a838 gfc_new_symbol(char const*, gfc_namespace*)
../../gcc-trunk/gcc/fortran/symbol.c:3132
0x84ab97 gfc_get_sym_tree(char const*, gfc_namespace*, gfc_symtree**, bool)
../../gcc-trunk/gcc/fortran/symbol.c:3373
0x7e565d select_type_set_tmp
../../gcc-trunk/gcc/fortran/match.c:6114
0x7ee260 gfc_match_class_is()
../../gcc-trunk/gcc/fortran/match.c:6470
0x808c79 match_word
../../gcc-trunk/gcc/fortran/parse.c:65
0x80a4f1 decode_statement
../../gcc-trunk/gcc/fortran/parse.c:462
0x80d04c next_free
../../gcc-trunk/gcc/fortran/parse.c:1234
0x80d04c next_statement
../../gcc-trunk/gcc/fortran/parse.c:1466
0x810861 parse_select_type_block
../../gcc-trunk/gcc/fortran/parse.c:4236
0x810861 parse_executable
../../gcc-trunk/gcc/fortran/parse.c:5330
0x8118e6 parse_progunit
../../gcc-trunk/gcc/fortran/parse.c:5697
0x811bfc parse_contained
../../gcc-trunk/gcc/fortran/parse.c:5574
0x812a4c parse_module
../../gcc-trunk/gcc/fortran/parse.c:5944
0x812f75 gfc_parse_file()
../../gcc-trunk/gcc/fortran/parse.c:6247
0x859d3f gfc_be_parse_file
../../gcc-trunk/gcc/fortran/f95-lang.c:204
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See <https://gcc.gnu.org/bugs/> for instructions.


The problem seems to be that gfc_new_symbol() is passed a name
'__tmp_class_namethatisverylongbutnottoolongthatitshouldbeinvalid' in this
case which exceeds GFC_MAX_SYMBOL_LEN=64. My understanding is that the code is
valid since the symbol name itself,
'namethatisverylongbutnottoolongthatitshouldbeinvalid', is less than 64
characters.

I'm not sure what the correct fix for this is - should the name length limit in
gfc_new_symbol() just be increased by enough to allow the '__tmp_class_'
prefix?

This appears to be a regression as the same code (with the same long-but-
legal) symbol names compiled under earlier versions of gfortran. I'll attempt
to rollback to previous versions and see if I can identify where the change
occurred.

-Andrew

[Andrew Benson]

As suggested by Janus, PR87103 is easily fixed by the attached patch which
increases GFC_MAX_SYMBOL_LEN to 76 (sufficient to hold the maximum allowed F08
symbol length of 63, plus a null terminator, plus the "__tmp_class_" prefix).

Bootstraps and regtests successfully.

OK for trunk?

-Andrew

Post by Andrew Benson
I just opened PR for this: 87103
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87103
module a
type :: nameThatIsVeryLongButNotTooLongThatItShouldBeInvalid
end type nameThatIsVeryLongButNotTooLongThatItShouldBeInvalid
contains
subroutine b(self)
class(nameThatIsVeryLongButNotTooLongThatItShouldBeInvalid), intent(in)
:: self
select type (self)
class is (nameThatIsVeryLongButNotTooLongThatItShouldBeInvalid)
end select
end subroutine b
end module a
$ gfortran -v
Using built-in specs.
COLLECT_GCC=gfortran
COLLECT_LTO_WRAPPER=/home/abenson/Galacticus/Tools/libexec/gcc/x86_64-pc-
linux-gnu/9.0.0/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: ../gcc-trunk/configure
--prefix=/home/abenson/Galacticus/Tools --enable-languages=c,c++,fortran
--disable-multilib : (reconfigured) ../gcc- trunk/configure
--prefix=/home/abenson/Galacticus/Tools --enable-languages=c,c+ +,fortran
--disable-multilib : (reconfigured) ../gcc-trunk/configure --prefix=/
home/abenson/Galacticus/Tools --disable-multilib --enable-languages=c,c+
+,fortran,lto --no-create --no-recursion : (reconfigured)
../gcc-trunk/configure --prefix=/home/abenson/Galacticus/Tools
--disable-multilib --enable- languages=c,c++,fortran,lto --no-create
--no-recursion : (reconfigured) ../gcc- trunk/configure
--prefix=/home/abenson/Galacticus/Tools --disable-multilib --
enable-languages=c,c++,fortran,lto --no-create --no-recursion
Thread model: posix
gcc version 9.0.0 20180825 (experimental) (GCC)
$ gfortran -c tmp1.F90 -o tmp1.o
f951: internal compiler error: new_symbol(): Symbol name too long
0x7b9711 gfc_internal_error(char const*, ...)
../../gcc-trunk/gcc/fortran/error.c:1362
0x84a838 gfc_new_symbol(char const*, gfc_namespace*)
../../gcc-trunk/gcc/fortran/symbol.c:3132
0x84ab97 gfc_get_sym_tree(char const*, gfc_namespace*, gfc_symtree**, bool)
../../gcc-trunk/gcc/fortran/symbol.c:3373
0x7e565d select_type_set_tmp
../../gcc-trunk/gcc/fortran/match.c:6114
0x7ee260 gfc_match_class_is()
../../gcc-trunk/gcc/fortran/match.c:6470
0x808c79 match_word
../../gcc-trunk/gcc/fortran/parse.c:65
0x80a4f1 decode_statement
../../gcc-trunk/gcc/fortran/parse.c:462
0x80d04c next_free
../../gcc-trunk/gcc/fortran/parse.c:1234
0x80d04c next_statement
../../gcc-trunk/gcc/fortran/parse.c:1466
0x810861 parse_select_type_block
../../gcc-trunk/gcc/fortran/parse.c:4236
0x810861 parse_executable
../../gcc-trunk/gcc/fortran/parse.c:5330
0x8118e6 parse_progunit
../../gcc-trunk/gcc/fortran/parse.c:5697
0x811bfc parse_contained
../../gcc-trunk/gcc/fortran/parse.c:5574
0x812a4c parse_module
../../gcc-trunk/gcc/fortran/parse.c:5944
0x812f75 gfc_parse_file()
../../gcc-trunk/gcc/fortran/parse.c:6247
0x859d3f gfc_be_parse_file
../../gcc-trunk/gcc/fortran/f95-lang.c:204
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See <https://gcc.gnu.org/bugs/> for instructions.
The problem seems to be that gfc_new_symbol() is passed a name
'__tmp_class_namethatisverylongbutnottoolongthatitshouldbeinvalid' in this
case which exceeds GFC_MAX_SYMBOL_LEN=64. My understanding is that the code
is valid since the symbol name itself,
'namethatisverylongbutnottoolongthatitshouldbeinvalid', is less than 64
characters.
I'm not sure what the correct fix for this is - should the name length limit
in gfc_new_symbol() just be increased by enough to allow the '__tmp_class_'
prefix?
This appears to be a regression as the same code (with the same long-but-
legal) symbol names compiled under earlier versions of gfortran. I'll
attempt to rollback to previous versions and see if I can identify where
the change occurred.
-Andrew

[Bernhard Reutner-Fischer]

Post by Andrew Benson
As suggested by Janus, PR87103 is easily fixed by the attached patch which
increases GFC_MAX_SYMBOL_LEN to 76 (sufficient to hold the maximum allowed F08
symbol length of 63, plus a null terminator, plus the "__tmp_class_" prefix).

This is so much wrong.
Note that this will be fixed properly by the changes contained in the
https://gcc.gnu.org/git/?p=gcc.git;a=shortlog;h=refs/heads/aldot/fortran-fe-stringpool
branch.
There we keep the GFC_MAX_SYMBOL_LEN at 63 proper but use an internal
buffer double that size which in turn is sufficient to hold all
compiler-generated identifiers.
See gfc_get_string() even in current TOT.

Maybe we should bite the bullet and start to merge the stringpool
changes now instead of this hack?

thanks and cheers,

[Bernhard Reutner-Fischer]

Post by Bernhard Reutner-Fischer

Post by Andrew Benson
As suggested by Janus, PR87103 is easily fixed by the attached patch which
increases GFC_MAX_SYMBOL_LEN to 76 (sufficient to hold the maximum allowed F08
symbol length of 63, plus a null terminator, plus the "__tmp_class_" prefix).

This is so much wrong.
Note that this will be fixed properly by the changes contained in the
https://gcc.gnu.org/git/?p=gcc.git;a=shortlog;h=refs/heads/aldot/fortran-fe-stringpool
branch.
There we keep the GFC_MAX_SYMBOL_LEN at 63 proper but use an internal
buffer double that size which in turn is sufficient to hold all
compiler-generated identifiers.
See gfc_get_string() even in current TOT.
Maybe we should bite the bullet and start to merge the stringpool
changes now instead of this hack?

It all makes sense to me, please proceed. (my 2 cents worth)

Ok so i will reread the fortran-fe-stringpool series and submit it
here for review.

Let's return to the issue at hand for a moment, though.
I tested the attached alternate fix on top of the
fortran-fe-stringpool branch where it fixes PR87103.
Maybe somebody has spare cycles to test it on top of current trunk?

thanks,

[PATCH,FORTRAN] PR87103: Remove max symbol length check from gfc_new_symbol

gfc_match_name does check for too long names already. Since
gfc_new_symbol is also called for symbols with internal names containing
compiler-generated prefixes, these internal names can easily exceed the
max_identifier_length mandated by the standard.

gcc/fortran/ChangeLog

2018-09-04 Bernhard Reutner-Fischer <***@gcc.gnu.org>

PR fortran/87103
* expr.c (gfc_check_conformance): Check vsnprintf for truncation.
* iresolve.c (gfc_get_string): Likewise.
* symbol.c (gfc_new_symbol): Remove check for maximum symbol
name length. Remove redundant 0 setting of new calloc()ed
gfc_symbol.

[Andrew Benson]

On Wednesday, September 5, 2018 12:35:04 PM PDT Bernhard Reutner-Fischer

Post by Bernhard Reutner-Fischer

Post by Bernhard Reutner-Fischer

Post by Andrew Benson
As suggested by Janus, PR87103 is easily fixed by the attached patch which
increases GFC_MAX_SYMBOL_LEN to 76 (sufficient to hold the maximum
allowed F08 symbol length of 63, plus a null terminator, plus the
"__tmp_class_" prefix).> >

This is so much wrong.
Note that this will be fixed properly by the changes contained in the
https://gcc.gnu.org/git/?p=gcc.git;a=shortlog;h=refs/heads/aldot/fortran
-fe-stringpool branch.
There we keep the GFC_MAX_SYMBOL_LEN at 63 proper but use an internal
buffer double that size which in turn is sufficient to hold all
compiler-generated identifiers.
See gfc_get_string() even in current TOT.
Maybe we should bite the bullet and start to merge the stringpool
changes now instead of this hack?

It all makes sense to me, please proceed. (my 2 cents worth)

Ok so i will reread the fortran-fe-stringpool series and submit it
here for review.
Let's return to the issue at hand for a moment, though.
I tested the attached alternate fix on top of the
fortran-fe-stringpool branch where it fixes PR87103.
Maybe somebody has spare cycles to test it on top of current trunk?
thanks,
[PATCH,FORTRAN] PR87103: Remove max symbol length check from gfc_new_symbol
gfc_match_name does check for too long names already. Since
gfc_new_symbol is also called for symbols with internal names containing
compiler-generated prefixes, these internal names can easily exceed the
max_identifier_length mandated by the standard.
gcc/fortran/ChangeLog
PR fortran/87103
* expr.c (gfc_check_conformance): Check vsnprintf for truncation.
* iresolve.c (gfc_get_string): Likewise.
* symbol.c (gfc_new_symbol): Remove check for maximum symbol
name length. Remove redundant 0 setting of new calloc()ed
gfc_symbol.

This patch tests successfully on the current trunk for me.