[flang] Avoid cycles during instantiation of derived types

[rogfer01]

Summary

Derived-type-spec (such as type(t)) typically cause the instantiation of a class which is also used to define the offsets of its data components and the size of the class.

Fortran derived types are always "completely" defined (i.e., no incomplete / opaque derived types exist on which we can build a pointer to them like in C/C++) so they can have their offsets always computed.

However, we must be careful not to instantiate a derived type while it is being defined. This can happen due to cycles introduced by forward references, such as the one below.

type t1
  type(t2), pointer :: b ! (A)
end type t1

type :: t2 ! (B)
  type(t1), pointer :: a ! (C)
end type t2 ! (D)

At (A), flang determines that this is a forward declaration so no instantiation happens.

At (B), flang determines t2 is not a forward declaration anymore, because we are defining it.

At (C), flang chooses to instantiate t1. Instantiation of t1 finds the field b at (A). Now t2 is not a forward declaration anymore, so it can be instantiated. But at this point the field a has not been added to t2, so we compute the size of an empty class. Because this computation is done just once, we end emitting a wrong derived type descriptor with a sizeinbytes field set to 0.

Because these kind of cycles can only happen via forward referenced derived types specifiers, the idea here is to avoid instantiating the derived type being defined (i.e. t2) until (D). Keeping the attribute "is forward reference" on until (D) avoids that.

Fixes https://github.com/llvm/llvm-project/issues/64973

[rogfer01]

My familiarity with this part of the code is limited, so I welcome feedback on better integrating this with the existing machinery. Thanks!

[rogfer01]

Ok, I realised this is incomplete, an even simpler testcase still shows the issue:

program main
  implicit none

  type t1
    type(t2), pointer :: b
  end type t1

  type :: t2  ! (B)
    type(t1) :: a  ! no pointer
  end type t2 ! (D)
end program main

I will explore a different approach. Maybe we can not forget in (B) that t2 had been mentioned as a forward reference, this would prevent instantiation. Then we can drop that bit in (D).

[rogfer01]

ChangeLog:

• Remember that a derived type is forward declared until the end of the derived type definition. This way we avoid instantiating it while not completely defined.

[rogfer01]

Ping.

[PeteSteinfeld]

Thanks for finding and fixing this!

All builds and tests correctly and looks good to me. It would be good for @klausler to take a look, though.

[klausler]

I understand the problem but I'm not sure that this is a complete solution. Instantiating t1 at the end of t2 won't work if t1 also had a forward reference to another type t3. I'll have to think about this some more. It might be simpler and more complete to just defer all instantiations of pointer and allocatable component (but not variable) types unless the component appears in a specification expression. They'll get instantiated at the end of the specification part if they remain uninstantiated.

[rogfer01]

In D159117#4639726, @klausler wrote:

I understand the problem but I'm not sure that this is a complete solution. Instantiating t1 at the end of t2 won't work if t1 also had a forward reference to another type t3. I'll have to think about this some more. It might be simpler and more complete to just defer all instantiations of pointer and allocatable component (but not variable) types unless the component appears in a specification expression. They'll get instantiated at the end of the specification part if they remain uninstantiated.

Right. Apologies, @klausler, I may have confused you with the original description which I forgot to update along with my last change. I will update it.

In my last change, what I did was to keep marking t2 as forward referenced (if it had been marked as such because of such reference) until the end of its definition. The existing code does not attempt to instantiate those types. This means that while checking the definition of t2 we instantiate t1 and the latter (i.e. t1) won't attempt to instantiate t2. Once the whole type definition of t2 is complete we can instantiate it as needed, so we unmark it as forward referenced.

[klausler]

flang/lib/Semantics/resolve-names.cpp

5260–5266

It is weird to define a lambda with no arguments just so you can call it once. Just use a local variable.

[rogfer01]

ChangeLog:

• Simplify the DerivedTypeDetails creation. Don't use a lambda to get an rvalue. Instead, std::move a local variable to the symbol.

flang/lib/Semantics/resolve-names.cpp

5260–5266

Right, I didn't want to leave an empty (moved) husk after a std::move but I guess I'm just trying to hard.

