Page MenuHomePhabricator

[flang] Added Semantic Checks for 2 Data constraints and fixed the semantic errors in 3 test cases
ClosedPublic

Authored by anchu-rajendran on Apr 18 2020, 6:58 AM.

Details

Summary
  • C876, C877 checks are implemented.
  • Modified pre-fir-tree02.f90, block-data01.f90 and data01.f90 as they were not conforming to C876
  • Fixed IsConstantExpr to check C879

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
  • Abstracted the code to IsAutomaticArray Function.
  • Added implementation to catch semantic bugs related to fields of derived type variables.
  • Made the changes suggested.
anchu-rajendran marked 14 inline comments as done.Apr 20 2020, 2:13 AM
anchu-rajendran added inline comments.
flang/lib/Semantics/check-data.cpp
41

Thank you for your review comments! I have made the suggested modifications.

41

The code has been changed to check all symbols so the related semantic errors with derived types can be caught.

flang/test/Semantics/data04.f90
2

Thank you for your suggestions! I have added extra code for handling derived types and tests with derived types for checking constraints related to automatic arrays, use association, allocatable variables and common blocks.

anchu-rajendran marked 2 inline comments as done.Apr 20 2020, 2:15 AM

Thanks for addressing the comments.

Also check the following.

type t1
 integer, allocatable :: arr(:)
end type
type(t1) d1
data d1/t1((/0,1/))/

Besides the constraints for the data statement, there are several other points mentioned in the data statement section. Like point 8 in 8.6.7 (test below) which says that the number of variables and data-statement-constants shall be the same. Are these outside the scope of your work? If so, please create a ticket somewhere so that we can come back to it later.

program mn
  character(len=3) :: n1, n2
  data n1, n2 / 'f18' /
end
flang/lib/Semantics/check-data.cpp
36

I wasn't sure whether using the type from the module makes the variable d1 a use associated variable. What is your conclusion?

tskeith accepted this revision.Apr 21 2020, 7:23 AM
tskeith added inline comments.
flang/include/flang/Semantics/tools.h
159

I don't think there is any reason for this to be inline.

This revision is now accepted and ready to land.Apr 21 2020, 7:23 AM
anchu-rajendran updated this revision to Diff 259225.EditedApr 22 2020, 3:55 AM
anchu-rajendran edited the summary of this revision. (Show Details)

Fixed a few corner cases:
Implemented a function: GetFirstName

module dt
  type d
    real r
  end type
end module
program test
  use dt
  type(d) :: d1
  data d1%r/2.5/
end program

In the above example, d1%r should not be identified as use-associated since, d1 is declared in the same scope. Previous revision identifies d1%r as use associated since the semantic checks were performed on last name r and not on first name:d1. The current revision applies checks on first name.

A few other corner cases will also be caught with this modification. An example follows:

 module m2
    type newType
      integer number
    end type
    contains
    subroutine checkDerivedType(m2_number)
      type(newType) m2_number
      DATA m2_number%number /1/
   end
end

In the above example, m2_number%number should not be used as a data obect as it is a dummy argument. This constraint will only be checked if check is applied on first name.

anchu-rajendran marked 2 inline comments as done.Apr 22 2020, 4:00 AM
anchu-rajendran added inline comments.
flang/lib/Semantics/check-data.cpp
36

Hi,
Thank you for pointing this out! That was a corner case which was not checked properly. I have modified the implementation and included tests for the same.

anchu-rajendran marked an inline comment as done.Apr 22 2020, 4:04 AM

Thanks for addressing the comments.

Also check the following.

type t1
 integer, allocatable :: arr(:)
end type
type(t1) d1
data d1/t1((/0,1/))/

Besides the constraints for the data statement, there are several other points mentioned in the data statement section. Like point 8 in 8.6.7 (test below) which says that the number of variables and data-statement-constants shall be the same. Are these outside the scope of your work? If so, please create a ticket somewhere so that we can come back to it later.

