[fortran] PR 91413 Generate warning when making array static

Message ID 20190810203415.9460-1-blomqvist.janne@gmail.com
State New
Headers show
Series
  • [fortran] PR 91413 Generate warning when making array static
Related show

Commit Message

Janne Blomqvist Aug. 10, 2019, 8:34 p.m.
When moving a local variable from the stack to static storage, the
procedure is no longer safe to be called recursively or concurrently
from multiple threads.  Thus generate a warning when this is done.
Also double the default limit for switching from stack to static.

Regtested on x86_64-pc-linux-gnu, Ok for trunk?

gcc/fortran/ChangeLog:

2019-08-10  Janne Blomqvist  <jb@gcc.gnu.org>

	PR fortran/91413
	* invoke.texi (-fmax-stack-var-size): Document increased default.
	* options.c (gfc_post_options): Increase default stack var size to
	65536 bytes.
	* trans-decl.c (gfc_finish_var_decl): Generate warning when local
	array moved to static storage.
---
 gcc/fortran/invoke.texi  |  2 +-
 gcc/fortran/options.c    |  2 +-
 gcc/fortran/trans-decl.c | 10 ++++++++++
 3 files changed, 12 insertions(+), 2 deletions(-)

-- 
2.17.1

Comments

Steve Kargl Aug. 10, 2019, 8:57 p.m. | #1
On Sat, Aug 10, 2019 at 11:34:15PM +0300, Janne Blomqvist wrote:
> When moving a local variable from the stack to static storage, the

> procedure is no longer safe to be called recursively or concurrently

> from multiple threads.  Thus generate a warning when this is done.

> Also double the default limit for switching from stack to static.

> 

> Regtested on x86_64-pc-linux-gnu, Ok for trunk?

> 


OK. 

-- 
Steve
Janne Blomqvist Aug. 11, 2019, 10:01 a.m. | #2
On Sat, Aug 10, 2019 at 11:57 PM Steve Kargl
<sgk@troutmask.apl.washington.edu> wrote:
>

> On Sat, Aug 10, 2019 at 11:34:15PM +0300, Janne Blomqvist wrote:

> > When moving a local variable from the stack to static storage, the

> > procedure is no longer safe to be called recursively or concurrently

> > from multiple threads.  Thus generate a warning when this is done.

> > Also double the default limit for switching from stack to static.

> >

> > Regtested on x86_64-pc-linux-gnu, Ok for trunk?

> >

>

> OK.


Thanks for the quick review, committed as r274264.

To be clear, this patch does not fix the PR, just makes it slightly
less likely to happen, and makes some noise when it does happen.

To reignite the discussion from
https://gcc.gnu.org/ml/fortran/2017-12/msg00082.html , any opinions
how this should properly be fixed? I can see two main options:

- Use the heap instead of static storage for local variables going
over the size limit set by -fmax-stack-var-size= .   This is IMHO a
bit ugly, going behind the back of the user like this; If the user
wants to use the heap, she can use allocatable arrays. Also, it needs
to be done somewhere else than in trans-decl.c (gfc_finish_var_decl)
as at that point there is no access to the block, and hence one cannot
add new code to malloc() and free() the variable. I'm not sure where
the proper place for this would be.

- Make -frecursive the default for GFC_STD_F2018 (and thus also for
-std=gnu), while keeping the existing behavior for older standards.
This would follow the wishes of the user, but would risk crashing
applications with stack exhaustion.

Any opinions which would be preferable, or any other solution to this problem?

-- 
Janne Blomqvist

Patch

diff --git a/gcc/fortran/invoke.texi b/gcc/fortran/invoke.texi
index 3c1b2ac7a26..1039c6084c4 100644
--- a/gcc/fortran/invoke.texi
+++ b/gcc/fortran/invoke.texi
@@ -1720,7 +1720,7 @@  This option currently only affects local arrays declared with constant
 bounds, and may not apply to all character variables.
 Future versions of GNU Fortran may improve this behavior.
 
-The default value for @var{n} is 32768.
+The default value for @var{n} is 65536.
 
 @item -fstack-arrays
 @opindex @code{fstack-arrays}
diff --git a/gcc/fortran/options.c b/gcc/fortran/options.c
index ef37cccec97..146be2f1420 100644
--- a/gcc/fortran/options.c
+++ b/gcc/fortran/options.c
@@ -437,7 +437,7 @@  gfc_post_options (const char **pfilename)
 
   /* Set default.  */
   if (flag_max_stack_var_size == -2)
-    flag_max_stack_var_size = 32768;
+    flag_max_stack_var_size = 65536;
 
   /* Implement -fno-automatic as -fmax-stack-var-size=0.  */
   if (!flag_automatic)
diff --git a/gcc/fortran/trans-decl.c b/gcc/fortran/trans-decl.c
index 86c3d3a9796..2a9b852568a 100644
--- a/gcc/fortran/trans-decl.c
+++ b/gcc/fortran/trans-decl.c
@@ -753,6 +753,16 @@  gfc_finish_var_decl (tree decl, gfc_symbol * sym)
 	  || sym->attr.allocatable)
       && !DECL_ARTIFICIAL (decl))
     {
+      gfc_warning (OPT_Wsurprising,
+		   "Array %qs at %L is larger than limit set by"
+		   " %<-fmax-stack-var-size=%>, moved from stack to static"
+		   " storage. This makes the procedure unsafe when called"
+		   " recursively, or concurrently from multiple threads."
+		   " Consider using %<-frecursive%>, or increase the"
+		   " %<-fmax-stack-var-size=%> limit, or change the code to"
+		   " use an ALLOCATABLE array.",
+		   sym->name, &sym->declared_at);
+
       TREE_STATIC (decl) = 1;
 
       /* Because the size of this variable isn't known until now, we may have