Semantic checks to see if a DO variable is modified #860

psteinfeld · 2019-12-07T04:09:35Z

This is driven by Section 11.1.7.4.3, paragraph 2, which states:
Except for the incrementation of the DO variable that occurs in step (3), the
DO variable shall neither be redefined nor become undefined while the DO
construct is active.

Sections 19.6.5 and 19.6.6 describe the events that can cause variables to
become defined and undefined. Section 19.6.7 describes the contexts that imply
the redefinition or undefinition of a variable. For all of these situations
that are reasonably easy to discover at compile time, we should emit messages.

I added infrastructure to SemanticsContext to track active DO variables and
the source locations where they appear in DO statements. I also added code
to tools.[h,cc] to check to see if a DO variable is already defined, and, if
so, to emit an error message along with a reference to the relevant DO
construct. I also added calls to several places where variables are defined
to determine if the definitions are happening in the context of an active DO
construct.

tskeith · 2019-12-09T04:28:07Z

lib/semantics/semantics.cc

@@ -208,6 +208,28 @@ void SemanticsContext::PopConstruct() {
  constructStack_.pop_back();
 }

+void SemanticsContext::ActivateDoVariable(
+    const Symbol *doVariable, const parser::CharBlock location) {


These doVariable arguments should be const Symbol & rather than const Symbol *. I don't think they are ever null.

Thanks, Tim. I'll make that change.

tskeith · 2019-12-09T04:29:16Z

lib/semantics/semantics.h

@@ -172,6 +177,7 @@ class SemanticsContext {

  bool CheckError(bool);
  ConstructStack constructStack_;
+  std::map<const Symbol *, const parser::CharBlock> activeDoVariables_;


If the do variable symbols are changed to refs, the key for this map should then be SymbolRef.

Sounds good.

tskeith · 2019-12-09T04:31:14Z

lib/semantics/tools.h

+void CheckDoVarRedefine(
+    const Symbol &, SemanticsContext &, const parser::CharBlock &);
+void CheckDoVarRedefine(const parser::Variable &, SemanticsContext &);
+void CheckDoVarRedefine(const parser::Name &, SemanticsContext &);


It might make more sense to make these be member functions of SemanticContext as they depend on the active do variable there. It seems like that would make the new API simpler: just Activate/DeactivateDoVariable and CheckDoVarRedefine.

Good suggestion. I'll do that.

jeanPerier

I think you did a great job chasing the places where do variable may get redefined ! I wonder if one could leverage this work for other variables that may not be defined/re-defined (e.g. intent(in) argument (C844) and team-value in CHANGE TEAM (11.1.5.2 par 1)). Maybe the following would allow doing so:

Moving the map activeDoVariables_ to the BaseChecker and named it something like cannotBeDefinedVariables.
Providing a way to register/de-register symbols to this map (like ActivateDoVariable, but attached to the BaseChecker instead of the SemanticsContext).
The SemanticsVisitor could then be in charge of checking for all the spots that may define a variable that it is not defining a variable that cannot be defined. Alternatively, checkers for the statements that may define variables could be in charge of doing this check like you did.
Checkers for constructs like DO or CHANGE TEAM could then register and de-register variables that cannot be defined.
Anyway, this is an open proposal more than a request. It may just not be worth it given the other use cases (and one could make such change when the need will arise).

jeanPerier · 2019-12-09T15:43:56Z

lib/semantics/check-deallocate.cc

@@ -35,6 +35,8 @@ void DeallocateChecker::Leave(const parser::DeallocateStmt &deallocateStmt) {
              } else if (!IsAllocatableOrPointer(*symbol)) {  // C932
                context_.Say(name.source,
                    "name in DEALLOCATE statement must have the ALLOCATABLE or POINTER attribute"_err_en_US);
+              } else {
+                CheckDoVarRedefine(name, context_);


You may also want to check that it is not being allocated to prevent:

integer, allocatable :: i allocate(i) do i=1,10 print *, i if (i.eq.5) then allocate(i, source=4) ! hidden i-- end if end do

Thanks, Jean.

With respect to other similar instances of variables/names not being definable, I'll poke around and see what I can find in the standard and make a proposal for review. I may want to deal with generalizing the definability analysis to a future pull request.

With respect to not including allocation, my original analysis was based on sections 19.6.5 and 19.6.6, which specify the events that case variables to become defined or undefined. I neglected to include section 19.6.7, which specifies that contexts which imply definition or undefinition of variables. In addition to allocation, I also neglected to deal with actual arguments passed as INTENT(OUT) or INTENT(INOUT). I'll include tests and implementation for the cases described in section 19.6.7.

jeanPerier · 2019-12-09T16:33:13Z

lib/semantics/check-do.h

 struct ExitStmt;
+struct InquireSpec;
+struct IoControlSpec;
+struct IoControlSpec;


IoControlSpec seems duplicated.

D'oh! Good catch. I'll fix that.

tskeith · 2019-12-19T23:37:29Z

lib/semantics/check-do.cc

+      context_.ActivateDoVariable(GetDoVariable(doConstruct));
+    } else if (doConstruct.IsDoConcurrent()) {
+      auto &loopControl{doConstruct.GetLoopControl()};
+      if (loopControl) {


You could write these two lines as:
if (const auto &loopControl{doConstruct.GetLoopControl()}) {

You should use const auto & rather than just auto &.

Thanks, Tim. I'll change that.

tskeith · 2019-12-19T23:37:54Z

lib/semantics/check-do.cc

+      context_.DeactivateDoVariable(GetDoVariable(doConstruct));
+    } else if (doConstruct.IsDoConcurrent()) {
+      auto &loopControl{doConstruct.GetLoopControl()};
+      if (loopControl) {


Same as previous comment.

tskeith · 2019-12-19T23:44:01Z

lib/semantics/semantics.cc

@@ -208,6 +209,70 @@ void SemanticsContext::PopConstruct() {
  constructStack_.pop_back();
 }

+void SemanticsContext::SayDoVarRedefine(
+    const Symbol &variable, const parser::CharBlock &location) {


Other "Say" functions that take a CharBlock for the location of the message have that as the first argument, so it would be consistent.

tskeith · 2019-12-19T23:45:11Z

lib/semantics/semantics.cc

+void SemanticsContext::SayDoVarRedefine(
+    const Symbol &variable, const parser::CharBlock &location) {
+  const parser::CharBlock doLoc{GetDoVariableLocation(variable)};
+  Say(location, "Cannot redefine DO variable %s"_err_en_US, variable.name())


Names from the program that are mentioned in error messages are enclosed in single quotes. It makes messages easier to read.

Thanks. I'll add the quotes.

tskeith · 2019-12-20T00:02:27Z

lib/semantics/semantics.cc

+
+parser::CharBlock SemanticsContext::GetDoVariableLocation(
+    const Symbol &variable) {
+  if (IsActiveDoVariable(variable)) {


When would this be called on something that isn't an active do variable? If this actually did return an empty CharBlock to SayDoVarRedefine we wouldn't get a good error message location.

GetDoVariableLocation() gets called in two situations. One is to get the location for error messages. The other is to deactivate a DO variable at the end of a DO loop. In the case of nested DO loops that use the same variable, we only want to deactivate the loop variable for the outer loop when we exit it, not when we exit the inner loop. So DeactivateDoVariable() calls GetDoVariableLocation() to make sure that its deactivating the correct variable.

I don't think GetDoVariableLocation() will be called for an error message when there's not an empty location for the DO variable. But I can certainly add a check that will verify that.

jeanPerier · 2019-12-20T15:46:34Z

lib/semantics/check-allocate.cc

@@ -620,6 +620,7 @@ bool AllocationCheckerHelper::RunCoarrayRelatedChecks(
        "Allocatable object must not be coindexed in ALLOCATE"_err_en_US);
    return false;
  }
+  context.CheckDoVarRedefine(name_);


A minor point: this check is not related to coarrays yet is is done in RunCoarrayRelatedChecks. To avoid misleading future maintainers, I would move this check just before the following line instead:

f18/lib/semantics/check-allocate.cc

Line 542 in 867dd04

return RunCoarrayRelatedChecks(context);

Thanks, Jean. That's a better place. I'll move it.

I added infrastructure to SemanticsContext to track active DO variables and the source locations where they appear in DO statements. I also added code to semantics.[h,cc] to check to see if a DO variable is already defined, and, if so, to emit an error message along with a reference to the relevant DO construct. I also added calls to several places where variables are defined to determine if the definitions are happening in the context of an active DO construct. I have not yet added the checks for DO variables being redefined when passing them as actual arguments to dummy arguments with INTENT(OUT) or INTENT(INOUT). I wanted to get these changes merged first and catch up with the other changes in master.

I added infrastructure to SemanticsContext to track active DO variables and the source locations where they appear in DO statements. I also added code to semantics.[h,cc] to check to see if a DO variable is already defined, and, if so, to emit an error message along with a reference to the relevant DO construct. I also added calls to several places where variables are defined to determine if the definitions are happening in the context of an active DO construct. I have not yet added the checks for DO variables being redefined when passing them as actual arguments to dummy arguments with INTENT(OUT) or INTENT(INOUT). I wanted to get these changes merged first and catch up with the other changes in master. Original-commit: flang-compiler/f18@1bbfcca Reviewed-on: flang-compiler/f18#860

…/ps-do-redefine Semantic checks to see if a DO variable is modified Original-commit: flang-compiler/f18@6715b16 Reviewed-on: flang-compiler/f18#860

I added infrastructure to SemanticsContext to track active DO variables and the source locations where they appear in DO statements. I also added code to semantics.[h,cc] to check to see if a DO variable is already defined, and, if so, to emit an error message along with a reference to the relevant DO construct. I also added calls to several places where variables are defined to determine if the definitions are happening in the context of an active DO construct. I have not yet added the checks for DO variables being redefined when passing them as actual arguments to dummy arguments with INTENT(OUT) or INTENT(INOUT). I wanted to get these changes merged first and catch up with the other changes in master. Original-commit: flang-compiler/f18@1bbfcca Reviewed-on: flang-compiler/f18#860

psteinfeld requested review from jeanPerier, klausler, tskeith and vjayathirtha-nv December 7, 2019 04:09

tskeith reviewed Dec 9, 2019

View reviewed changes

jeanPerier reviewed Dec 9, 2019

View reviewed changes

psteinfeld requested a review from tskeith December 19, 2019 19:51

tskeith reviewed Dec 19, 2019

View reviewed changes

tskeith reviewed Dec 20, 2019

View reviewed changes

jeanPerier reviewed Dec 20, 2019

View reviewed changes

tskeith approved these changes Dec 21, 2019

View reviewed changes

psteinfeld force-pushed the ps-do-redefine branch from 867dd04 to 1bbfcca Compare December 24, 2019 23:17

psteinfeld merged commit 6715b16 into master Dec 24, 2019

psteinfeld deleted the ps-do-redefine branch December 24, 2019 23:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Semantic checks to see if a DO variable is modified #860

Semantic checks to see if a DO variable is modified #860

psteinfeld commented Dec 7, 2019 •

edited

tskeith Dec 9, 2019

psteinfeld Dec 9, 2019

tskeith Dec 9, 2019

psteinfeld Dec 9, 2019

tskeith Dec 9, 2019

psteinfeld Dec 9, 2019

jeanPerier left a comment

jeanPerier Dec 9, 2019

psteinfeld Dec 9, 2019

jeanPerier Dec 9, 2019

psteinfeld Dec 9, 2019

tskeith Dec 19, 2019

psteinfeld Dec 20, 2019

tskeith Dec 19, 2019

psteinfeld Dec 20, 2019

tskeith Dec 19, 2019

tskeith Dec 19, 2019

psteinfeld Dec 20, 2019

tskeith Dec 20, 2019

psteinfeld Dec 20, 2019

jeanPerier Dec 20, 2019

psteinfeld Dec 20, 2019

Semantic checks to see if a DO variable is modified #860

Semantic checks to see if a DO variable is modified #860

Conversation

psteinfeld commented Dec 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanPerier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psteinfeld commented Dec 7, 2019 •

edited