program mn
  character(len=3) :: n1, n2
  data n1, n2 / 'f18' /
end

I see that there are a few more constraints listed as you mentioned which requires expression evaluations and type checking. I will create a ticket for them now.

Removed inline specifier from IsAutomaticArray

anchu-rajendran marked an inline comment as done.Apr 22 2020, 4:27 AM

Thanks for addressing the comments.

Also check the following.

type t1
 integer, allocatable :: arr(:)
end type
type(t1) d1
data d1/t1((/0,1/))/

The current revision does not catch this error. I dont think this case is covered under the Constraint C876 (or C877)
C876 says a variable whose designator appears as a data-stmt-object shall not be an allocatable variable . This means d1 cannot be an allocatable (and it does not say anything about the fields of d1). In this case, d1 is not.

Thanks for addressing the comments.

Also check the following.

type t1
 integer, allocatable :: arr(:)
end type
type(t1) d1
data d1/t1((/0,1/))/

The current revision does not catch this error. I dont think this case is covered under the Constraint C876 (or C877)
C876 says a variable whose designator appears as a data-stmt-object shall not be an allocatable variable . This means d1 cannot be an allocatable (and it does not say anything about the fields of d1). In this case, d1 is not.

The code is in error because the structure constructor is not a constant expression in the sense of 10.1.12. I have a fix for this in my rework of DATA statement semantics in progress.

Thanks for addressing the comments.

Also check the following.

type t1
 integer, allocatable :: arr(:)
end type
type(t1) d1
data d1/t1((/0,1/))/

The current revision does not catch this error. I dont think this case is covered under the Constraint C876 (or C877)
C876 says a variable whose designator appears as a data-stmt-object shall not be an allocatable variable . This means d1 cannot be an allocatable (and it does not say anything about the fields of d1). In this case, d1 is not.

The code is in error because the structure constructor is not a constant expression in the sense of 10.1.12. I have a fix for this in my rework of DATA statement semantics in progress.

@klausler, Okay, Thank you for pointing that out! I have fixed the bug with IsConstantExpr in my local branch which was causing C879 not to be checked properly which I shall submit for review after merging this patch. @kiranchandramohan , I hope all the concerns are addressed so I can merge this.

If all the concerns are addressed, you can merge. Thanks for working on this.

tskeith added inline comments.Apr 24 2020, 7:53 AM
flang/lib/Semantics/check-data.cpp
19

symbol might be null so you need to check it here.

If all the concerns are addressed, you can merge. Thanks for working on this.

Earlier, Anchu said that she would change this code to use typed expressions. Has this been done? It doesn't look like it. It took me most of the week to rework her earlier patch to use the right representation. None of these analyses should be done on the parse tree, and the new functions in parser/tools.h aren't necessary.

klausler requested changes to this revision.Apr 24 2020, 3:12 PM

https://reviews.llvm.org/D78834 contains the patch that reworks check-data.{h,cpp} to check typed expressions rather than the raw parse tree. Please adjust this change to do the same, if possible.

This revision now requires changes to proceed.Apr 24 2020, 3:12 PM
klausler added inline comments.Apr 24 2020, 4:39 PM
flang/lib/Semantics/check-data.cpp
50

Every serious production Fortran compiler allows DATA initialization of COMMON in any subprogram;. Ignore the standard here, or change the error into a warning message.

anchu-rajendran added a comment.EditedApr 25 2020, 2:31 AM

https://reviews.llvm.org/D78834 contains the patch that reworks check-data.{h,cpp} to check typed expressions rather than the raw parse tree. Please adjust this change to do the same, if possible.

Thank You! I went through your patch and now, I get your point on why we should implement semantic checks on typed expressions. I have tried to implement C876 and C877 on top of this patch and I could perform those checks on typed expressions. I will submit those for review after you merge your patch.

