[flang] Fix internal error with DATA-statement style initializers

[unterumarmung]

Summary

The code below causes flang to crash with an exception.
After fixing the crash flang with an internal error "no symbol found for 'bar'"
This change fixes all the issues.

program name
  implicit none
  integer, parameter :: bar = 1
  integer foo(bar) /bar*2/
end program name

[unterumarmung]

Note to reviewers: this change doesn't fix the error itself, only the crash.
Currently (with this change applied), flang invocation with the given code gives the following error:

error: Semantic errors in a.f90
./a.f90:4:28: error: Internal: no symbol found for 'm_count'
    integer :: foo(m_count) /m_count*2/
                             ^^^^^^^

So, any pointers that could help solve this problem are appreciated!

[kiranchandramohan]

For parser, semantics, runtime patches please add @klausler as the primary reviewer.

As far as this patch is concerned,
-> Isn't the ideal behaviour a parse/semantic error? Other compilers give the following error messages:
gfortran:

   5 | integer :: foo(m_count) /m_count*2/
      |                        1
Error: Syntax error in data declaration at (1)

intel/ifx and ifort

error #6669: The specification of this object is invalid.
integer :: foo(m_count) /m_count*2/

Classic Flang/nvfortran

F90-S-0034-Syntax error at or near / (abc.f90: 5)
  0 inform,   0 warnings,   1 severes, 0 fatal for name

-> Do you want to add the testcase along with the patch?

[unterumarmung]

In D124914#3490765, @kiranchandramohan wrote:

For parser, semantics, runtime patches please add @klausler as the primary reviewer.

I'm sorry, I'm fairly new to the Phabricator and LLVM and I didn't know that a reviewer can be primary. If I understand correctly, the primary reviewer is the first one in the list?

In D124914#3490765, @kiranchandramohan wrote:

As far as this patch is concerned,
-> Isn't the ideal behaviour a parse/semantic error? Other compilers give the following error messages:

You're totally right! I accidently confused code snippets I used for testing.
The given snippet is not correct and should give a meaningful semantic error.
Yet, the following code should be compiled with no problems:

program name
  implicit none
  integer, parameter :: m_count = 1
  integer foo(m_count) /m_count*2/
end program name

Invocations of gfortran and nvfortran give no errors:

$ gfortran a.f90
$ nvfortran a.f90

But flang gives the error:

$ build/bin/flang-new a.f90
error: Semantic errors in a.f90
./a.f90:4:25: error: Internal: no symbol found for 'm_count'
    integer foo(m_count) /m_count*2/
                          ^^^^^^^

[unterumarmung]

In D124914#3490765, @kiranchandramohan wrote:

-> Do you want to add the testcase along with the patch?

If this patch remains fixing only the crash, I do not think that test case is needed. I'm not sure that adding a test case for internal compiler error is ok. If it is, I'll add the test case.
If I fix the error fully, I'm going to add the test case. Maybe the full solution will be done in another patch. I'd like to gather opinions how I should address this.

What do you think?

[klausler]

In D124914#3490820, @unterumarmung wrote:

In D124914#3490765, @kiranchandramohan wrote:

-> Do you want to add the testcase along with the patch?

If this patch remains fixing only the crash, I do not think that test case is needed. I'm not sure that adding a test case for internal compiler error is ok. If it is, I'll add the test case.
If I fix the error fully, I'm going to add the test case. Maybe the full solution will be done in another patch. I'd like to gather opinions how I should address this.

What do you think?

First, the DATA-statement style initializer syntax is a legacy feature; the use of the "::" probably disables it in other compilers.

I think you should try to do more than patch just the crash. This program should compile.

And you don't have to add me as "primary reviewer". I'm swamped.

[unterumarmung]

Fixed internal error, added test case, rebased

[unterumarmung]

Fixed a typo in the commit message

[unterumarmung]

In D124914#3491087, @klausler wrote:

I think you should try to do more than patch just the crash. This program should compile.

I fixed the issue, could you please take a look?

In D124914#3491087, @klausler wrote:

And you don't have to add me as "primary reviewer". I'm swamped.

Ok. Is there anyone less busy I could request review from for my changes in parsing and semantics?