flang/lib/Semantics/resolve-names.cpp

 bool DeclarationVisitor::Pre(const parser::DerivedTypeDef &x) {
   auto &stmt{std::get<parser::Statement<parser::DerivedTypeStmt>>(x.t)};
   Walk(stmt);
   Walk(std::get<std::list<parser::Statement<parser::TypeParamDefStmt>>>(x.t));
   auto &scope{currScope()};
   CHECK(scope.symbol());
   CHECK(scope.symbol()->scope() == &scope);
   auto &details{scope.symbol()->get<DerivedTypeDetails>()};
-  details.set_isForwardReferenced(false);
   std::set<SourceName> paramNames;
   for (auto &paramName : std::get<std::list<parser::Name>>(stmt.statement.t)) {
     details.add_paramName(paramName.source);
     auto *symbol{FindInScope(scope, paramName)};
     if (!symbol) {
       Say(paramName,
           "No definition found for type parameter '%s'"_err_en_US); // C742
       // No symbol for a type param.  Create one and mark it as containing an
if (derivedTypeInfo_.sequence) {
     }
     if (derivedTypeInfo_.extends) { // C735
       Say(stmt.source,
           "A sequence type may not have the EXTENDS attribute"_err_en_US);
     }
   }
   Walk(std::get<std::optional<parser::TypeBoundProcedurePart>>(x.t));
   Walk(std::get<parser::Statement<parser::EndTypeStmt>>(x.t));
+  details.set_isForwardReferenced(false);
   derivedTypeInfo_ = {};
   PopScope();
   return false;
 }
 
 bool DeclarationVisitor::Pre(const parser::DerivedTypeStmt &) {
   return BeginAttrs();
 }
 void DeclarationVisitor::Post(const parser::DerivedTypeStmt &x) {
   auto &name{std::get<parser::Name>(x.t)};
   // Resolve the EXTENDS() clause before creating the derived
   // type's symbol to foil attempts to recursively extend a type.
   auto *extendsName{derivedTypeInfo_.extends};
   std::optional<DerivedTypeSpec> extendsType{
       ResolveExtendsType(name, extendsName)};
-  auto &symbol{MakeSymbol(name, GetAttrs(), DerivedTypeDetails{})};
+  DerivedTypeDetails derivedTypeDetails;
+  if (Symbol *typeSymbol{FindInScope(currScope(), name)}; typeSymbol &&
+      typeSymbol->has<DerivedTypeDetails>() &&
+      typeSymbol->get<DerivedTypeDetails>().isForwardReferenced()) {
+    derivedTypeDetails.set_isForwardReferenced(true);
+  }
+  auto &symbol{MakeSymbol(name, GetAttrs(), std::move(derivedTypeDetails))};
   symbol.ReplaceName(name.source);
   derivedTypeInfo_.type = &symbol;
   PushScope(Scope::Kind::DerivedType, &symbol);
   if (extendsType) {
     // Declare the "parent component"; private if the type is.
     // Any symbol stored in the EXTENDS() clause is temporarily
     // hidden so that a new symbol can be created for the parent
     // component without producing spurious errors about already

flang/test/Semantics/typeinfo05.f90

+!RUN: bbc --dump-symbols %s | FileCheck %s
+!RUN: %flang_fc1 -fdebug-dump-symbols %s | FileCheck %s
+! Ensure that cycles via POINTERs do not instantiate incomplete derived
+! types that would lead to types whose sizeinbytes=0
+program main
+  implicit none
+  type t1
+    type(t2), pointer :: b
+  end type t1
+!CHECK: .dt.t1, SAVE, TARGET (CompilerCreated, ReadOnly): ObjectEntity type: TYPE(derivedtype) init:derivedtype(binding=NULL(),name=.n.t1,sizeinbytes=40_8,uninstantiated=NULL(),kindparameter=NULL(),lenparameterkind=NULL(),component=.c.t1,procptr=NULL(),special=NULL(),specialbitset=0_4,hasparent=0_1,noinitializationneeded=0_1,nodestructionneeded=1_1,nofinalizationneeded=1_1)
+  type :: t2
+    type(t1) :: a
+  end type t2
+! CHECK: .dt.t2, SAVE, TARGET (CompilerCreated, ReadOnly): ObjectEntity type: TYPE(derivedtype) init:derivedtype(binding=NULL(),name=.n.t2,sizeinbytes=40_8,uninstantiated=NULL(),kindparameter=NULL(),lenparameterkind=NULL(),component=.c.t2,procptr=NULL(),special=NULL(),specialbitset=0_4,hasparent=0_1,noinitializationneeded=0_1,nodestructionneeded=1_1,nofinalizationneeded=1_1)
+end program main
+

flang/test/Semantics/typeinfo06.f90

+!RUN: bbc --dump-symbols %s | FileCheck %s
+!RUN: %flang_fc1 -fdebug-dump-symbols %s | FileCheck %s
+! Ensure that cycles via ALLOCATABLEs do not instantiate incomplete derived
+! types that would lead to types whose sizeinbytes=0
+program main
+  implicit none
+  type t1
+    type(t2), allocatable :: b
+  end type t1
+!CHECK: .dt.t1, SAVE, TARGET (CompilerCreated, ReadOnly): ObjectEntity type: TYPE(derivedtype) init:derivedtype(binding=NULL(),name=.n.t1,sizeinbytes=40_8,uninstantiated=NULL(),kindparameter=NULL(),lenparameterkind=NULL(),component=.c.t1,procptr=NULL(),special=NULL(),specialbitset=0_4,hasparent=0_1,noinitializationneeded=0_1,nodestructionneeded=0_1,nofinalizationneeded=1_1)
+  type :: t2
+    type(t1) :: a
+  end type t2
+! CHECK: .dt.t2, SAVE, TARGET (CompilerCreated, ReadOnly): ObjectEntity type: TYPE(derivedtype) init:derivedtype(binding=NULL(),name=.n.t2,sizeinbytes=40_8,uninstantiated=NULL(),kindparameter=NULL(),lenparameterkind=NULL(),component=.c.t2,procptr=NULL(),special=NULL(),specialbitset=0_4,hasparent=0_1,noinitializationneeded=0_1,nodestructionneeded=0_1,nofinalizationneeded=1_1)
+end program main
+