The change to convert code to have checks on typed expressions was asked by you on my previous patch (https://reviews.llvm.org/D78008). I asked for a merge as I was not sure about the changes you were expecting and I thought the code in this patch would be least affected. Sorry for my misunderstanding and thanks for the changes!

anchu-rajendran edited the summary of this revision. (Show Details)
anchu-rajendran marked an inline comment as done.Apr 26 2020, 5:16 AM

https://reviews.llvm.org/D78834 contains the patch that reworks check-data.{h,cpp} to check typed expressions rather than the raw parse tree. Please adjust this change to do the same, if possible.

I have tried to rework the implementation as suggested. Please let me know if improvements are required.

klausler requested changes to this revision.May 4 2020, 9:22 AM
klausler added inline comments.
flang/lib/Evaluate/check-expression.cpp
63

The logic here is wrong. Just recurse on the base (return (*this)(component.base());). If the base is an array element reference with non-constant subscripts, the component reference isn't a constant.

flang/lib/Semantics/check-data.cpp
61

This code doesn't implement the C877 constraint correctly. A pointer is allowed in a DATA statement object only if it is the entire rightmost part-ref. Your code would accept subscripts or a substring reference that was applied to a pointer component (A%PTR(1)(2:3)).

150

This member function doesn't use any state from the class; it could be made static, but it would be even better if it were in lib/Semantics/tools.cpp as it will probably be useful in other cases.

157

Please follow the naming conventions.

flang/lib/Semantics/tools.cpp
584

This code would return true for an explicit-shape dummy argument array with non-constant bounds, yes? Such objects are not "automatic". I think that you need a better name for this predicate.

This revision now requires changes to proceed.May 4 2020, 9:22 AM
anchu-rajendran marked 7 inline comments as done.
  • Addressed the review comments
  • Modified IsAutomaticArray to check all automatic objects as described in 3.11
  • Added extra checks to emit error if right most pointer reference in a component is subscripted (C877)

@klausler , Thank you for the review comments! I have made the suggested changes.

flang/lib/Semantics/tools.cpp
584

Thank You! I have changed the implementation according to the description of automatic data objects given in 3.11

Made changes to update the variable symbolCheckStatus correctly.

Ping @klausler . Can you please take another look and let me know if any concerns need to be addressed. I have tried to address your comments.

klausler requested changes to this revision.May 14 2020, 8:55 AM
klausler added inline comments.
flang/lib/Semantics/check-data.cpp
62

These state variables are still too confusing and they're going to lead to bugs later when people modify this code.

I think it would be more clear if there were a single state variable that allowed a pointer to appear, and it would be initialized to true if and only if the entire designator were a simple Symbol or a Component. The state variable would be reset to false during the analysis of a Component.

94

This looks like you don't apply checks for dummy arguments, functions, blank COMMON, and so on if the designator is a component. If so, that's wrong. DATA DUMMYARG%X / 1.0 /.

flang/lib/Semantics/tools.cpp
585

What about pointers and allocatables and functions and statement functions and construct entities and probably other types of symbol? I think your function can return true for them, and that seems wrong if the symbol isn't a local object.

flang/test/Semantics/data04.f90
43

Please extend these tests to include cases where the dummy argument (or whatever) is not the whole designator; add subscripts, substring references, components, &c.

This revision now requires changes to proceed.May 14 2020, 8:55 AM
anchu-rajendran marked an inline comment as done.May 14 2020, 10:01 AM
anchu-rajendran added inline comments.
flang/lib/Semantics/check-data.cpp
94

@klausler ,This is implemented in line 76 in the same file . I see that, (for the examples I could think of,) when a component is used, only the first symbol needs to be checked for checks in CheckFirstSymbol. Line 116 in data04.f90 covers this case and emits proper error message as the checks are done on component. similarly lines 88, 119, 140 in the same file covers other cases
One thing that is lacking here is none of these tests are done on subscripts. However, I could not create a test case were subscript is a constant (enforced by C875) and would violate one of the other constraints. (eg, C874 is not checked on subscripts). Can I know your opinion on this?

anchu-rajendran updated this revision to Diff 264197.EditedMay 15 2020, 3:20 AM
anchu-rajendran marked an inline comment as done.

Reduced the use of state variables while handling component
Added code to check constraints on subscripts and restricted pointers inside subscript.

tskeith added inline comments.May 15 2020, 7:24 AM
flang/lib/Semantics/check-data.cpp
73

Do you need to continue after an error is reported or can you just return false? The latter would simplify the code.

99

Why do you need a new DataVarChecker here rather than continuing to use the existing one?

anchu-rajendran marked 6 inline comments as done.

Removed the err variable while handling Component

anchu-rajendran marked 2 inline comments as done.May 17 2020, 10:09 PM
anchu-rajendran added inline comments.
flang/lib/Semantics/check-data.cpp
73

I have returned false than reporting the errors. Thank you!

99

Subscripts can have any kind of DataRef. If a subscript is a%b%c(i), i think it is better to have tests on this DataRef separately so functions like CheckFirstSymbol will be called on a and say if a is a dummy argument, it reports an error.

For eg., consider a%b(c%d) is the data object being analyzed, where b is an array. In this case, when execution reaches checks on subscript, this.isFirstSymbolChecked_ will be true and this.hasSubscript_ will be true. For any checks that need to be done on subscript , I think it is better to set these variables for each subscript separately as it can mark the beginning of a DataRef. However I cannot find an use case where this checker will be used to report more errors (as the subscript needs to be a constant). I shall remove this part of code if it not expected to catch more semantic errors.

flang/lib/Semantics/tools.cpp
585

If it is a dummy, the function returns false. The only cases it returns true are (i) if length of a character is given by a non-constant expression (ii) the arrays with atleast one of the bounds is a non-constant expression. Else, the conservative value returned is false. Standard defines automatic object as
"nondummy data object with a type parameter or array bound that depends on the value of a specification-expr that is not a constant expression "
As kind parameter is always a constant expression, i did checks on only the length parameter.

For the cases you pointed out, I did check for pointers & allocatables and I saw it returns false. @klausler, Can you hint me an example code that would make this not behave as expected?

flang/test/Semantics/data04.f90
43

lines 88, 116, 119, 140 in the same file cover the cases

anchu-rajendran updated this revision to Diff 264539.EditedMay 17 2020, 11:11 PM
anchu-rajendran marked 2 inline comments as done and an inline comment as not done.

Updated IsAutomaticObject to return false if the symbol is an allocatable or a pointer and test cases are added to data04.f90

anchu-rajendran marked an inline comment as done.

Added a test with derived type for C876 checking the object should not be in blank COMMON

anchu-rajendran marked 4 inline comments as done.May 17 2020, 11:51 PM
anchu-rajendran added inline comments.
flang/lib/Semantics/check-data.cpp
94

The line numbers are changed in recent diff to 119, 122, 148, 154 in data04.f90 for CheckFirstSymbol and 85, 88, 91 for CheckAnySymbol

flang/lib/Semantics/tools.cpp
585

Pointers and allocatables are checked and examples are added to data04.f90 at line numbers: 42, 45

flang/test/Semantics/data04.f90
43

The line numbers are changed in recent diff to 119, 122, 148, 154 for CheckFirstSymbol and 85, 88, 91 for CheckAnySymbol

ping @klausler. If there are further improvements to be made, can you please suggest those. I have raised a few concerns related to questions which you have posted before.

I'm nearly done with the implementation of the rest of semantic processing for DATA statements (iterating through the objects and values, putting them into correspondence, and constructing a single initializer for each object).

You might as well commit your changes, and I'll clean up any remaining problems here when I merge your changes with mine.

klausler accepted this revision.Jun 2 2020, 9:25 AM
This revision is now accepted and ready to land.Jun 2 2020, 9:25 AM
This revision was automatically updated to reflect the committed changes.