Re: [patch, fortran] PR93486 - ICE on valid with nested submodules and long submodule names
Hi Paul, Thanks for the review. This is now committed as: r10-6976-gf3c276aec26d9e406cc4bbf0e18b1105df63f0ee I'll keep this in mind for future patches - this one seemed simple enough that I'd be confident to commit it without review after waiting for a few days. I'm hoping to find time to finish some other patches soon, some of which are more complicated and I'd definitely want to get reviewed before I commit them. Thanks again everyone. -Andrew On Monday, March 2, 2020 6:41:46 AM PST Paul Richard Thomas wrote: > Andrew, > > I agree with Steve. That said, I took a look at your patch and it's > just fine. OK to commit. > > Cheers > > Paul > > On Mon, 2 Mar 2020 at 02:10, Steve Kargl > > wrote: > > On Sun, Mar 01, 2020 at 11:43:23PM +0100, Thomas Koenig wrote: > > > Am 01.03.20 um 23:42 schrieb Steve Kargl: > > > > PS: in general, after multiple > > > > pings, just commit the patch. > > > > > > ... well, maybe after a "If there is no reply within a > > > couple of days, I will commit this" :-) > > > > Andrew submitted the patch and pinged it twice. gfortran > > development is running on fumes. Beating one's head > > against a wall seems counter productive. I'm operating > > on a principle that if one has commit access for gfortran, > > one is committing a patch with the best attentions. Could > > this lead to a regression? Sure. The alternative of > > constantly pinging patches is to simply stop submitting > > patches. > > > > > > -- > > Steve -- * Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html * Galacticus: https://github.com/galacticusorg/galacticus
Re: [patch, fortran] PR93486 - ICE on valid with nested submodules and long submodule names
Thanks, Steve. I'll get this committed tomorrow morning. -Andrew On Sunday, March 1, 2020 2:42:13 PM PST Steve Kargl wrote: > On Sun, Mar 01, 2020 at 11:33:10AM -0800, Andrew Benson wrote: > > *ping* > > Andrew, > > The patch looks fine to me. PS: in general, after multiple > pings, just commit the patch. -- * Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html * Galacticus: https://github.com/galacticusorg/galacticus
Re: [patch, fortran] PR93486 - ICE on valid with nested submodules and long submodule names
*ping* On Tuesday, January 28, 2020 4:45:59 PM PST Andrew Benson wrote: > I opened PR93486 for this problem: > > The following causes an ICE with revision > ad690d79cfbb905c5546c9333c5fd089d906505b: > > module ivs > interface l > module procedure l_ > end interface l > contains > function l_() > end function l_ > end module ivs > > module aModeratleyLongModuleName > use ivs > interface > module subroutine cmo() > end subroutine cmo > end interface > end module aModeratleyLongModuleName > > submodule (aModeratleyLongModuleName) > aNameForASubmoduleThatIsVeryLongButWhichIsLegalStill > contains > module procedure cmo > end procedure cmo > end submodule aNameForASubmoduleThatIsVeryLongButWhichIsLegalStill > > submodule > (aModeratleyLongModuleName:aNameForASubmoduleThatIsVeryLongButWhichIsLegalSt > ill) sb > end submodule sb > > submodule (aModeratleyLongModuleName:sb) sc > end submodule sc > > > $ gfortran -v > Using built-in specs. > COLLECT_GCC=gfortran > COLLECT_LTO_WRAPPER=/data001/abenson/Galacticus/Tools_Devel_Install/bin/../ > libexec/gcc/x86_64-pc-linux-gnu/10.0.1/lto-wrapper > Target: x86_64-pc-linux-gnu > Configured with: ../gcc-git/configure --prefix=/home/abenson/Galacticus/ > Tools_Devel --enable-languages=c,c++,fortran --disable-multilib > Thread model: posix > Supported LTO compression algorithms: zlib > gcc version 10.0.1 20200128 (experimental) (GCC) > > > $ gfortran -c test.F90 -o test.o > f951: internal compiler error: Segmentation fault > 0xe1bc9f crash_signal > ../../gcc-git/gcc/toplev.c:328 > 0x7f98119c71ef ??? > > /data001/abenson/Galacticus/Tools/glibc-2.12.1/signal/../sysdeps/unix/ > sysv/linux/x86_64/sigaction.c:0 > 0x84b3f0 gfc_use_modules() > ../../gcc-git/gcc/fortran/module.c:7315 > 0x85acc8 use_modules > ../../gcc-git/gcc/fortran/parse.c:114 > 0x866daa parse_module > ../../gcc-git/gcc/fortran/parse.c:6111 > 0x86733c gfc_parse_file() > ../../gcc-git/gcc/fortran/parse.c:6428 > 0x8b780f gfc_be_parse_file > ../../gcc-git/gcc/fortran/f95-lang.c:210 > 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 ICE goes away if the module and/or submodule names are made shorter. > > The problem is caused when loading (generic or operator) interfaces from a > module file. The module name can include the submodule name, so a size of > 2*GFC_MAX_SYMBOL_LEN+2 is required to read this in. > > The attached patch fixes this problem and regression tests cleanly *if* the > patch for PR87103 is applied (otherwise that problem gets triggered by the > new test case). > > PR87103 has been a long-standing issue - there's a patch that works (either > my original ugly fix, or the better fix proposed by Bernhard at https:// > gcc.gnu.org/ml/fortran/2018-09/msg00044.html ). It would be very nice to > get one of these patches committed. > > -Andrew -- * Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html * Galacticus: https://github.com/galacticusorg/galacticus
Re: [patch, fortran] PR93486 - ICE on valid with nested submodules and long submodule names
*ping* On Tuesday, January 28, 2020 4:45:59 PM PST Andrew Benson wrote: > I opened PR93486 for this problem: > > The following causes an ICE with revision > ad690d79cfbb905c5546c9333c5fd089d906505b: > > module ivs > interface l > module procedure l_ > end interface l > contains > function l_() > end function l_ > end module ivs > > module aModeratleyLongModuleName > use ivs > interface > module subroutine cmo() > end subroutine cmo > end interface > end module aModeratleyLongModuleName > > submodule (aModeratleyLongModuleName) > aNameForASubmoduleThatIsVeryLongButWhichIsLegalStill > contains > module procedure cmo > end procedure cmo > end submodule aNameForASubmoduleThatIsVeryLongButWhichIsLegalStill > > submodule > (aModeratleyLongModuleName:aNameForASubmoduleThatIsVeryLongButWhichIsLegalSt > ill) sb > end submodule sb > > submodule (aModeratleyLongModuleName:sb) sc > end submodule sc > > > $ gfortran -v > Using built-in specs. > COLLECT_GCC=gfortran > COLLECT_LTO_WRAPPER=/data001/abenson/Galacticus/Tools_Devel_Install/bin/../ > libexec/gcc/x86_64-pc-linux-gnu/10.0.1/lto-wrapper > Target: x86_64-pc-linux-gnu > Configured with: ../gcc-git/configure --prefix=/home/abenson/Galacticus/ > Tools_Devel --enable-languages=c,c++,fortran --disable-multilib > Thread model: posix > Supported LTO compression algorithms: zlib > gcc version 10.0.1 20200128 (experimental) (GCC) > > > $ gfortran -c test.F90 -o test.o > f951: internal compiler error: Segmentation fault > 0xe1bc9f crash_signal > ../../gcc-git/gcc/toplev.c:328 > 0x7f98119c71ef ??? > > /data001/abenson/Galacticus/Tools/glibc-2.12.1/signal/../sysdeps/unix/ > sysv/linux/x86_64/sigaction.c:0 > 0x84b3f0 gfc_use_modules() > ../../gcc-git/gcc/fortran/module.c:7315 > 0x85acc8 use_modules > ../../gcc-git/gcc/fortran/parse.c:114 > 0x866daa parse_module > ../../gcc-git/gcc/fortran/parse.c:6111 > 0x86733c gfc_parse_file() > ../../gcc-git/gcc/fortran/parse.c:6428 > 0x8b780f gfc_be_parse_file > ../../gcc-git/gcc/fortran/f95-lang.c:210 > 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 ICE goes away if the module and/or submodule names are made shorter. > > The problem is caused when loading (generic or operator) interfaces from a > module file. The module name can include the submodule name, so a size of > 2*GFC_MAX_SYMBOL_LEN+2 is required to read this in. > > The attached patch fixes this problem and regression tests cleanly *if* the > patch for PR87103 is applied (otherwise that problem gets triggered by the > new test case). > > PR87103 has been a long-standing issue - there's a patch that works (either > my original ugly fix, or the better fix proposed by Bernhard at https:// > gcc.gnu.org/ml/fortran/2018-09/msg00044.html ). It would be very nice to > get one of these patches committed. > > -Andrew -- * Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html * Galacticus: https://github.com/galacticusorg/galacticus
Re: [patch, fortran] PR93486 - ICE on valid with nested submodules and long submodule names
*ping* On Tuesday, January 28, 2020 4:45:59 PM PST Andrew Benson wrote: > I opened PR93486 for this problem: > > The following causes an ICE with revision > ad690d79cfbb905c5546c9333c5fd089d906505b: > > module ivs > interface l > module procedure l_ > end interface l > contains > function l_() > end function l_ > end module ivs > > module aModeratleyLongModuleName > use ivs > interface > module subroutine cmo() > end subroutine cmo > end interface > end module aModeratleyLongModuleName > > submodule (aModeratleyLongModuleName) > aNameForASubmoduleThatIsVeryLongButWhichIsLegalStill > contains > module procedure cmo > end procedure cmo > end submodule aNameForASubmoduleThatIsVeryLongButWhichIsLegalStill > > submodule > (aModeratleyLongModuleName:aNameForASubmoduleThatIsVeryLongButWhichIsLegalSt > ill) sb > end submodule sb > > submodule (aModeratleyLongModuleName:sb) sc > end submodule sc > > > $ gfortran -v > Using built-in specs. > COLLECT_GCC=gfortran > COLLECT_LTO_WRAPPER=/data001/abenson/Galacticus/Tools_Devel_Install/bin/../ > libexec/gcc/x86_64-pc-linux-gnu/10.0.1/lto-wrapper > Target: x86_64-pc-linux-gnu > Configured with: ../gcc-git/configure --prefix=/home/abenson/Galacticus/ > Tools_Devel --enable-languages=c,c++,fortran --disable-multilib > Thread model: posix > Supported LTO compression algorithms: zlib > gcc version 10.0.1 20200128 (experimental) (GCC) > > > $ gfortran -c test.F90 -o test.o > f951: internal compiler error: Segmentation fault > 0xe1bc9f crash_signal > ../../gcc-git/gcc/toplev.c:328 > 0x7f98119c71ef ??? > > /data001/abenson/Galacticus/Tools/glibc-2.12.1/signal/../sysdeps/unix/ > sysv/linux/x86_64/sigaction.c:0 > 0x84b3f0 gfc_use_modules() > ../../gcc-git/gcc/fortran/module.c:7315 > 0x85acc8 use_modules > ../../gcc-git/gcc/fortran/parse.c:114 > 0x866daa parse_module > ../../gcc-git/gcc/fortran/parse.c:6111 > 0x86733c gfc_parse_file() > ../../gcc-git/gcc/fortran/parse.c:6428 > 0x8b780f gfc_be_parse_file > ../../gcc-git/gcc/fortran/f95-lang.c:210 > 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 ICE goes away if the module and/or submodule names are made shorter. > > The problem is caused when loading (generic or operator) interfaces from a > module file. The module name can include the submodule name, so a size of > 2*GFC_MAX_SYMBOL_LEN+2 is required to read this in. > > The attached patch fixes this problem and regression tests cleanly *if* the > patch for PR87103 is applied (otherwise that problem gets triggered by the > new test case). > > PR87103 has been a long-standing issue - there's a patch that works (either > my original ugly fix, or the better fix proposed by Bernhard at https:// > gcc.gnu.org/ml/fortran/2018-09/msg00044.html ). It would be very nice to > get one of these patches committed. > > -Andrew -- * Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html * Galacticus: https://github.com/galacticusorg/galacticus
Re: [patch, fortran] PR93486 - ICE on valid with nested submodules and long submodule names
*ping* On Tuesday, January 28, 2020 4:45:59 PM PST Andrew Benson wrote: > I opened PR93486 for this problem: > > The following causes an ICE with revision > ad690d79cfbb905c5546c9333c5fd089d906505b: > > module ivs > interface l > module procedure l_ > end interface l > contains > function l_() > end function l_ > end module ivs > > module aModeratleyLongModuleName > use ivs > interface > module subroutine cmo() > end subroutine cmo > end interface > end module aModeratleyLongModuleName > > submodule (aModeratleyLongModuleName) > aNameForASubmoduleThatIsVeryLongButWhichIsLegalStill > contains > module procedure cmo > end procedure cmo > end submodule aNameForASubmoduleThatIsVeryLongButWhichIsLegalStill > > submodule > (aModeratleyLongModuleName:aNameForASubmoduleThatIsVeryLongButWhichIsLegalSt > ill) sb > end submodule sb > > submodule (aModeratleyLongModuleName:sb) sc > end submodule sc > > > $ gfortran -v > Using built-in specs. > COLLECT_GCC=gfortran > COLLECT_LTO_WRAPPER=/data001/abenson/Galacticus/Tools_Devel_Install/bin/../ > libexec/gcc/x86_64-pc-linux-gnu/10.0.1/lto-wrapper > Target: x86_64-pc-linux-gnu > Configured with: ../gcc-git/configure --prefix=/home/abenson/Galacticus/ > Tools_Devel --enable-languages=c,c++,fortran --disable-multilib > Thread model: posix > Supported LTO compression algorithms: zlib > gcc version 10.0.1 20200128 (experimental) (GCC) > > > $ gfortran -c test.F90 -o test.o > f951: internal compiler error: Segmentation fault > 0xe1bc9f crash_signal > ../../gcc-git/gcc/toplev.c:328 > 0x7f98119c71ef ??? > > /data001/abenson/Galacticus/Tools/glibc-2.12.1/signal/../sysdeps/unix/ > sysv/linux/x86_64/sigaction.c:0 > 0x84b3f0 gfc_use_modules() > ../../gcc-git/gcc/fortran/module.c:7315 > 0x85acc8 use_modules > ../../gcc-git/gcc/fortran/parse.c:114 > 0x866daa parse_module > ../../gcc-git/gcc/fortran/parse.c:6111 > 0x86733c gfc_parse_file() > ../../gcc-git/gcc/fortran/parse.c:6428 > 0x8b780f gfc_be_parse_file > ../../gcc-git/gcc/fortran/f95-lang.c:210 > 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 ICE goes away if the module and/or submodule names are made shorter. > > The problem is caused when loading (generic or operator) interfaces from a > module file. The module name can include the submodule name, so a size of > 2*GFC_MAX_SYMBOL_LEN+2 is required to read this in. > > The attached patch fixes this problem and regression tests cleanly *if* the > patch for PR87103 is applied (otherwise that problem gets triggered by the > new test case). > > PR87103 has been a long-standing issue - there's a patch that works (either > my original ugly fix, or the better fix proposed by Bernhard at https:// > gcc.gnu.org/ml/fortran/2018-09/msg00044.html ). It would be very nice to > get one of these patches committed. > > -Andrew -- * Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html * Galacticus: https://github.com/galacticusorg/galacticus
Re: patch, fortan] PR83113 Bogus "duplicate allocatable attribute" error for submodule character function
Thanks - that worked. On Monday, February 10, 2020 12:18:57 PM PST Segher Boessenkool wrote: > On Mon, Feb 10, 2020 at 10:05:48AM -0800, Andrew Benson wrote: > > I don't think I have the ability to mark the PR as resolved. Can someone > > do > > that? > > You have an @gcc.gnu.org account; if you use that for your BZ account, > you will magically get everything you need here. > > > Segher -- * Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html * Galacticus: https://github.com/galacticusorg/galacticus
Re: patch, fortan] PR83113 Bogus "duplicate allocatable attribute" error for submodule character function
I don't think I have the ability to mark the PR as resolved. Can someone do that? On Monday, February 10, 2020 10:02:24 AM PST Andrew Benson wrote: > This is now committed (after adding the macro as Steve suggested): > > https://gcc.gnu.org/g:7848054c68bad6e2aa40cb59f77cc99bd8448d52 > > On Sunday, February 9, 2020 4:32:26 PM PST Steve Kargl wrote: > > Patch looks to me. OK to commit. > > > > I'm wondering if we need a macro or helper function to > > simplify in conditional from > > > > + if (gfc_state_stack->previous && gfc_state_stack->previous->previous > > + && gfc_state_stack->previous->previous->state == COMP_SUBMODULE > > + && sym->attr.module_procedure) > > > > to for example > > > > + if (IN_SUBMODULE (sym)) > > > > where is > > > > #define IN_SUBMODULE (x) \ > > > >(gfc_state_stack->previous && gfc_state_stack->previous->previous \ > > > > && gfc_state_stack->previous->previous->state == COMP_SUBMODULE \ > > && sym->attr.module_procedure) > > > > > > *ping* > > > > > > On Friday, January 31, 2020 5:06:49 PM PST Andrew Benson wrote: > > > > I've attached on updated patch for PR83113. The now removes the ICE > > > > when > > > > a > > > > duplicate DIMENSION attribute is specified in a submodule function. > > > > The > > > > patch reg tests cleanly. > > > > > > > > OK to commit? > > > > > > > > Note that this patch doesn't check that the attributes of duplicate > > > > declarations in a submodule function are valid or consistent with > > > > those > > > > declared in the module. For example both: > > > > > > > > module mm > > > > > > > > implicit none > > > > interface > > > > > > > > module function c() > > > > > > > >integer, dimension(2) :: c !! Dimension 2 here > > > > > > > > end function c > > > > > > > > end interface > > > > > > > > end module mm > > > > > > > > submodule (mm) oo > > > > > > > > implicit none > > > > > > > > contains > > > > > > > > module function c() > > > > > > > > integer, dimension(3) :: c !! Inconsistent dimension 3 here > > > > > > > > end function c > > > > > > > > end submodule oo > > > > > > > > > > > > and > > > > > > > > > > > > module mm > > > > > > > > implicit none > > > > interface > > > > > > > > module function c() > > > > > > > >integer, dimension(2) :: c !! Dimension 2 here > > > > > > > > end function c > > > > > > > > end interface > > > > > > > > end module mm > > > > > > > > submodule (mm) oo > > > > > > > > implicit none > > > > > > > > contains > > > > > > > > module function c() > > > > > > > > integer, dimension(:) :: c !! Invalid cannot have a deferred > > > > shape > > > > > > > > end function c > > > > > > > > end submodule oo > > > > > > > > > > > > compile successfully, but should be rejected. Presumably we should add > > > > some > > > > check that the attributes of the declaration in the submodule match > > > > those in the module? > > > > > > > > If so, I can go ahead and open a PR for this problem. > > > > > > > > -Andrew > > > > > > > > On Friday, January 24, 2020 10:54:24 AM PST Andrew Benson wrote: > > > > > Below is a partial patch for PR 83113 > > > > > > > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83113 > > > > > > > > > > It simply circumvents the check for duplicate attributes when > > > > > looking > > > > > at > > > > > declarations in a module procedure during compilation of a > > > &
Re: patch, fortan] PR83113 Bogus "duplicate allocatable attribute" error for submodule character function
This is now committed (after adding the macro as Steve suggested): https://gcc.gnu.org/g:7848054c68bad6e2aa40cb59f77cc99bd8448d52 On Sunday, February 9, 2020 4:32:26 PM PST Steve Kargl wrote: > Patch looks to me. OK to commit. > > I'm wondering if we need a macro or helper function to > simplify in conditional from > > + if (gfc_state_stack->previous && gfc_state_stack->previous->previous > + && gfc_state_stack->previous->previous->state == COMP_SUBMODULE > + && sym->attr.module_procedure) > > to for example > > + if (IN_SUBMODULE (sym)) > > where is > > #define IN_SUBMODULE (x) \ >(gfc_state_stack->previous && gfc_state_stack->previous->previous \ > && gfc_state_stack->previous->previous->state == COMP_SUBMODULE \ > && sym->attr.module_procedure) > > > *ping* > > > > On Friday, January 31, 2020 5:06:49 PM PST Andrew Benson wrote: > > > I've attached on updated patch for PR83113. The now removes the ICE when > > > a > > > duplicate DIMENSION attribute is specified in a submodule function. The > > > patch reg tests cleanly. > > > > > > OK to commit? > > > > > > Note that this patch doesn't check that the attributes of duplicate > > > declarations in a submodule function are valid or consistent with those > > > declared in the module. For example both: > > > > > > module mm > > > > > > implicit none > > > interface > > > > > > module function c() > > > > > >integer, dimension(2) :: c !! Dimension 2 here > > > > > > end function c > > > > > > end interface > > > > > > end module mm > > > > > > submodule (mm) oo > > > > > > implicit none > > > > > > contains > > > > > > module function c() > > > > > > integer, dimension(3) :: c !! Inconsistent dimension 3 here > > > > > > end function c > > > > > > end submodule oo > > > > > > > > > and > > > > > > > > > module mm > > > > > > implicit none > > > interface > > > > > > module function c() > > > > > >integer, dimension(2) :: c !! Dimension 2 here > > > > > > end function c > > > > > > end interface > > > > > > end module mm > > > > > > submodule (mm) oo > > > > > > implicit none > > > > > > contains > > > > > > module function c() > > > > > > integer, dimension(:) :: c !! Invalid cannot have a deferred shape > > > > > > end function c > > > > > > end submodule oo > > > > > > > > > compile successfully, but should be rejected. Presumably we should add > > > some > > > check that the attributes of the declaration in the submodule match > > > those in the module? > > > > > > If so, I can go ahead and open a PR for this problem. > > > > > > -Andrew > > > > > > On Friday, January 24, 2020 10:54:24 AM PST Andrew Benson wrote: > > > > Below is a partial patch for PR 83113 > > > > > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83113 > > > > > > > > It simply circumvents the check for duplicate attributes when looking > > > > at > > > > declarations in a module procedure during compilation of a submodule. > > > > > > > > As it stands, the patch handles the cases of "allocatable", > > > > "dimension", > > > > and "pointer" attributes (corresponding to the two test cases in the > > > > PR > > > > plus a case that showed up in my own code). There are other attributes > > > > which should be handled similarly but before I make those changes I > > > > wanted to seek some opinions on whether this is the correct/best way > > > > to > > > > fix this issue. > > > > > > > > The patch regression tests cleanly - however, while the patch solves > > > > the > > > > "duplicate attribute" problem for the test case in comment #4: > > > > > > > > https://gcc.gnu.
Re: patch, fortan] PR83113 Bogus "duplicate allocatable attribute" error for submodule character function
Hi Steve, Thanks for the review. Adding a macro as you suggest seems like a very good idea. I'll make that change tomorrow before committing this. Thanks, Andrew -- * Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html * Galacticus: https://github.com/galacticusorg/galacticus On Sun, Feb 9, 2020, 4:32 PM Steve Kargl wrote: > Patch looks to me. OK to commit. > > I'm wondering if we need a macro or helper function to > simplify in conditional from > > + if (gfc_state_stack->previous && gfc_state_stack->previous->previous > + && gfc_state_stack->previous->previous->state == COMP_SUBMODULE > + && sym->attr.module_procedure) > > to for example > > + if (IN_SUBMODULE (sym)) > > where is > > #define IN_SUBMODULE (x) \ >(gfc_state_stack->previous && gfc_state_stack->previous->previous \ > && gfc_state_stack->previous->previous->state == COMP_SUBMODULE \ > && sym->attr.module_procedure) > > -- > steve > > On Sun, Feb 09, 2020 at 02:59:52PM -0800, Andrew Benson wrote: > > *ping* > > > > On Friday, January 31, 2020 5:06:49 PM PST Andrew Benson wrote: > > > I've attached on updated patch for PR83113. The now removes the ICE > when a > > > duplicate DIMENSION attribute is specified in a submodule function. > The > > > patch reg tests cleanly. > > > > > > OK to commit? > > > > > > Note that this patch doesn't check that the attributes of duplicate > > > declarations in a submodule function are valid or consistent with those > > > declared in the module. For example both: > > > > > > module mm > > > implicit none > > > interface > > > module function c() > > >integer, dimension(2) :: c !! Dimension 2 here > > > end function c > > > end interface > > > end module mm > > > > > > submodule (mm) oo > > > implicit none > > > contains > > > module function c() > > > integer, dimension(3) :: c !! Inconsistent dimension 3 here > > > end function c > > > end submodule oo > > > > > > > > > and > > > > > > > > > module mm > > > implicit none > > > interface > > > module function c() > > >integer, dimension(2) :: c !! Dimension 2 here > > > end function c > > > end interface > > > end module mm > > > > > > submodule (mm) oo > > > implicit none > > > contains > > > module function c() > > > integer, dimension(:) :: c !! Invalid cannot have a deferred shape > > > end function c > > > end submodule oo > > > > > > > > > compile successfully, but should be rejected. Presumably we should add > some > > > check that the attributes of the declaration in the submodule match > those in > > > the module? > > > > > > If so, I can go ahead and open a PR for this problem. > > > > > > -Andrew > > > > > > On Friday, January 24, 2020 10:54:24 AM PST Andrew Benson wrote: > > > > Below is a partial patch for PR 83113 > > > > > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83113 > > > > > > > > It simply circumvents the check for duplicate attributes when > looking at > > > > declarations in a module procedure during compilation of a submodule. > > > > > > > > As it stands, the patch handles the cases of "allocatable", > "dimension", > > > > and "pointer" attributes (corresponding to the two test cases in the > PR > > > > plus a case that showed up in my own code). There are other > attributes > > > > which should be handled similarly but before I make those changes I > > > > wanted to seek some opinions on whether this is the correct/best way > to > > > > fix this issue. > > > > > > > > The patch regression tests cleanly - however, while the patch > solves the > > > > "duplicate attribute" problem for the test case in comment #4: > > > > > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83113#c4 > > > > > > > > that test case then ICEs with: > > > > > > > > $ gfortran -c pr83113_2.F90 -o pr83113_2.o > > > > f951: internal compiler error: in gfc_set_array_spec, at > > > > f
Re: patch, fortan] PR83113 Bogus "duplicate allocatable attribute" error for submodule character function
*ping* On Friday, January 31, 2020 5:06:49 PM PST Andrew Benson wrote: > I've attached on updated patch for PR83113. The now removes the ICE when a > duplicate DIMENSION attribute is specified in a submodule function. The > patch reg tests cleanly. > > OK to commit? > > Note that this patch doesn't check that the attributes of duplicate > declarations in a submodule function are valid or consistent with those > declared in the module. For example both: > > module mm > implicit none > interface > module function c() >integer, dimension(2) :: c !! Dimension 2 here > end function c > end interface > end module mm > > submodule (mm) oo > implicit none > contains > module function c() > integer, dimension(3) :: c !! Inconsistent dimension 3 here > end function c > end submodule oo > > > and > > > module mm > implicit none > interface > module function c() >integer, dimension(2) :: c !! Dimension 2 here > end function c > end interface > end module mm > > submodule (mm) oo > implicit none > contains > module function c() > integer, dimension(:) :: c !! Invalid cannot have a deferred shape > end function c > end submodule oo > > > compile successfully, but should be rejected. Presumably we should add some > check that the attributes of the declaration in the submodule match those in > the module? > > If so, I can go ahead and open a PR for this problem. > > -Andrew > > On Friday, January 24, 2020 10:54:24 AM PST Andrew Benson wrote: > > Below is a partial patch for PR 83113 > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83113 > > > > It simply circumvents the check for duplicate attributes when looking at > > declarations in a module procedure during compilation of a submodule. > > > > As it stands, the patch handles the cases of "allocatable", "dimension", > > and "pointer" attributes (corresponding to the two test cases in the PR > > plus a case that showed up in my own code). There are other attributes > > which should be handled similarly but before I make those changes I > > wanted to seek some opinions on whether this is the correct/best way to > > fix this issue. > > > > The patch regression tests cleanly - however, while the patch solves the > > "duplicate attribute" problem for the test case in comment #4: > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83113#c4 > > > > that test case then ICEs with: > > > > $ gfortran -c pr83113_2.F90 -o pr83113_2.o > > f951: internal compiler error: in gfc_set_array_spec, at > > fortran/array.c:879 0x5fd22f gfc_set_array_spec(gfc_symbol*, > > gfc_array_spec*, locus*) > > > > ../../gcc-git/gcc/fortran/array.c:879 > > > > 0x7e868e build_sym > > > > ../../gcc-git/gcc/fortran/decl.c:1670 > > > > 0x7f0ada variable_decl > > > > ../../gcc-git/gcc/fortran/decl.c:2760 > > > > 0x7f0ada gfc_match_data_decl() > > > > ../../gcc-git/gcc/fortran/decl.c:6120 > > > > 0x85b66d match_word > > > > ../../gcc-git/gcc/fortran/parse.c:65 > > > > 0x85b66d decode_statement > > > > ../../gcc-git/gcc/fortran/parse.c:376 > > > > 0x861094 next_free > > > > ../../gcc-git/gcc/fortran/parse.c:1279 > > > > 0x861094 next_statement > > > > ../../gcc-git/gcc/fortran/parse.c:1511 > > > > 0x8638ac parse_spec > > > > ../../gcc-git/gcc/fortran/parse.c:3738 > > > > 0x86591c parse_progunit > > > > ../../gcc-git/gcc/fortran/parse.c:5851 > > > > 0x865df9 parse_contained > > > > ../../gcc-git/gcc/fortran/parse.c:5752 > > > > 0x866b5e parse_module > > > > ../../gcc-git/gcc/fortran/parse.c:6124 > > > > 0x86709c gfc_parse_file() > > > > ../../gcc-git/gcc/fortran/parse.c:6427 > > > > 0x8b756f gfc_be_parse_file > > > > ../../gcc-git/gcc/fortran/f95-lang.c:210 > > > > 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. > > > > > > This looks like a separate problem to me, but looking quickly at where the > > ICE occurs it seems to be related to coarrays of which my understanding is > > very limited - so any insights on this would be welcome. > > > > Patch is appended below. > > > > -Andrew -- * Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html * Galacticus: https://github.com/galacticusorg/galacticus
patch, fortan] PR83113 Bogus "duplicate allocatable attribute" error for submodule character function
I've attached on updated patch for PR83113. The now removes the ICE when a duplicate DIMENSION attribute is specified in a submodule function. The patch reg tests cleanly. OK to commit? Note that this patch doesn't check that the attributes of duplicate declarations in a submodule function are valid or consistent with those declared in the module. For example both: module mm implicit none interface module function c() integer, dimension(2) :: c !! Dimension 2 here end function c end interface end module mm submodule (mm) oo implicit none contains module function c() integer, dimension(3) :: c !! Inconsistent dimension 3 here end function c end submodule oo and module mm implicit none interface module function c() integer, dimension(2) :: c !! Dimension 2 here end function c end interface end module mm submodule (mm) oo implicit none contains module function c() integer, dimension(:) :: c !! Invalid cannot have a deferred shape end function c end submodule oo compile successfully, but should be rejected. Presumably we should add some check that the attributes of the declaration in the submodule match those in the module? If so, I can go ahead and open a PR for this problem. -Andrew On Friday, January 24, 2020 10:54:24 AM PST Andrew Benson wrote: > Below is a partial patch for PR 83113 > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83113 > > It simply circumvents the check for duplicate attributes when looking at > declarations in a module procedure during compilation of a submodule. > > As it stands, the patch handles the cases of "allocatable", "dimension", and > "pointer" attributes (corresponding to the two test cases in the PR plus a > case that showed up in my own code). There are other attributes which > should be handled similarly but before I make those changes I wanted to > seek some opinions on whether this is the correct/best way to fix this > issue. > > The patch regression tests cleanly - however, while the patch solves the > "duplicate attribute" problem for the test case in comment #4: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83113#c4 > > that test case then ICEs with: > > $ gfortran -c pr83113_2.F90 -o pr83113_2.o > f951: internal compiler error: in gfc_set_array_spec, at fortran/array.c:879 > 0x5fd22f gfc_set_array_spec(gfc_symbol*, gfc_array_spec*, locus*) > ../../gcc-git/gcc/fortran/array.c:879 > 0x7e868e build_sym > ../../gcc-git/gcc/fortran/decl.c:1670 > 0x7f0ada variable_decl > ../../gcc-git/gcc/fortran/decl.c:2760 > 0x7f0ada gfc_match_data_decl() > ../../gcc-git/gcc/fortran/decl.c:6120 > 0x85b66d match_word > ../../gcc-git/gcc/fortran/parse.c:65 > 0x85b66d decode_statement > ../../gcc-git/gcc/fortran/parse.c:376 > 0x861094 next_free > ../../gcc-git/gcc/fortran/parse.c:1279 > 0x861094 next_statement > ../../gcc-git/gcc/fortran/parse.c:1511 > 0x8638ac parse_spec > ../../gcc-git/gcc/fortran/parse.c:3738 > 0x86591c parse_progunit > ../../gcc-git/gcc/fortran/parse.c:5851 > 0x865df9 parse_contained > ../../gcc-git/gcc/fortran/parse.c:5752 > 0x866b5e parse_module > ../../gcc-git/gcc/fortran/parse.c:6124 > 0x86709c gfc_parse_file() > ../../gcc-git/gcc/fortran/parse.c:6427 > 0x8b756f gfc_be_parse_file > ../../gcc-git/gcc/fortran/f95-lang.c:210 > 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. > > > This looks like a separate problem to me, but looking quickly at where the > ICE occurs it seems to be related to coarrays of which my understanding is > very limited - so any insights on this would be welcome. > > Patch is appended below. > > -Andrew -- * Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html * Galacticus: https://github.com/galacticusorg/galacticus diff --git a/gcc/fortran/array.c b/gcc/fortran/array.c index c873cf2..ff1ff72 100644 --- a/gcc/fortran/array.c +++ b/gcc/fortran/array.c @@ -23,6 +23,7 @@ along with GCC; see the file COPYING3. If not see #include "coretypes.h" #include "options.h" #include "gfortran.h" +#include "parse.h" #include "match.h" #include "constructor.h" @@ -835,6 +836,13 @@ gfc_set_array_spec (gfc_symbol *sym, gfc_array_spec *as, locus *error_loc) if (as == NULL) return true; + /* If the symbol corresponds to a submodule module procedure the array spec is + already set, so do not attempt to set it again here. */ + if (gfc_state_stack->previous &&
Re: [patch, fortan] PR87103 - [OOP] ICE in gfc_new_symbol() due to overlong symbol name
Thanks Bernhard - this is now committed: https://gcc.gnu.org/g:004ac7b780308dc899e565b887c7def0a6e100f2 On Thursday, January 30, 2020 5:27:55 PM PST Bernhard Reutner-Fischer wrote: > On 29 January 2020 21:19:52 CET, Andrew Benson wrote: > >I think this patch is still waiting to be applied. I checked that it > >applies > >against trunk (with line offsets) and reg tests cleanly and posted an > >updated > >version (diff'd against current trunk) at: > > > >https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87103#c7 > > > >I'm happy to go ahead and commit this if Bernhard is ok with me doing > >so. > > Please go ahead and push it. > Many thanks in advance and sorry for the delay! > thanks, > > >-Andrew > > > >On Wednesday, August 28, 2019 9:00:36 PM PST Bernhard Reutner-Fischer > > > >wrote: > >> On Fri, 23 Aug 2019 17:17:37 -0700 > >> > >> Andrew Benson wrote: > >> > This PR is still open - I re-tested the patch on the current trunk. > > > >The > > > >> > patch still applies with some line offsets (I've attached the > > > >updated > > > >> > patch) and regtests cleanly. It would be very helpful to me to get > > > >this > > > >> > patch committed if possible. > >> > >> I think Jerry ACKed the patch back then. I'll try to find the time to > >> commit it maybe during one of the coming weekends unless someone else > >> beats me to it.. > >> > >> Thanks for the reminder! > >> Bernhard > >> > >> > Thanks, > >> > Andrew > >> > > >> > On Wednesday, September 5, 2018 12:35:04 PM PDT Bernhard > > > >Reutner-Fischer > > > >> > wrote: > >> > > On Wed, 5 Sep 2018 at 03:30, Jerry DeLisle > > > > > > > >wrote: > >> > > > On 09/04/2018 10:43 AM, Bernhard Reutner-Fischer wrote: > >> > > > > On Tue, 4 Sep 2018 at 18:43, Andrew Benson > >> > > > > > >> > > >> > wrote: > >> > > > >> 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/for > > > >> > > > > tran > >> > > > > -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 > >> > > > >> > > 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: http://users.obs.carnegiescience.edu/abenson/contact.html * Galacticus: https://github.com/galacticusorg/galacticus
Re: [patch, fortan] PR87103 - [OOP] ICE in gfc_new_symbol() due to overlong symbol name
I think this patch is still waiting to be applied. I checked that it applies against trunk (with line offsets) and reg tests cleanly and posted an updated version (diff'd against current trunk) at: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87103#c7 I'm happy to go ahead and commit this if Bernhard is ok with me doing so. -Andrew On Wednesday, August 28, 2019 9:00:36 PM PST Bernhard Reutner-Fischer wrote: > On Fri, 23 Aug 2019 17:17:37 -0700 > > Andrew Benson wrote: > > This PR is still open - I re-tested the patch on the current trunk. The > > patch still applies with some line offsets (I've attached the updated > > patch) and regtests cleanly. It would be very helpful to me to get this > > patch committed if possible. > > I think Jerry ACKed the patch back then. I'll try to find the time to > commit it maybe during one of the coming weekends unless someone else > beats me to it.. > > Thanks for the reminder! > Bernhard > > > Thanks, > > Andrew > > > > On Wednesday, September 5, 2018 12:35:04 PM PDT Bernhard Reutner-Fischer > > > > wrote: > > > On Wed, 5 Sep 2018 at 03:30, Jerry DeLisle wrote: > > > > On 09/04/2018 10:43 AM, Bernhard Reutner-Fischer wrote: > > > > > On Tue, 4 Sep 2018 at 18:43, Andrew Benson > > > > > > > > > wrote: > > > > >> 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/for > > > > > tran > > > > > -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 > > > > > > 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: http://users.obs.carnegiescience.edu/abenson/contact.html * Galacticus: https://github.com/galacticusorg/galacticus
Re: [patch, fortran, wwwdocs] PR93461 - Bogus "symbol is already defined" with long subroutine names in submodule
Thanks Tobias and Gerald - that change is now committed: https://gcc.gnu.org/git/?p=gcc-wwwdocs.git;a=commit;h=b88b1edba71bbca44429c28b7518d76678ab6acb On Wednesday, January 29, 2020 11:57:02 AM PST Tobias Burnus wrote: > LGTM – and also to Gerald. Hence, go ahead! > > Thanks for the patch, > > Tobias > > On 1/29/20 1:45 AM, Andrew Benson wrote: > > Hi Tobias, > > > > On Tuesday, January 28, 2020 6:49:54 PM PST Tobias Burnus wrote: > >> Thus, I do not think it is a real problem in practice. We could be nice, > >> however, and add a note to the release notes (i.e. > >> https://gcc.gnu.org/gcc-10/changes.html); I not completely sure whether > >> it is worthwhile but why not. [See https://gcc.gnu.org/about.html#git > >> about how to change WWW files and CC Gerald as web maintainer when > >> submitting wwwdocs patches.] > > > > I've attached a draft patch to update the release notes about this ABI > > breakage. I don't know if I've explained it sufficiently clearly though? > > > > -Andrew -- * Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html * Galacticus: https://github.com/galacticusorg/galacticus
Re: [patch, fortran, wwwdocs] PR93461 - Bogus "symbol is already defined" with long subroutine names in submodule
Hi Tobias, On Tuesday, January 28, 2020 6:49:54 PM PST Tobias Burnus wrote: > Thus, I do not think it is a real problem in practice. We could be nice, > however, and add a note to the release notes (i.e. > https://gcc.gnu.org/gcc-10/changes.html); I not completely sure whether > it is worthwhile but why not. [See https://gcc.gnu.org/about.html#git > about how to change WWW files and CC Gerald as web maintainer when > submitting wwwdocs patches.] I've attached a draft patch to update the release notes about this ABI breakage. I don't know if I've explained it sufficiently clearly though? -Andrew -- * Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html * Galacticus: https://github.com/galacticusorg/galacticus diff --git a/htdocs/gcc-10/changes.html b/htdocs/gcc-10/changes.html index dcce6b8..5a959a1 100644 --- a/htdocs/gcc-10/changes.html +++ b/htdocs/gcc-10/changes.html @@ -446,6 +446,13 @@ a work-in-progress. objects with allocatable components. In this case, the optional arguments STAT= and ERRMSG= are currently ignored. + +The handling of module and submodule names has been reworked to allow the +full 63-character length mandated by the standard. Previously symbol names +were truncated if the combined length of module, submodule, and function +name exceeded 126 characters. This change therefore breaks the ABI, but only +for cases where this 126 character limit was exceeded. +
[patch, fortran] PR93486 - ICE on valid with nested submodules and long submodule names
I opened PR93486 for this problem:
The following causes an ICE with revision
ad690d79cfbb905c5546c9333c5fd089d906505b:
module ivs
interface l
module procedure l_
end interface l
contains
function l_()
end function l_
end module ivs
module aModeratleyLongModuleName
use ivs
interface
module subroutine cmo()
end subroutine cmo
end interface
end module aModeratleyLongModuleName
submodule (aModeratleyLongModuleName)
aNameForASubmoduleThatIsVeryLongButWhichIsLegalStill
contains
module procedure cmo
end procedure cmo
end submodule aNameForASubmoduleThatIsVeryLongButWhichIsLegalStill
submodule
(aModeratleyLongModuleName:aNameForASubmoduleThatIsVeryLongButWhichIsLegalStill)
sb
end submodule sb
submodule (aModeratleyLongModuleName:sb) sc
end submodule sc
$ gfortran -v
Using built-in specs.
COLLECT_GCC=gfortran
COLLECT_LTO_WRAPPER=/data001/abenson/Galacticus/Tools_Devel_Install/bin/../
libexec/gcc/x86_64-pc-linux-gnu/10.0.1/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: ../gcc-git/configure --prefix=/home/abenson/Galacticus/
Tools_Devel --enable-languages=c,c++,fortran --disable-multilib
Thread model: posix
Supported LTO compression algorithms: zlib
gcc version 10.0.1 20200128 (experimental) (GCC)
$ gfortran -c test.F90 -o test.o
f951: internal compiler error: Segmentation fault
0xe1bc9f crash_signal
../../gcc-git/gcc/toplev.c:328
0x7f98119c71ef ???
/data001/abenson/Galacticus/Tools/glibc-2.12.1/signal/../sysdeps/unix/
sysv/linux/x86_64/sigaction.c:0
0x84b3f0 gfc_use_modules()
../../gcc-git/gcc/fortran/module.c:7315
0x85acc8 use_modules
../../gcc-git/gcc/fortran/parse.c:114
0x866daa parse_module
../../gcc-git/gcc/fortran/parse.c:6111
0x86733c gfc_parse_file()
../../gcc-git/gcc/fortran/parse.c:6428
0x8b780f gfc_be_parse_file
../../gcc-git/gcc/fortran/f95-lang.c:210
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 ICE goes away if the module and/or submodule names are made shorter.
The problem is caused when loading (generic or operator) interfaces from a
module file. The module name can include the submodule name, so a size of
2*GFC_MAX_SYMBOL_LEN+2 is required to read this in.
The attached patch fixes this problem and regression tests cleanly *if* the
patch for PR87103 is applied (otherwise that problem gets triggered by the new
test case).
PR87103 has been a long-standing issue - there's a patch that works (either my
original ugly fix, or the better fix proposed by Bernhard at https://
gcc.gnu.org/ml/fortran/2018-09/msg00044.html ). It would be very nice to get
one of these patches committed.
-Andrew
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html
* Galacticus: https://github.com/galacticusorg/galacticus
diff --git a/gcc/fortran/module.c b/gcc/fortran/module.c
index 4487f65..b6a4e87 100644
--- a/gcc/fortran/module.c
+++ b/gcc/fortran/module.c
@@ -4568,7 +4568,9 @@ static void
load_operator_interfaces (void)
{
const char *p;
- char name[GFC_MAX_SYMBOL_LEN + 1], module[GFC_MAX_SYMBOL_LEN + 1];
+ /* "module" must be large enough for the case of submodules in which the name
+ has the form module.submodule */
+ char name[GFC_MAX_SYMBOL_LEN + 1], module[2 * GFC_MAX_SYMBOL_LEN + 2];
gfc_user_op *uop;
pointer_info *pi = NULL;
int n, i;
@@ -4624,7 +4626,9 @@ static void
load_generic_interfaces (void)
{
const char *p;
- char name[GFC_MAX_SYMBOL_LEN + 1], module[GFC_MAX_SYMBOL_LEN + 1];
+ /* "module" must be large enough for the case of submodules in which the name
+ has the form module.submodule */
+ char name[GFC_MAX_SYMBOL_LEN + 1], module[2 * GFC_MAX_SYMBOL_LEN + 2];
gfc_symbol *sym;
gfc_interface *generic = NULL, *gen = NULL;
int n, i, renamed;
diff --git a/gcc/testsuite/gfortran.dg/pr93486.f90 b/gcc/testsuite/gfortran.dg/pr93486.f90
new file mode 100644
index 000..5037d40
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr93486.f90
@@ -0,0 +1,30 @@
+! { dg-do compile }
+! PR fortran/93486
+module ivs
+ interface l
+ module procedure l_
+ end interface l
+contains
+ function l_()
+ end function l_
+end module ivs
+
+module aModeratleyLongModuleName
+ use ivs
+ interface
+ module subroutine cmo()
+ end subroutine cmo
+ end interface
+end module aModeratleyLongModuleName
+
+submodule (aModeratleyLongModuleName) aNameForASubmoduleThatIsVeryLongButWhichIsLegalStill
+contains
+ module procedure cmo
+ end procedure cmo
+end submodule aNameForASubmoduleThatIsVeryLongButWhichIsLegalStill
+
+submodule (aModeratleyLongModuleName:aNameForASubmoduleThatIsVeryLongButWhichIsLegalStill) sb
+end submodule sb
+
+submodule (aModeratleyLongModuleName:sb) sc
+end submodule sc
2020-01-28 Andrew Benson
PR fortran/93486
* module.c: Increase size of var
Re: [patch, fortran] PR93461 - Bogus "symbol is already defined" with long subroutine names in submodule
Hi Tobias,
Thanks - committed as
https://gcc.gnu.org/git/gitweb.cgi?
p=gcc.git;a=commit;h=ad690d79cfbb905c5546c9333c5fd089d906505b
I'll send a separate email about changes to the release notes.
-Andrew
On Tuesday, January 28, 2020 6:49:54 PM PST Tobias Burnus wrote:
> Hi Andrew,
>
> On 1/28/20 5:41 PM, Andrew Benson wrote:
> >> Can you also update the comment before the #define? It currently has:
> > Thanks for the suggestion. An updated patch is attached.
>
> Thanks. LGTM now.
>
> >> PS: I wonder whether there are relevant systems which will fail because
> >> they do not handle that long symbol names...
> >
> > Good question. Should I hold off on committing the patch until this can be
> > tested further? Or should I just go ahead and commit and deal with any
> > such
> > problems if they show up?
>
> I think it is fine – as a user, one can always choose a shorter name if
> the system one is interested in doesn't support it. Besides, following
> the current scheme, we do not really have a choice without breaking the ABI.
> > Also, Richard Biener raised the question (in the PR) of whether this patch
> > would be an ABI change. I can see that it probably would be, but don't
> > know
> > for sure. If it would change the ABI is there anything else that I need to
> > include in the patch?
>
> As just mentioned on the PR, I think it is a small ABI change – before a
> very long identified got cut off now it is available in full extent. — I
> do not see any simple way to avoid this ABI change and, frankly, I also
> do not think any real-world program uses that long identifiers.
>
> Namely, instead of using 3 times the length of 42 character
> ("ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnop") the standard permits 3
> times the length of 63 characters
> (ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz1234567890_).
> Already 42 is quite long for a module, submodule or procedure name. And
> that does work even without the patch. And as only the sum counts, if
> one part only uses 10 characters (e.g. "my_module5"), now 2*58
> characters available two others.
>
> If at the end it really causes problems (source code not available), the
> user still can modify the module file and change the procedure name to
> the trimmed value and continue.
>
> Thus, I do not think it is a real problem in practice. We could be nice,
> however, and add a note to the release notes (i.e.
> https://gcc.gnu.org/gcc-10/changes.html); I not completely sure whether
> it is worthwhile but why not. [See https://gcc.gnu.org/about.html#git
> about how to change WWW files and CC Gerald as web maintainer when
> submitting wwwdocs patches.]
>
> Thanks,
>
> Tobias
>
> > patch.diff
> >
> > diff --git a/gcc/fortran/trans.h b/gcc/fortran/trans.h
> > index 52bc045..69171f3 100644
> > --- a/gcc/fortran/trans.h
> > +++ b/gcc/fortran/trans.h
> > @@ -23,8 +23,8 @@ along with GCC; see the file COPYING3. If not see
> >
> > #include "predict.h" /* For enum br_predictor and PRED_*. */
> >
> > -/* Mangled symbols take the form __module__name. */
> > -#define GFC_MAX_MANGLED_SYMBOL_LEN (GFC_MAX_SYMBOL_LEN*2+4)
> > +/* Mangled symbols take the form __module__name or
> > __module.submodule__name. */ +#define GFC_MAX_MANGLED_SYMBOL_LEN
> > (GFC_MAX_SYMBOL_LEN*3+5)
> >
> > /* Struct for holding a block of statements. It should be treated as an
> >
> > opaque entity and not modified directly. This allows us to change
> > the
> >
> > diff --git a/gcc/testsuite/gfortran.dg/pr93461.f90
> > b/gcc/testsuite/gfortran.dg/pr93461.f90 new file mode 100644
> > index 000..3bef326
> > --- /dev/null
> > +++ b/gcc/testsuite/gfortran.dg/pr93461.f90
> > @@ -0,0 +1,22 @@
> > +! { dg-do compile }
> > +! PR fortran/93461
> > +module aModuleWithAnAllowedName
> > + interface
> > + module subroutine aShortName()
> > + end subroutine aShortName
> > + end interface
> > +end module aModuleWithAnAllowedName
> > +
> > +submodule (aModuleWithAnAllowedName)
> > aSubmoduleWithAVeryVeryVeryLongButEntirelyLegalName +contains
> > + subroutine aShortName()
> > +call aSubroutineWithAVeryLongNameThatWillCauseAProblem()
> > +call aSubroutineWithAVeryLongNameThatWillCauseAProblemAlso()
> > + end subroutine aShortName
> > +
> > + subroutine aSubroutineWithAVeryLongNameThatWillCauseAProblem()
> > + end subroutine aSubroutineWithAVeryLongNameThatWillCauseAProblem
> > +
> > + subroutine aSubroutineWithAVeryLongNameThat
Re: [patch, fortran] PR93473 - ICE on valid with long module + submodule names
Thanks - committed as a83b5cc5828ee34471de415e8893242dd3b0a91b https://gcc.gnu.org/git/gitweb.cgi? p=gcc.git;a=commit;h=a83b5cc5828ee34471de415e8893242dd3b0a91b I'll close the PR. Thanks, Andrew On Tuesday, January 28, 2020 6:32:21 PM PST Tobias Burnus wrote: > LGTM now. > > Thanks, > > Tobias > > On 1/28/20 5:41 PM, Andrew Benson wrote: > > On Tuesday, January 28, 2020 9:27:58 AM PST Tobias Burnus wrote: > >> On 1/28/20 12:41 AM, Andrew Benson wrote: > >>> The problem occurs in set_syms_host_assoc() where the "parent1" and > >>> "parent2" variables have a maximum length of GFC_MAX_SYMBOL_LEN+1. This > >>> is insufficient when the parent names are a module+submodule name > >>> concatenated with a ".". The patch above fixes this by increasing their > >>> length to 2*GFC_MAX_SYMBOL_LEN+2. > >>> > >>> A patch to fix this is attached. The patch regression tests cleanly - ok > >>> to > >>> commit? > >> > >> The patch is okay, but can you add a comment – similar to the other > >> patch – which makes clear why one needs twice the normal > >> > >> GFC_MAX_SYMBOL_LEN (+2)? You currently have: > >>> const char dot[2] = "."; > >>> > >>> - char parent1[GFC_MAX_SYMBOL_LEN + 1]; > >>> - char parent2[GFC_MAX_SYMBOL_LEN + 1]; > >>> + char parent1[2 * GFC_MAX_SYMBOL_LEN + 2]; > >>> + char parent2[2 * GFC_MAX_SYMBOL_LEN + 2]; > > > > Yes - I've added a comment here explaining the naming convention. An > > updated patch is attached. > > > > -Andrew -- * Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html * Galacticus: https://github.com/galacticusorg/galacticus
Re: [patch, fortran] PR93461 - Bogus "symbol is already defined" with long subroutine names in submodule
Hi Tobias,
> > The problem occurs because GFC_MAX_MANGLED_SYMBOL_LEN is set to
> > GFC_MAX_SYMBOL_LEN*2+4, which is sufficient for a module name plus
> > function name (plus the additional "_"'s that get prepended), but
> > insufficient if a submodule name is included. The name then gets
> > truncated and can lead to two different functions having the same
> > (truncated) symbol name.
> >
> > The fix is to increase this length to GFC_MAX_SYMBOL_LEN*3+5 - which
> > allows for the submodule name plus the "." added between module and
> > submodule names.
> >
> > I've attached a patch for this which includes a new test case for this PR.
> > The patch regression tests cleanly.
> >
> > OK to commit?
>
> Can you also update the comment before the #define? It currently has:
>
> /* Mangled symbols take the form __module__name. */
> -#define GFC_MAX_MANGLED_SYMBOL_LEN (GFC_MAX_SYMBOL_LEN*2+4)
> +#define GFC_MAX_MANGLED_SYMBOL_LEN (GFC_MAX_SYMBOL_LEN*3+5)
Thanks for the suggestion. An updated patch is attached.
> PS: I wonder whether there are relevant systems which will fail because they
> do not handle that long symbol names...
Good question. Should I hold off on committing the patch until this can be
tested further? Or should I just go ahead and commit and deal with any such
problems if they show up?
Also, Richard Biener raised the question (in the PR) of whether this patch
would be an ABI change. I can see that it probably would be, but don't know
for sure. If it would change the ABI is there anything else that I need to
include in the patch?
Thanks,
Andrew
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html
* Galacticus: https://github.com/galacticusorg/galacticus
diff --git a/gcc/fortran/trans.h b/gcc/fortran/trans.h
index 52bc045..69171f3 100644
--- a/gcc/fortran/trans.h
+++ b/gcc/fortran/trans.h
@@ -23,8 +23,8 @@ along with GCC; see the file COPYING3. If not see
#include "predict.h" /* For enum br_predictor and PRED_*. */
-/* Mangled symbols take the form __module__name. */
-#define GFC_MAX_MANGLED_SYMBOL_LEN (GFC_MAX_SYMBOL_LEN*2+4)
+/* Mangled symbols take the form __module__name or __module.submodule__name. */
+#define GFC_MAX_MANGLED_SYMBOL_LEN (GFC_MAX_SYMBOL_LEN*3+5)
/* Struct for holding a block of statements. It should be treated as an
opaque entity and not modified directly. This allows us to change the
diff --git a/gcc/testsuite/gfortran.dg/pr93461.f90 b/gcc/testsuite/gfortran.dg/pr93461.f90
new file mode 100644
index 000..3bef326
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr93461.f90
@@ -0,0 +1,22 @@
+! { dg-do compile }
+! PR fortran/93461
+module aModuleWithAnAllowedName
+ interface
+ module subroutine aShortName()
+ end subroutine aShortName
+ end interface
+end module aModuleWithAnAllowedName
+
+submodule (aModuleWithAnAllowedName) aSubmoduleWithAVeryVeryVeryLongButEntirelyLegalName
+contains
+ subroutine aShortName()
+call aSubroutineWithAVeryLongNameThatWillCauseAProblem()
+call aSubroutineWithAVeryLongNameThatWillCauseAProblemAlso()
+ end subroutine aShortName
+
+ subroutine aSubroutineWithAVeryLongNameThatWillCauseAProblem()
+ end subroutine aSubroutineWithAVeryLongNameThatWillCauseAProblem
+
+ subroutine aSubroutineWithAVeryLongNameThatWillCauseAProblemAlso()
+ end subroutine aSubroutineWithAVeryLongNameThatWillCauseAProblemAlso
+end submodule aSubmoduleWithAVeryVeryVeryLongButEntirelyLegalName
2020-01-28 Andrew Benson
PR fortran/93461
* trans.h: Increase GFC_MAX_MANGLED_SYMBOL_LEN to
GFC_MAX_SYMBOL_LEN*3+5 to allow for inclusion of submodule name,
plus the "." between module and submodule names.
* gfortran.dg/pr93461.f90: New test.
Re: [patch, fortran] PR93473 - ICE on valid with long module + submodule names
On Tuesday, January 28, 2020 9:27:58 AM PST Tobias Burnus wrote:
> On 1/28/20 12:41 AM, Andrew Benson wrote:
> > The problem occurs in set_syms_host_assoc() where the "parent1" and
> > "parent2" variables have a maximum length of GFC_MAX_SYMBOL_LEN+1. This
> > is insufficient when the parent names are a module+submodule name
> > concatenated with a ".". The patch above fixes this by increasing their
> > length to 2*GFC_MAX_SYMBOL_LEN+2.
> >
> > A patch to fix this is attached. The patch regression tests cleanly - ok
> > to
> > commit?
>
> The patch is okay, but can you add a comment – similar to the other
> patch – which makes clear why one needs twice the normal
>
> GFC_MAX_SYMBOL_LEN (+2)? You currently have:
> > const char dot[2] = ".";
> >
> > - char parent1[GFC_MAX_SYMBOL_LEN + 1];
> > - char parent2[GFC_MAX_SYMBOL_LEN + 1];
> > + char parent1[2 * GFC_MAX_SYMBOL_LEN + 2];
> > + char parent2[2 * GFC_MAX_SYMBOL_LEN + 2];
Yes - I've added a comment here explaining the naming convention. An updated
patch is attached.
-Andrew
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html
* Galacticus: https://github.com/galacticusorg/galacticus
2020-01-28 Andrew Benson
PR fortran/93473
* parse.c: Increase length of char variables to allow them to hold
a concatenated module + submodule name.
* gfortran.dg/pr93473.f90: New test.
diff --git a/gcc/fortran/parse.c b/gcc/fortran/parse.c
index 4bff0c8..f71a95d 100644
--- a/gcc/fortran/parse.c
+++ b/gcc/fortran/parse.c
@@ -6045,8 +6045,9 @@ set_syms_host_assoc (gfc_symbol *sym)
{
gfc_component *c;
const char dot[2] = ".";
- char parent1[GFC_MAX_SYMBOL_LEN + 1];
- char parent2[GFC_MAX_SYMBOL_LEN + 1];
+ /* Symbols take the form module.submodule_ or module.name_. */
+ char parent1[2 * GFC_MAX_SYMBOL_LEN + 2];
+ char parent2[2 * GFC_MAX_SYMBOL_LEN + 2];
if (sym == NULL)
return;
diff --git a/gcc/testsuite/gfortran.dg/pr93473.f90 b/gcc/testsuite/gfortran.dg/pr93473.f90
new file mode 100644
index 000..dda8525
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr93473.f90
@@ -0,0 +1,28 @@
+! { dg-do compile }
+! { dg-options "-ffree-line-length-none" }
+! PR fortran/93473
+module aModestlyLongModuleName
+
+ type :: aTypeWithASignificantlyLongNameButStillAllowedOK
+ end type aTypeWithASignificantlyLongNameButStillAllowedOK
+
+ interface
+ module function aFunctionWithALongButStillAllowedName(parameters) result(self)
+ type(aTypeWithASignificantlyLongNameButStillAllowedOK) :: self
+ end function aFunctionWithALongButStillAllowedName
+ end interface
+
+end module aModestlyLongModuleName
+
+submodule (aModestlyLongModuleName) aTypeWithASignificantlyLongNameButStillAllowedOK_
+
+contains
+
+ module procedure aFunctionWithALongButStillAllowedName
+ class(*), pointer :: genericObject
+ end procedure aFunctionWithALongButStillAllowedName
+
+end submodule aTypeWithASignificantlyLongNameButStillAllowedOK_
+
+submodule (aModestlyLongModuleName:aTypeWithASignificantlyLongNameButStillAllowedOK_) aSubmoduleWithASignificantlyLongButStillAllowedName__
+end submodule aSubmoduleWithASignificantlyLongButStillAllowedName__
[patch, fortran] PR93473 - ICE on valid with long module + submodule names
I created PR93473 for this problem: The following code causes a bogus "symbol
is already defined" error (using git commit
472dc648ce3e7661762931d584d239611ddca964):
module aModestlyLongModuleName
type :: aTypeWithASignificantlyLongNameButStillAllowedOK
end type aTypeWithASignificantlyLongNameButStillAllowedOK
interface
module function aFunctionWithALongButStillAllowedName(parameters)
result(self)
type(aTypeWithASignificantlyLongNameButStillAllowedOK) :: self
end function aFunctionWithALongButStillAllowedName
end interface
end module aModestlyLongModuleName
submodule (aModestlyLongModuleName)
aTypeWithASignificantlyLongNameButStillAllowedOK_
contains
module procedure aFunctionWithALongButStillAllowedName
class(*), pointer :: genericObject
end procedure aFunctionWithALongButStillAllowedName
end submodule aTypeWithASignificantlyLongNameButStillAllowedOK_
submodule
(aModestlyLongModuleName:aTypeWithASignificantlyLongNameButStillAllowedOK_)
aSubmoduleWithASignificantlyLongButStillAllowedName__
end submodule aSubmoduleWithASignificantlyLongButStillAllowedName__
$ gfortran -v
Using built-in specs.
COLLECT_GCC=gfortran
COLLECT_LTO_WRAPPER=/data001/abenson/Galacticus/Tools_Devel_Install/bin/../
libexec/gcc/x86_64-pc-linux-gnu/10.0.1/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: ../gcc-git/configure --prefix=/home/abenson/Galacticus/
Tools_Devel --enable-languages=c,c++,fortran --disable-multilib
Thread model: posix
Supported LTO compression algorithms: zlib
gcc version 10.0.1 20200127 (experimental) (GCC)
$ gfortran -c 1057.F90 -o test.o -ffree-line-length-none
f951: internal compiler error: Segmentation fault
0xe1021f crash_signal
../../gcc-git/gcc/toplev.c:328
0x7fd1480c91ef ???
/data001/abenson/Galacticus/Tools/glibc-2.12.1/signal/../sysdeps/unix/
sysv/linux/x86_64/sigaction.c:0
0x891106 do_traverse_symtree
../../gcc-git/gcc/fortran/symbol.c:4173
0x85739b parse_module
../../gcc-git/gcc/fortran/parse.c:6111
0x85782d gfc_parse_file()
../../gcc-git/gcc/fortran/parse.c:6427
0x8a7f2f gfc_be_parse_file
../../gcc-git/gcc/fortran/f95-lang.c:210
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 occurs in set_syms_host_assoc() where the "parent1" and "parent2"
variables have a maximum length of GFC_MAX_SYMBOL_LEN+1. This is insufficient
when the parent names are a module+submodule name concatenated with a ".". The
patch above fixes this by increasing their length to 2*GFC_MAX_SYMBOL_LEN+2.
A patch to fix this is attached. The patch regression tests cleanly - ok to
commit?
-Andrew
diff --git a/gcc/fortran/parse.c b/gcc/fortran/parse.c
index 4bff0c8..cbace25 100644
--- a/gcc/fortran/parse.c
+++ b/gcc/fortran/parse.c
@@ -6045,8 +6045,8 @@ set_syms_host_assoc (gfc_symbol *sym)
{
gfc_component *c;
const char dot[2] = ".";
- char parent1[GFC_MAX_SYMBOL_LEN + 1];
- char parent2[GFC_MAX_SYMBOL_LEN + 1];
+ char parent1[2 * GFC_MAX_SYMBOL_LEN + 2];
+ char parent2[2 * GFC_MAX_SYMBOL_LEN + 2];
if (sym == NULL)
return;
diff --git a/gcc/testsuite/gfortran.dg/pr93473.f90 b/gcc/testsuite/gfortran.dg/pr93473.f90
new file mode 100644
index 000..dda8525
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr93473.f90
@@ -0,0 +1,28 @@
+! { dg-do compile }
+! { dg-options "-ffree-line-length-none" }
+! PR fortran/93473
+module aModestlyLongModuleName
+
+ type :: aTypeWithASignificantlyLongNameButStillAllowedOK
+ end type aTypeWithASignificantlyLongNameButStillAllowedOK
+
+ interface
+ module function aFunctionWithALongButStillAllowedName(parameters) result(self)
+ type(aTypeWithASignificantlyLongNameButStillAllowedOK) :: self
+ end function aFunctionWithALongButStillAllowedName
+ end interface
+
+end module aModestlyLongModuleName
+
+submodule (aModestlyLongModuleName) aTypeWithASignificantlyLongNameButStillAllowedOK_
+
+contains
+
+ module procedure aFunctionWithALongButStillAllowedName
+ class(*), pointer :: genericObject
+ end procedure aFunctionWithALongButStillAllowedName
+
+end submodule aTypeWithASignificantlyLongNameButStillAllowedOK_
+
+submodule (aModestlyLongModuleName:aTypeWithASignificantlyLongNameButStillAllowedOK_) aSubmoduleWithASignificantlyLongButStillAllowedName__
+end submodule aSubmoduleWithASignificantlyLongButStillAllowedName__
2020-01-27 Andrew Benson
PR fortran/93473
* parse.c: Increase length of char variables to allow them to hold
a concatenated module + submodule name.
* gfortran.dg/pr93473.f90: New test.
[patch, fortran] PR93461 - Bogus "symbol is already defined" with long subroutine names in submodule
I created PR93461 for this issue: The following code causes a bogus "symbol is
already defined" error (using git commit
73380abd6b2783215c7950a2ade5e3f4b271e2bc):
module aModuleWithAnAllowedName
interface
module subroutine aShortName()
end subroutine aShortName
end interface
end module aModuleWithAnAllowedName
submodule (aModuleWithAnAllowedName)
aSubmoduleWithAVeryVeryVeryLongButEntirelyLegalName
contains
subroutine aShortName()
call aSubroutineWithAVeryLongNameThatWillCauseAProblem()
call aSubroutineWithAVeryLongNameThatWillCauseAProblemAlso()
end subroutine aShortName
subroutine aSubroutineWithAVeryLongNameThatWillCauseAProblem()
end subroutine aSubroutineWithAVeryLongNameThatWillCauseAProblem
subroutine aSubroutineWithAVeryLongNameThatWillCauseAProblemAlso()
end subroutine aSubroutineWithAVeryLongNameThatWillCauseAProblemAlso
end submodule aSubmoduleWithAVeryVeryVeryLongButEntirelyLegalName
$ gfortran -v
Using built-in specs.
COLLECT_GCC=gfortran
COLLECT_LTO_WRAPPER=/data001/abenson/Galacticus/Tools_Devel_Install/bin/../
libexec/gcc/x86_64-pc-linux-gnu/10.0.1/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: ../gcc-git/configure --prefix=/home/abenson/Galacticus/
Tools_Devel --enable-languages=c,c++,fortran --disable-multilib
Thread model: posix
Supported LTO compression algorithms: zlib
gcc version 10.0.1 20200124 (experimental) (GCC)
$ gfortran -c symlength.F90 -o symlength.o -ffree-line-length-none -
frecursive -pthread -Wall -fbacktrace -ffpe-trap=invalid,zero,overflow -fdump-
core -O3 -ffinite-math-only -fno-math-errno -fopenmp -g
/tmp/cc8B4Hmp.s: Assembler messages:
/tmp/cc8B4Hmp.s:20: Error: symbol
`__amodulewithanallowedname.asubmodulewithaveryveryverylongbutentirelylegalname_MOD_asubroutinewithaverylongnamethatwillcauseaprobl'
is already defined
The problem occurs because GFC_MAX_MANGLED_SYMBOL_LEN is set to
GFC_MAX_SYMBOL_LEN*2+4, which is sufficient for a module name plus function
name
(plus the additional "_"'s that get prepended), but insufficient if a submodule
name is included. The name then gets truncated and can lead to two different
functions having the same (truncated) symbol name.
The fix is to increase this length to GFC_MAX_SYMBOL_LEN*3+5 - which allows for
the submodule name plus the "." added between module and submodule names.
I've attached a patch for this which includes a new test case for this PR. The
patch regression tests cleanly.
OK to commit?
-Andrew
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html
* Galacticus: https://github.com/galacticusorg/galacticus
diff --git a/gcc/fortran/trans.h b/gcc/fortran/trans.h
index 52bc045..5942320 100644
--- a/gcc/fortran/trans.h
+++ b/gcc/fortran/trans.h
@@ -24,7 +24,7 @@ along with GCC; see the file COPYING3. If not see
#include "predict.h" /* For enum br_predictor and PRED_*. */
/* Mangled symbols take the form __module__name. */
-#define GFC_MAX_MANGLED_SYMBOL_LEN (GFC_MAX_SYMBOL_LEN*2+4)
+#define GFC_MAX_MANGLED_SYMBOL_LEN (GFC_MAX_SYMBOL_LEN*3+5)
/* Struct for holding a block of statements. It should be treated as an
opaque entity and not modified directly. This allows us to change the
diff --git a/gcc/testsuite/gfortran.dg/pr93461.f90 b/gcc/testsuite/gfortran.dg/pr93461.f90
new file mode 100644
index 000..3bef326
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr93461.f90
@@ -0,0 +1,22 @@
+! { dg-do compile }
+! PR fortran/93461
+module aModuleWithAnAllowedName
+ interface
+ module subroutine aShortName()
+ end subroutine aShortName
+ end interface
+end module aModuleWithAnAllowedName
+
+submodule (aModuleWithAnAllowedName) aSubmoduleWithAVeryVeryVeryLongButEntirelyLegalName
+contains
+ subroutine aShortName()
+call aSubroutineWithAVeryLongNameThatWillCauseAProblem()
+call aSubroutineWithAVeryLongNameThatWillCauseAProblemAlso()
+ end subroutine aShortName
+
+ subroutine aSubroutineWithAVeryLongNameThatWillCauseAProblem()
+ end subroutine aSubroutineWithAVeryLongNameThatWillCauseAProblem
+
+ subroutine aSubroutineWithAVeryLongNameThatWillCauseAProblemAlso()
+ end subroutine aSubroutineWithAVeryLongNameThatWillCauseAProblemAlso
+end submodule aSubmoduleWithAVeryVeryVeryLongButEntirelyLegalName
2020-01-27 Andrew Benson
* trans.h: Increase GFC_MAX_MANGLED_SYMBOL_LEN to
GFC_MAX_SYMBOL_LEN*3+5 to allow for inclusion of submodule name,
plus the "." between module and submodule names.
Re: [patch, fortan] PR87103 - [OOP] ICE in gfc_new_symbol() due to overlong symbol name
Thanks Bernhard. On Wednesday, August 28, 2019 9:00:36 PM PDT Bernhard Reutner-Fischer wrote: > On Fri, 23 Aug 2019 17:17:37 -0700 > > Andrew Benson wrote: > > This PR is still open - I re-tested the patch on the current trunk. The > > patch still applies with some line offsets (I've attached the updated > > patch) and regtests cleanly. It would be very helpful to me to get this > > patch committed if possible. > > I think Jerry ACKed the patch back then. I'll try to find the time to > commit it maybe during one of the coming weekends unless someone else > beats me to it.. > > Thanks for the reminder! > Bernhard > > > Thanks, > > Andrew > > > > On Wednesday, September 5, 2018 12:35:04 PM PDT Bernhard Reutner-Fischer > > > > wrote: > > > On Wed, 5 Sep 2018 at 03:30, Jerry DeLisle wrote: > > > > On 09/04/2018 10:43 AM, Bernhard Reutner-Fischer wrote: > > > > > On Tue, 4 Sep 2018 at 18:43, Andrew Benson > > > > > > > > > wrote: > > > > >> 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/for > > > > > tran > > > > > -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 > > > > > > 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: http://users.obs.carnegiescience.edu/abenson/contact.html * Galacticus: https://bitbucket.org/galacticusdev/galacticus
Re: [patch, fortan] PR87103 - [OOP] ICE in gfc_new_symbol() due to overlong symbol name
This PR is still open - I re-tested the patch on the current trunk. The patch
still applies with some line offsets (I've attached the updated patch) and
regtests cleanly. It would be very helpful to me to get this patch committed
if possible.
Thanks,
Andrew
On Wednesday, September 5, 2018 12:35:04 PM PDT Bernhard Reutner-Fischer
wrote:
> On Wed, 5 Sep 2018 at 03:30, Jerry DeLisle wrote:
> > On 09/04/2018 10:43 AM, Bernhard Reutner-Fischer wrote:
> > > On Tue, 4 Sep 2018 at 18:43, Andrew Benson
wrote:
> > >> 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
>
> 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: http://users.obs.carnegiescience.edu/abenson/contact.html
* Galacticus: https://bitbucket.org/galacticusdev/galacticus
Index: gcc/fortran/expr.c
===
--- gcc/fortran/expr.c (revision 274889)
+++ gcc/fortran/expr.c (working copy)
@@ -3467,8 +3467,10 @@ gfc_check_conformance (gfc_expr *op1, gfc_expr *op
return true;
va_start (argp, optype_msgid);
- vsnprintf (buffer, 240, optype_msgid, argp);
+ d = vsnprintf (buffer, sizeof (buffer), optype_msgid, argp);
va_end (argp);
+ if (d < 1 || d >= (int) sizeof (buffer)) /* Reject truncation. */
+gfc_internal_error ("optype_msgid overflow: %d", d);
if (op1->rank != op2->rank)
{
Index: gcc/fortran/iresolve.c
===
--- gcc/fortran/iresolve.c (revision 274889)
+++ gcc/fortran/iresolve.c (working copy)
@@ -61,9 +61,12 @@ gfc_get_string (const char *format, ...)
}
else
{
+ int ret;
va_start (ap, format);
- vsnprintf (temp_name, sizeof (temp_name), format, ap);
+ ret = vsnprintf (temp_name, sizeof (temp_name), format, ap);
va_end (ap);
+ if (ret < 1 || ret >= (int) sizeof (temp_name)) /* Reject truncation. */
+ gfc_internal_error ("identifier overflow: %d", ret);
temp_name[sizeof (temp_name) - 1] = 0;
str = temp_name;
}
Index: gcc/fortran/symbol.c
===
--- gcc/fortran/symbol.c (revision 274889)
+++ gcc/fortran/symbol.c (working copy)
@@ -3135,25 +3135,9 @@ gfc_new_symbol (const char *name, gfc_namespace *n
gfc_clear_ts (>ts);
gfc_clear_attr (>attr);
p->ns = ns;
-
p->declared_at = gfc_current_locus;
-
- if (strlen (name) > GFC_MAX_SYMBOL_LEN)
-gfc_internal_error ("new_symbol(): Symbol name too long");
-
p->name = gfc_get_string ("%s", name);
- /* Make sure flags for symbol being C bound are clear initially. */
- p->attr.is_bind_c = 0;
- p->attr.is_iso_c = 0;
-
- /* Clear the ptrs we may need. */
- p->common_block = NULL;
- p->f2k_derived = NULL;
- p->assoc = NULL;
- p->dt_next = NULL;
- p->fn_result_spec = 0;
-
return p;
}
Re: [Patch, fortran] PR90786 - [7/8/9/10 Regression] ICE on procedure pointer assignment to function with class pointer result
Thanks Paul for the quick fix! On Saturday, June 8, 2019 4:56:46 PM PDT Paul Richard Thomas wrote: > Committed as obvious in revision 272084. > > The problem was that the lhs symbol itself was not being checked as a > proc_pointer - just the expression component. > > I will get on with backporting tomorrow. > > Cheers > > Paul > > 2019-06-08 Paul Thomas > > PR fortran/90786 > * trans-expr.c (pointer_assignment_is_proc_pointer) Remove as > it is very simple and only called from one place. > (gfc_trans_pointer_assignment): Rename non_proc_pointer_assign > as non_proc_ptr_assign. Assign to it directly, rather than call > to above, deleted function and use gfc_expr_attr instead of > only checking the reference chain. > > 2019-06-08 Paul Thomas > > PR fortran/90786 > * gfortran.dg/proc_ptr_51.f90 : New test. -- * Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html * Galacticus: https://bitbucket.org/galacticusdev/galacticus
Re: module_column not initialized in write_module()
Thanks Thomas, committed as r270777.
On Wednesday, May 1, 2019 9:35:13 PM PDT Thomas Koenig wrote:
> Hi Andrew,
>
> > Fixing this seems to be simple:
> >
> > --- gcc/fortran/module.c(revision 270772)
> > +++ gcc/fortran/module.c(working copy)
> > @@ -6052,6 +6052,9 @@ write_module (void)
> >
> > {
> >
> > int i;
> >
> > + /* Initialize the column counter. */
> > + module_column = 1;
> > +
> >
> > /* Write the operator interfaces. */
> > mio_lparen ();
> >
> > This causes no regressions. I've attached the patch including the
> > ChangeLog - is this ok to commit?
>
> OK for trunk.
>
> Thanks a lot for the patch!
>
> Regards
>
> Thomas
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html
* Galacticus: https://bitbucket.org/galacticusdev/galacticus
module_column not initialized in write_module()
The value of 'module_column' is not initialized in write_module() before a
module file is written. As a result, the first line break in the module file
can
occur at an arbitrary column, depending on what value 'module_column' happens
to have. While this doesn't affect the behavior of the module file as far as
gfortran is concerned, it does mean that the structure of the module file can
change, even though the actual content is unaltered.
For example, these first few lines of a (uncompressed) module file before and
after adding a single write statement to one of its functions:
GFORTRAN module version '15' created from
structure_formation.excursion_sets.first_crossing_distribution.p.F90
(() ()
() () () () () () () () () () () () () () () () () () () () () () () ()
())
()
and:
GFORTRAN module version '15' created from
structure_formation.excursion_sets.first_crossing_distribution.p.F90
(() () () () () () () () () () () () () () () () () () () () () () ()
() () () ())
()
When building large projects with gfortran I 'cmp' newly produced module files
with the equivalent module file from the previous build as a way to avoid
recompilation cascades. The changing position of the first line break makes
this approach not work.
Fixing this seems to be simple:
--- gcc/fortran/module.c(revision 270772)
+++ gcc/fortran/module.c(working copy)
@@ -6052,6 +6052,9 @@ write_module (void)
{
int i;
+ /* Initialize the column counter. */
+ module_column = 1;
+
/* Write the operator interfaces. */
mio_lparen ();
This causes no regressions. I've attached the patch including the ChangeLog -
is this ok to commit?
-AndrewIndex: gcc/fortran/ChangeLog
===
--- gcc/fortran/ChangeLog (revision 270772)
+++ gcc/fortran/ChangeLog (working copy)
@@ -1,3 +1,8 @@
+2019-05-01 Andrew Benson
+
+ * module.c (write_module): Initialize module_column before writing
+ module to ensure line break occurs at correct column.
+
2019-04-19 Steven G. Kargl
PR fortran/90166
Index: gcc/fortran/module.c
===
--- gcc/fortran/module.c (revision 270772)
+++ gcc/fortran/module.c (working copy)
@@ -6052,6 +6052,9 @@ write_module (void)
{
int i;
+ /* Initialize the column counter. */
+ module_column = 1;
+
/* Write the operator interfaces. */
mio_lparen ();
Re: [patch, fortan] PR87103 - [OOP] ICE in gfc_new_symbol() due to overlong symbol name
On Wednesday, September 5, 2018 12:35:04 PM PDT Bernhard Reutner-Fischer wrote: > On Wed, 5 Sep 2018 at 03:30, Jerry DeLisle wrote: > > On 09/04/2018 10:43 AM, Bernhard Reutner-Fischer wrote: > > > On Tue, 4 Sep 2018 at 18:43, Andrew Benson wrote: > > >> 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 > > 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. -- * Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html * Galacticus: https://bitbucket.org/abensonca/galacticus
[patch, fortan] PR87103 - [OOP] ICE in gfc_new_symbol() due to overlong symbol name
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 On Saturday, August 25, 2018 1:50:57 PM PDT Andrew Benson wrote: > 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 2018-09-04 Andrew Benson * gfortan.h: Increase 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. Index: gcc/fortran/gfortran.h ==
Re: Optimization in load_generic_interfaces()
Committed as r263784.
Thanks,
Andrew
On Wednesday, August 22, 2018 5:43:30 PM PDT Thomas Koenig wrote:
> Hi Andrew,
>
> [please also copy in gcc-patches for patches]
>
> > I'm continuing to look for optimizations to improve compile times for
> > files
> > which USE large numbers of modules containing large numbers of symbols.
> >
> > When the number of symbols becomes very large, find_symbol() becomes a
> > slow- point, because it can't use the structure of the balanced binary
> > tree to rapidly search the symtree, so just has to go through the whole
> > tree until it finds (or doesn't find) the symbol.
> >
> > I don't see a simple way to improve the speed of this function, but there
> > seems to be a simple change in load_generic_interfaces() which gives
> > significant speed up:
> >
> > Index: gcc/fortran/module.c
> > ===
> > --- gcc/fortran/module.c(revision 263667)
> > +++ gcc/fortran/module.c(working copy)
> > @@ -4559,9 +4559,6 @@ load_generic_interfaces (void)
> >
> >/* Decide if we need to load this one or not. */
> >p = find_use_name_n (name, , false);
> >
> > - st = find_symbol (gfc_current_ns->sym_root,
> > - name, module_name, 1);
> > -
> >
> >if (!p || gfc_find_symbol (p, NULL, 0, ))
> >
> > {
> >
> >/* Skip the specific names for these cases. */
> >
> > @@ -4570,6 +4567,9 @@ load_generic_interfaces (void)
> >
> >continue;
> >
> > }
> >
> > + st = find_symbol (gfc_current_ns->sym_root,
> > + name, module_name, 1);
> > +
> >
> >/* If the symbol exists already and is being USEd without being
> >
> > in an ONLY clause, do not load a new symtree(11.3.2). */
> >
> >if (!only_flag && st)
> >
> > This just delays the call to find_symbol() until after the first test of
> > whether the symbol needs to be loaded - if that test fails then
> > find_symbol() is never called.
> >
> > This has no significant effect on compile time for files which import
> > small
> > numbers of symbols. But for cases where the number is large I find that
> > the
> > compile time can be reduced by up to 40% in the cases I've tried.
> >
> > The change passes all regression tests cleanly.
>
> The patch is OK for trunk.
>
> Thanks!
>
> Regards
>
> Thomas
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html
* Galacticus: https://bitbucket.org/abensonca/galacticus
Re: Optimization in load_generic_interfaces()
Hi Thomas,
I have my sourceware account active now, so I should be able to commit this
myself. I'll do so right away.
Thanks,
Andrew
On Wednesday, August 22, 2018 5:43:30 PM PDT Thomas Koenig wrote:
> Hi Andrew,
>
> [please also copy in gcc-patches for patches]
>
> > I'm continuing to look for optimizations to improve compile times for
> > files
> > which USE large numbers of modules containing large numbers of symbols.
> >
> > When the number of symbols becomes very large, find_symbol() becomes a
> > slow- point, because it can't use the structure of the balanced binary
> > tree to rapidly search the symtree, so just has to go through the whole
> > tree until it finds (or doesn't find) the symbol.
> >
> > I don't see a simple way to improve the speed of this function, but there
> > seems to be a simple change in load_generic_interfaces() which gives
> > significant speed up:
> >
> > Index: gcc/fortran/module.c
> > ===
> > --- gcc/fortran/module.c(revision 263667)
> > +++ gcc/fortran/module.c(working copy)
> > @@ -4559,9 +4559,6 @@ load_generic_interfaces (void)
> >
> >/* Decide if we need to load this one or not. */
> >p = find_use_name_n (name, , false);
> >
> > - st = find_symbol (gfc_current_ns->sym_root,
> > - name, module_name, 1);
> > -
> >
> >if (!p || gfc_find_symbol (p, NULL, 0, ))
> >
> > {
> >
> >/* Skip the specific names for these cases. */
> >
> > @@ -4570,6 +4567,9 @@ load_generic_interfaces (void)
> >
> >continue;
> >
> > }
> >
> > + st = find_symbol (gfc_current_ns->sym_root,
> > + name, module_name, 1);
> > +
> >
> >/* If the symbol exists already and is being USEd without being
> >
> > in an ONLY clause, do not load a new symtree(11.3.2). */
> >
> >if (!only_flag && st)
> >
> > This just delays the call to find_symbol() until after the first test of
> > whether the symbol needs to be loaded - if that test fails then
> > find_symbol() is never called.
> >
> > This has no significant effect on compile time for files which import
> > small
> > numbers of symbols. But for cases where the number is large I find that
> > the
> > compile time can be reduced by up to 40% in the cases I've tried.
> >
> > The change passes all regression tests cleanly.
>
> The patch is OK for trunk.
>
> Thanks!
>
> Regards
>
> Thomas
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html
* Galacticus: https://bitbucket.org/abensonca/galacticus
Re: Optimization in load_generic_interfaces()
Hi Thomas,
Thanks for the review and approval (and for reminding me to CC gcc-patches).
I'm still working on getting my sourceware account for svn access set up, so I
can't commit the patch myself yet. If you (or anyone else) wants to go ahead
an commit it that would be good - otherwise I'll do so as soon as I have write
access.
Thanks,
Andrew
On Wednesday, August 22, 2018 5:43:30 PM PDT Thomas Koenig wrote:
> Hi Andrew,
>
> [please also copy in gcc-patches for patches]
>
> > I'm continuing to look for optimizations to improve compile times for
> > files
> > which USE large numbers of modules containing large numbers of symbols.
> >
> > When the number of symbols becomes very large, find_symbol() becomes a
> > slow- point, because it can't use the structure of the balanced binary
> > tree to rapidly search the symtree, so just has to go through the whole
> > tree until it finds (or doesn't find) the symbol.
> >
> > I don't see a simple way to improve the speed of this function, but there
> > seems to be a simple change in load_generic_interfaces() which gives
> > significant speed up:
> >
> > Index: gcc/fortran/module.c
> > ===
> > --- gcc/fortran/module.c(revision 263667)
> > +++ gcc/fortran/module.c(working copy)
> > @@ -4559,9 +4559,6 @@ load_generic_interfaces (void)
> >
> >/* Decide if we need to load this one or not. */
> >p = find_use_name_n (name, , false);
> >
> > - st = find_symbol (gfc_current_ns->sym_root,
> > - name, module_name, 1);
> > -
> >
> >if (!p || gfc_find_symbol (p, NULL, 0, ))
> >
> > {
> >
> >/* Skip the specific names for these cases. */
> >
> > @@ -4570,6 +4567,9 @@ load_generic_interfaces (void)
> >
> >continue;
> >
> > }
> >
> > + st = find_symbol (gfc_current_ns->sym_root,
> > + name, module_name, 1);
> > +
> >
> >/* If the symbol exists already and is being USEd without being
> >
> > in an ONLY clause, do not load a new symtree(11.3.2). */
> >
> >if (!only_flag && st)
> >
> > This just delays the call to find_symbol() until after the first test of
> > whether the symbol needs to be loaded - if that test fails then
> > find_symbol() is never called.
> >
> > This has no significant effect on compile time for files which import
> > small
> > numbers of symbols. But for cases where the number is large I find that
> > the
> > compile time can be reduced by up to 40% in the cases I've tried.
> >
> > The change passes all regression tests cleanly.
>
> The patch is OK for trunk.
>
> Thanks!
>
> Regards
>
> Thomas
--
* Andrew Benson: http://users.obs.carnegiescience.edu/abenson/contact.html
* Galacticus: https://bitbucket.org/abensonca/galacticus
Re: Slow compile - find_symtree_for_symbol()
No problem - I'm making use of the faster compile times already!
Cheers,
Andrew
On Monday, April 17, 2017 7:23:29 PM PDT Paul Richard Thomas wrote:
> Committed as revision 246952.
>
> Thanks for finding this deadwood.
>
> Cheers
>
> Paul
>
> On 16 April 2017 at 20:18, Janus Weil <ja...@gcc.gnu.org> wrote:
> > Hi Paul,
> >
> >> Prompted by this thread, I have just tried eliminating the call and
> >> the function altogether. No problem, the regtesting went through
> >> without a problem. Evidently the need for this function has gone away
> >> with some subsequent patch(es).
> >>
> >> OK for trunk?
> >
> > yes, ok from my side. Removing unneeded code is certainly a good idea,
> > even more so if it is responsible for a performance problem.
> >
> > I assume this should generate a similar (or even larger?) speedup for
> > Andrew's case as his earlier patch. Andrew, can you confirm this?
> >
> > Cheers,
> > Janus
> >
> >> 2017-04-16 Paul Thomas <pa...@gcc.gnu.org>
> >>
> >> PR fortran/80440
> >> * module.c (find_symtree_for_symbol): Delete.
> >> (read_module): Remove the call to the above.
> >>
> >> On 16 April 2017 at 14:27, Janus Weil <ja...@gcc.gnu.org> wrote:
> >>> Hi Paul,
> >>>
> >>>> The reason why the name wass not used is because of the rename
> >>>> facility in modules. In this case, the symtree name and the symbol
> >>>> names are different.
> >>>
> >>> thanks for the feedback!
> >>>
> >>> Do you understand why Andrew's patch (see
> >>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80440#c0) doesn't show
> >>> any testsuite failures then? Not even the test case from the original
> >>> r121824 fails (char_array_constructor_2.f90). Don't we have sufficient
> >>> coverage of use-renaming?
> >>>
> >>> Looking at the one occurrence of find_symtree_for_symbol in read_module:
> >>> /* If possible recycle the symtree that references the symbol.
> >>>
> >>> If a symtree is not found and the module does not import one,
> >>> a unique-name symtree is found by read_cleanup. */
> >>>
> >>> st = find_symtree_for_symbol (gfc_current_ns->sym_root, sym);
> >>> if (st != NULL)
> >>>
> >>> {
> >>>
> >>> info->u.rsym.symtree = st;
> >>> info->u.rsym.referenced = 1;
> >>>
> >>> }
> >>>
> >>> The comment there sounds a bit like this is just an (optional)
> >>> 'optimization'? If it actually slows down things in reality, could one
> >>> just get away without this piece of code altogether?
> >>>
> >>> Cheers,
> >>> Janus
> >>>
> >>>> On 15 April 2017 at 20:56, Andrew Benson <aben...@carnegiescience.edu>
wrote:
> >>>>> Hi Janus,
> >>>>>
> >>>>> On Saturday, April 15, 2017 1:48:36 PM PDT Janus Weil wrote:
> >>>>>> Hi Andrew,
> >>>>>>
> >>>>>> > Compile times for code that makes extensive USEs of modules seems
> >>>>>> > to be
> >>>>>> > very slow in some cases. I've been doing some investigation of the
> >>>>>> > cause
> >>>>>> > of this with the hope that I can maybe figure out some way to speed
> >>>>>> > things up.
> >>>>>> >
> >>>>>> > For example, I have a smallish file - 700 lines of code, which
> >>>>>> > takes
> >>>>>> > around 3 minutes to compile with a recent build of gfortran.
> >>>>>>
> >>>>>> whoa, 3 minutes definitely sounds pretty bad for 700 loc :(
> >>>>>
> >>>>> Yeah, definitely slow! For compiling a large project with many modules
> >>>>> it's
> >>>>> making development painful.
> >>>>>
> >>>>>> > Profiling f951 with
> >>>>>> > valgrind I find that 63% of that time is spent in
> >>>>>> > find_symtree_for_symbol(), which (if I understand correctly) is
> >>>>>> > searching
> >>>>>> > for a node in the symtree that
Re: [Patch, Fortran, F03] PR52909: Procedure pointers not private to modules
Hi Janus: I would surely appreciate some input from others, also from users (in particular from Andrew as the bug reporter). In general: Is ABI compatibility of different gfortran versions important to gfortran users? (For me personally, as a user, not so much. I usually don't link my own code with pre-compiled Fortran libraries). Right now, ABI compatibility of gfortran versions isn't very important to me, for the following reasons: * The main code I'm compiling with gfortran links against three libraries which all generate .mod files (HDF5, FGSL and FoX). I have to recompile those when the module format changes anyway, so recompiling if the ABI changes isn't a big problem. * I don't link to any libraries that I haven't compiled myself, so if the ABI changes I can just recompile them. (Although I should say that this isn't really by choice - I'm compiling them myself because they either weren't available on some of the HPC systems I'm using, or else the installed versions were too old.) * Since I'm using the development version of gfortran I'm expecting there to be changes, and I'm happy to see them if they are a result of new functionality being added of bugs being fixed. However, I think the above reasons are quite specific to me, and might not be true for the majority of gfortran users. Also, I certainly agree with Tobias that ABI (and .mod) compatibility are very important in general - if compatibility can be maintained without stifling progress on implementation of new features and bug fixing then I'm all for it. My knowledge of compiler development is far too limited to judge how difficult maintaining ABI/.mod compatibility is in any specific situation though! -Andrew -- * Andrew Benson: http://www.tapir.caltech.edu/~abenson/contact.html * Galacticus: http://sites.google.com/site/galacticusmodel
Re: [Patch, fortran] Fix PR fortran/50050 breakage: ICE on valid with null pointer initialization
Hi Mikael: this is an attempt to fix my recent breakage for PR50050. I forgot that shape can't always be known, and thus, that for some expressions, the shape field is a NULL pointer. This patch adds an early return in gfc_free_shape in the case shape is NULL. Then some external NULL shape checks are redundant and can be removed. I added some asserts in the cases there was no check before, so that the code is strictly equivalent. That patch works for me - both for the test case and for the full library that I was attempting to compile. Thanks, Andrew. -- * Andrew Benson: http://www.tapir.caltech.edu/~abenson/contact.html * Galacticus: http://sites.google.com/site/galacticusmodel