[unterumarmung]

Fixed newline at end of file for the test case

[unterumarmung]

@kiranchandramohan, could you please take a look?

[kiranchandramohan]

Apologies for the confusion with the reviewers. Please feel free to add reviewers from this list. Depending on time, and familiarity with the area one or more will respond. Thanks for the contribution.

I have a couple of minor comments.

flang/lib/Semantics/data-to-inits.cpp

320–322

Nit: Is this empty line required?

flang/test/Semantics/resolve111.f90

2

I think it would be better to have a debug-dump-symbols test for this (Only test what is in the MainProgram, particularly the init portion of the foo line)
!RUN: %flang_fc1 -fdebug-dump-symbols %s | FileCheck %s

[unterumarmung]

Address review comments, rebase

[unterumarmung]

@kiranchandramohan, I addressed your review comments, thank you!
If you are ok with the changes, could you please remove "request changes" on your review?

[kiranchandramohan]

LGTM. Thanks.

flang/lib/Semantics/data-to-inits.cpp

 public:
   ValueListIterator(SemanticsContext &context, const std::list<DSV> &list)
       : context_{context}, end_{list.end()}, at_{list.begin()} {
     SetRepetitionCount();
   }
   bool hasFatalError() const { return hasFatalError_; }
   bool IsAtEnd() const { return at_ == end_; }
   const SomeExpr *operator*() const { return GetExpr(context_, GetConstant()); }
-  parser::CharBlock LocateSource() const { return GetConstant().source; }
+  std::optional<parser::CharBlock> LocateSource() const {
+    if (!hasFatalError_) {
+      return GetConstant().source;
+    }
+    return {};
+  }
   ValueListIterator &operator++() {
     if (repetitionsRemaining_ > 0) {
       --repetitionsRemaining_;
     } else if (at_ != end_) {
       ++at_;
       SetRepetitionCount();
     }
     return *this;

 template <typename DSV>
 bool DataInitializationCompiler<DSV>::InitElement(
     const evaluate::OffsetSymbol &offsetSymbol, const SomeExpr &designator) {
   const Symbol &symbol{offsetSymbol.symbol()};
   const Symbol *lastSymbol{GetLastSymbol(designator)};
   bool isPointer{lastSymbol && IsPointer(*lastSymbol)};
   bool isProcPointer{lastSymbol && IsProcedurePointer(*lastSymbol)};
   evaluate::FoldingContext &context{exprAnalyzer_.GetFoldingContext()};
-  auto restorer{context.messages().SetLocation(values_.LocateSource())};
+  auto &messages{context.messages()};
+  auto restorer{
+      messages.SetLocation(values_.LocateSource().value_or(messages.at()))};
 
   const auto DescribeElement{[&]() {
     if (auto badDesignator{
             evaluate::OffsetToDesignator(context, offsetSymbol)}) {
       return badDesignator->AsFortran();
     } else {
       // Error recovery
       std::string buf;

flang/lib/Semantics/resolve-names.cpp

common::visit(
               }
             },
             [&](const parser::InitialDataTarget &) {
               // Defer analysis to the end of the specification part
               // so that forward references and attribute checks like SAVE
               // work better.
               ultimate.set(Symbol::Flag::InDataStmt);
             },
-            [&](const std::list<Indirection<parser::DataStmtValue>> &) {
+            [&](const std::list<Indirection<parser::DataStmtValue>> &values) {
               // Handled later in data-to-inits conversion
               ultimate.set(Symbol::Flag::InDataStmt);
+              Walk(values);
             },
         },
         init.u);
   }
 }
 
 void DeclarationVisitor::PointerInitialization(
     const parser::Name &name, const parser::InitialDataTarget &target) {

flang/test/Semantics/resolve111.f90

+!RUN: %flang_fc1 -fdebug-dump-symbols %s | FileCheck %s
+!Resolve names in legacy data initializers
+program name
+  implicit none
+  integer, parameter :: bar = 1
+  integer foo(bar) /bar*2/ !CHECK: foo (InDataStmt) size=4 offset=4: ObjectEntity type: INTEGER(4) shape: 1_8:1_8 init:[INTEGER(4)::2_4]
+end program name