SAVE statement should not apply to nested scoping units

[rikigigi]

Summary

SAVE statement, according to 8.6.14, must apply to the same scoping unit, that excludes nested scoping units. For example, if the SAVE statement is found in a MODULE, the functions contained in that module should not inherit the SAVE attribute. I think that the code was doing this, failing the following source:

MODULE pippo
SAVE

CONTAINS
PURE FUNCTION fft_stick_index( )
   IMPLICIT NONE
   INTEGER :: fft_stick_index
   INTEGER :: mc  !error: A pure subprogram may not have a variable with the SAVE attribute
END FUNCTION

END MODULE

[tskeith]

flang/lib/Evaluate/tools.cpp

1008

It's not clear to me why the call to hasSAVE is necessary at all. In resolve-names.cpp we set the SAVE attribute on the appropriate entities when a SAVE statement is encountered. So the symbol should have the SAVE attribute set here if SAVE is set in the scope.

I think you can remove the call to hasSAVE completely. And since it's the only call, remove the function and related stuff.

@klausler, do you know of any reason we need hasSAVE?

[klausler]

flang/lib/Evaluate/tools.cpp

1008

I recall that a "bare" SAVE statement in a scope wasn't causing name resolution to set the attribute on all items in the scope because it wasn't clear whether each entity was an "allowed item" in the sense of 8.6.14 and C859 -- a bare SAVE can appear before the other declarations that make it clear whether an entity is a common block, variable, or procedure pointer, and so the decision about SAVE had to be deferred.

[rikigigi]

flang/lib/Evaluate/tools.cpp

1008

So do you think that I should remove completely the hasSAVE call or it is better to keep it?

[tskeith]

flang/lib/Evaluate/tools.cpp

1008

We seem to have two methods of handling a bare SAVE statement, in resolve-names.cpp and here. That should be cleaned up so there is only one. It would be nice to have a test that shows where the resolve-names.cpp mechanism doesn't work.

But none of that has to happen to get this bug fixed, so I think it's fine to leave it as it is.

[rikigigi]

Only a question: I don't have push access to the git repo (and it is my first patch), are you going to push it for me? Thanks

[tskeith]

In D88279#2296334, @rikigigi wrote:

Only a question: I don't have push access to the git repo (and it is my first patch), are you going to push it for me? Thanks

Sure, can you send me your email address for the commit?

[rikigigi]

In D88279#2296578, @tskeith wrote:

In D88279#2296334, @rikigigi wrote:

Only a question: I don't have push access to the git repo (and it is my first patch), are you going to push it for me? Thanks

Sure, can you send me your email address for the commit?

rbertoss at sissa dot it

thanks

flang/lib/Evaluate/tools.cpp

       return true;
     } else if (IsDummy(symbol) || IsFunctionResult(symbol)) {
       return false;
-    } else {
-      for (; !scope->IsGlobal(); scope = &scope->parent()) {
-        if (scope->hasSAVE()) {
-          return true;
-        }
-      }
+    } else if (scope->hasSAVE() ) {
+      return true;
     }
   }
   return false;