[flang] Change how memory for Symbol instances is managed.

With this change, all instances Symbol are stored in class Symbols.
Scope.symbols_, which used to own the symbol memory, now maps names to
Symbol* instead. This causes a bunch of reference-to-pointer changes
because of the change in type of key-value pairs. It also requires a
default constructor for Symbol, which means owner_ can't be a reference.

Symbols manages Symbol instances by allocating a block of them at a time
and returning the next one when needed. They are never freed.

The reason for the change is that there are a few cases where we need
to have a two symbols with the same name, so they can't both live in
the map in Scope. Those are:
1. When there is an erroneous redeclaration of a name we may delete the
   first symbol and replace it with a new one. If we have saved a pointer
   to the first one it is now dangling. This can be seen by running
   `f18 -fdebug-dump-symbols -fparse-only test/semantics/resolve19.f90`
   under valgrind. Subroutine s is declared twice: each results in a
   scope that contains a pointer back to the symbol for the subroutine.
   After the second symbol for s is created the first is gone so the
   pointer in the scope is invalid.
2. A generic and one of its specifics can have the same name. We currently
   handle that by moving the symbol for the specific into a unique_ptr
   in the generic. So in that case the symbol is owned by another symbol
   instead of by the scope. It is simpler if we only have to deal with
   moving the raw pointer around.
3. A generic and a derived type can have the same name. This case isn't
   handled yet, but it can be done like flang-compiler/f18#2 above. It's more complicated
   because the derived type and the generic can be declared in either
   order.

Original-commit: flang-compiler/f18@55a68cf0235c8a3ac855de7dc0e2b08690866be0
Reviewed-on: https://github.com/flang-compiler/f18/pull/107

diff --git a/flang/lib/semantics/resolve-names.cc b/flang/lib/semantics/resolve-names.cc
index 06cbdeb..c2fd0ab 100644 (file)
--- a/flang/lib/semantics/resolve-names.cc
+++ b/flang/lib/semantics/resolve-names.cc
@@ -298,7 +298,7 @@ public:
     if (!symbol) {
       const auto pair = CurrScope().try_emplace(name, attrs, details);
       CHECK(pair.second);  // name was not found, so must be able to add
-      return pair.first->second;
+      return *pair.first->second;
     }
     symbol->add_occurrence(name);
     if (symbol->CanReplaceDetails(details)) {
@@ -325,6 +325,10 @@ public:
   Symbol &MakeSymbol(const parser::Name &name, D &&details) {
     return MakeSymbol(name, Attrs(), details);
   }
+  template<typename D>
+  Symbol &MakeSymbol(const SourceName &name, D &&details) {
+    return MakeSymbol(name, Attrs(), details);
+  }
   Symbol &MakeSymbol(const SourceName &name, Attrs attrs = Attrs{}) {
     return MakeSymbol(name, attrs, UnknownDetails());
   }
@@ -390,7 +394,7 @@ protected:
   void AddToGeneric(const parser::Name &name, bool expectModuleProc = false);
   void AddToGeneric(const Symbol &symbol);
   // Add to generic the symbol for the subprogram with the same name
-  void SetSpecificInGeneric(Symbol &&symbol);
+  void SetSpecificInGeneric(Symbol *symbol);
 
 private:
   bool inInterfaceBlock_{false};  // set when in interface block
@@ -1072,7 +1076,7 @@ Symbol *ScopeHandler::FindSymbol(const SourceName &name) {
   if (it == CurrScope().end()) {
     return nullptr;
   } else {
-    return &it->second;
+    return it->second;
   }
 }
 void ScopeHandler::EraseSymbol(const SourceName &name) {
@@ -1083,7 +1087,7 @@ void ScopeHandler::ApplyImplicitRules() {
   if (!isImplicitNoneType()) {
     implicitRules().AddDefaultRules();
     for (auto &pair : CurrScope()) {
-      Symbol &symbol = pair.second;
+      Symbol &symbol = *pair.second;
       if (symbol.has<UnknownDetails>()) {
         symbol.set_details(ObjectEntityDetails{});
       } else if (auto *details = symbol.detailsIf<EntityDetails>()) {
@@ -1145,7 +1149,7 @@ bool ModuleVisitor::Pre(const parser::UseStmt &x) {
     Say(x.moduleName, "Module '%s' not found"_err_en_US);
     return false;
   }
-  const auto *details = it->second.detailsIf<ModuleDetails>();
+  const auto *details = it->second->detailsIf<ModuleDetails>();
   if (!details) {
     Say(x.moduleName, "'%s' is not a module"_err_en_US);
     return false;
@@ -1173,7 +1177,7 @@ void ModuleVisitor::Post(const parser::UseStmt &x) {
     }
     const SourceName &moduleName{x.moduleName.source};
     for (const auto &pair : *useModuleScope_) {
-      const Symbol &symbol{pair.second};
+      const Symbol &symbol{*pair.second};
       if (symbol.attrs().test(Attr::PUBLIC) &&
           !symbol.detailsIf<ModuleDetails>()) {
         const SourceName &name{symbol.name()};
@@ -1205,7 +1209,7 @@ void ModuleVisitor::AddUse(const SourceName &location,
         useModuleScope_->name());
     return;
   }
-  const Symbol &useSymbol{it->second};
+  const Symbol &useSymbol{*it->second};
   if (useSymbol.attrs().test(Attr::PRIVATE)) {
     Say(useName, "'%s' is PRIVATE in '%s'"_err_en_US, useName,
         useModuleScope_->name());
@@ -1258,7 +1262,7 @@ void ModuleVisitor::Post(const parser::Module &) {
 
 void ModuleVisitor::ApplyDefaultAccess() {
   for (auto &pair : CurrScope()) {
-    Symbol &symbol = pair.second;
+    Symbol &symbol = *pair.second;
     if (!symbol.attrs().HasAny({Attr::PUBLIC, Attr::PRIVATE})) {
       symbol.attrs().set(defaultAccess_);
     }
@@ -1336,11 +1340,10 @@ bool InterfaceVisitor::Pre(const parser::GenericSpec &x) {
     } else {
       CHECK(!"can't happen");
     }
-    Symbol symbol{CurrScope(), genericSymbol_->name(), genericSymbol_->attrs(),
-        std::move(details)};
+    GenericDetails genericDetails;
+    genericDetails.set_specific(genericSymbol_);
     EraseSymbol(*genericName);
-    genericSymbol_ = &MakeSymbol(*genericName);
-    genericSymbol_->set_details(GenericDetails{std::move(symbol)});
+    genericSymbol_ = &MakeSymbol(*genericName, genericDetails);
   }
   CHECK(genericSymbol_->has<GenericDetails>());
   return false;
@@ -1383,8 +1386,7 @@ void InterfaceVisitor::AddToGeneric(
     return;
   }
   if (symbol == genericSymbol_) {
-    if (auto *specific =
-            genericSymbol_->details<GenericDetails>().specific().get()) {
+    if (auto *specific = genericSymbol_->details<GenericDetails>().specific()) {
       symbol = specific;
     }
   }
@@ -1404,8 +1406,8 @@ void InterfaceVisitor::AddToGeneric(
 void InterfaceVisitor::AddToGeneric(const Symbol &symbol) {
   genericSymbol_->details<GenericDetails>().add_specificProc(&symbol);
 }
-void InterfaceVisitor::SetSpecificInGeneric(Symbol &&symbol) {
-  genericSymbol_->details<GenericDetails>().set_specific(std::move(symbol));
+void InterfaceVisitor::SetSpecificInGeneric(Symbol *symbol) {
+  genericSymbol_->details<GenericDetails>().set_specific(symbol);
 }
 
 // SubprogramVisitor implementation
@@ -1442,7 +1444,7 @@ bool SubprogramVisitor::Pre(const parser::StmtFunctionStmt &x) {
     EntityDetails dummyDetails{true};
     auto it = CurrScope().parent().find(dummyName.source);
     if (it != CurrScope().parent().end()) {
-      if (auto *d = it->second.detailsIf<EntityDetails>()) {
+      if (auto *d = it->second->detailsIf<EntityDetails>()) {
         if (d->type()) {
           dummyDetails.set_type(*d->type());
         }
@@ -1612,14 +1614,14 @@ Symbol *SubprogramVisitor::GetSpecificFromGeneric(const parser::Name &name) {
   if (Symbol *symbol = FindSymbol(name.source)) {
     if (auto *details = symbol->detailsIf<GenericDetails>()) {
       // found generic, want subprogram
-      auto *specific = details->specific().get();
+      auto *specific = details->specific();
       if (isGeneric()) {
         if (specific) {
           SayAlreadyDeclared(name.source, *specific);
         } else {
-          SetSpecificInGeneric(
-              Symbol{CurrScope(), name.source, Attrs{}, SubprogramDetails{}});
-          specific = details->specific().get();
+          specific = &CurrScope().MakeSymbol(
+              name.source, Attrs{}, SubprogramDetails{});
+          SetSpecificInGeneric(specific);
         }
       }
       if (specific) {
@@ -1712,7 +1714,7 @@ bool DeclarationVisitor::HandleAttributeStmt(
     const auto pair = CurrScope().try_emplace(name.source, Attrs{attr});
     if (!pair.second) {
       // symbol was already there: set attribute on it
-      Symbol &symbol{pair.first->second};
+      Symbol &symbol{*pair.first->second};
       if (attr != Attr::ASYNCHRONOUS && attr != Attr::VOLATILE &&
           symbol.has<UseDetails>()) {
         Say(*currStmtSource(),
@@ -2060,7 +2062,7 @@ void ResolveNamesVisitor::Post(const parser::SpecificationPart &s) {
     // Check that every name referenced has an explicit type
     for (const auto &pair : CurrScope()) {
       const auto &name = pair.first;
-      const auto &symbol = pair.second;
+      const auto &symbol = *pair.second;
       if (NeedsExplicitType(symbol)) {
         Say(name, "No explicit type declared for '%s'"_err_en_US);
       }
@@ -2249,7 +2251,7 @@ static void DumpSymbols(std::ostream &os, const Scope &scope, int indent = 0) {
   ++indent;
   for (const auto &symbol : scope) {
     PutIndent(os, indent);
-    os << symbol.second << "\n";
+    os << *symbol.second << "\n";
   }
   for (const auto &child : scope.children()) {
     DumpSymbols(os, child, indent);

diff --git a/flang/lib/semantics/scope.cc b/flang/lib/semantics/scope.cc
index c766588..e2f0ec4 100644 (file)
--- a/flang/lib/semantics/scope.cc
+++ b/flang/lib/semantics/scope.cc
@@ -22,6 +22,8 @@ const Scope Scope::systemScope{
     Scope::systemScope, Scope::Kind::System, nullptr};
 Scope Scope::globalScope{Scope::systemScope, Scope::Kind::Global, nullptr};
 
+Symbols<1024> Scope::allSymbols;
+
 Scope &Scope::MakeScope(Kind kind, Symbol *symbol) {
   children_.emplace_back(*this, kind, symbol);
   return children_.back();
@@ -34,7 +36,7 @@ std::ostream &operator<<(std::ostream &os, const Scope &scope) {
   }
   os << scope.children_.size() << " children\n";
   for (const auto &sym : scope.symbols_) {
-    os << "  " << sym.second << "\n";
+    os << "  " << *sym.second << "\n";
   }
   return os;
 }

diff --git a/flang/lib/semantics/scope.h b/flang/lib/semantics/scope.h
index 40311a1..7ee9c28 100644 (file)
--- a/flang/lib/semantics/scope.h
+++ b/flang/lib/semantics/scope.h
@@ -26,7 +26,7 @@
 namespace Fortran::semantics {
 
 class Scope {
-  using mapType = std::map<SourceName, Symbol>;
+  using mapType = std::map<SourceName, Symbol *>;
 
 public:
   // root of the scope tree; contains intrinsics:
@@ -86,7 +86,14 @@ public:
   template<typename D>
   std::pair<iterator, bool> try_emplace(
       const SourceName &name, Attrs attrs, D &&details) {
-    return symbols_.try_emplace(name, *this, name, attrs, details);
+    Symbol &symbol{MakeSymbol(name, attrs, std::move(details))};
+    return symbols_.insert(std::make_pair(name, &symbol));
+  }
+
+  /// Make a Symbol but don't add it to the scope.
+  template<typename D>
+  Symbol &MakeSymbol(const SourceName &name, Attrs attrs, D &&details) {
+    return allSymbols.Make(*this, name, attrs, std::move(details));
   }
 
   std::list<Scope> &children() { return children_; }
@@ -99,6 +106,10 @@ private:
   std::list<Scope> children_;
   mapType symbols_;
 
+  // Storage for all Symbols. Every Symbol is in allSymbols and every Symbol*
+  // or Symbol& points to one in there.
+  static Symbols<1024> allSymbols;
+
   friend std::ostream &operator<<(std::ostream &, const Scope &);
 };
 

diff --git a/flang/lib/semantics/symbol.cc b/flang/lib/semantics/symbol.cc
index c79d2b8..c38881a 100644 (file)
--- a/flang/lib/semantics/symbol.cc
+++ b/flang/lib/semantics/symbol.cc
@@ -60,20 +60,19 @@ GenericDetails::GenericDetails(const listType &specificProcs) {
   }
 }
 
-void GenericDetails::set_specific(Symbol &&specific) {
+void GenericDetails::set_specific(Symbol *specific) {
   CHECK(!specific_);
-  specific_ = std::unique_ptr<Symbol>(new Symbol(std::move(specific)));
+  specific_ = specific;
 }
 
 const Symbol *GenericDetails::CheckSpecific() const {
-  if (const auto *specific = specific_.get()) {
+  if (specific_) {
     for (const auto *proc : specificProcs_) {
-      if (proc == specific) {
+      if (proc == specific_) {
         return nullptr;
       }
     }
-    std::cerr << "specific: " << *specific << "\n";
-    return specific;
+    return specific_;
   } else {
     return nullptr;
   }

diff --git a/flang/lib/semantics/symbol.h b/flang/lib/semantics/symbol.h
index 4af6135..600e3c8 100644 (file)
--- a/flang/lib/semantics/symbol.h
+++ b/flang/lib/semantics/symbol.h
@@ -182,13 +182,13 @@ public:
   using listType = std::list<const Symbol *>;
   GenericDetails() {}
   GenericDetails(const listType &specificProcs);
-  GenericDetails(Symbol &&specific) { set_specific(std::move(specific)); }
+  GenericDetails(Symbol *specific) : specific_{specific} {}
 
   const listType specificProcs() const { return specificProcs_; }
   void add_specificProc(const Symbol *proc) { specificProcs_.push_back(proc); }
 
-  std::unique_ptr<Symbol> &specific() { return specific_; }
-  void set_specific(Symbol &&specific);
+  Symbol *specific() { return specific_; }
+  void set_specific(Symbol *specific);
 
   // Check that specific is one of the specificProcs. If not, return the
   // specific as a raw pointer.
@@ -198,7 +198,7 @@ private:
   // all of the specific procedures for this generic
   listType specificProcs_;
   // a specific procedure with the same name as this generic, if any
-  std::unique_ptr<Symbol> specific_;
+  Symbol *specific_{nullptr};
 };
 
 class UnknownDetails {};
@@ -214,12 +214,7 @@ public:
   ENUM_CLASS(Flag, Function, Subroutine);
   using Flags = common::EnumSet<Flag, Flag_enumSize>;
 
-  Symbol(const Scope &owner, const SourceName &name, const Attrs &attrs,
-      Details &&details)
-    : owner_{owner}, attrs_{attrs}, details_{std::move(details)} {
-    add_occurrence(name);
-  }
-  const Scope &owner() const { return owner_; }
+  const Scope &owner() const { return *owner_; }
   const SourceName &name() const { return occurrences_.front(); }
   Attrs &attrs() { return attrs_; }
   const Attrs &attrs() const { return attrs_; }
@@ -273,17 +268,54 @@ public:
   bool operator!=(const Symbol &that) const { return this != &that; }
 
 private:
-  const Scope &owner_;
+  const Scope *owner_;
   std::list<SourceName> occurrences_;
   Attrs attrs_;
   Flags flags_;
   Details details_;
 
+  Symbol() {}  // only created in class Symbols
   const std::string GetDetailsName() const;
   friend std::ostream &operator<<(std::ostream &, const Symbol &);
+  template<std::size_t> friend class Symbols;
+  template<class, std::size_t> friend class std::array;
 };
 
 std::ostream &operator<<(std::ostream &, Symbol::Flag);
 
+// Manage memory for all symbols. BLOCK_SIZE symbols at a time are allocated.
+// Make() returns a reference to the next available one. They are never
+// deleted.
+template<std::size_t BLOCK_SIZE> class Symbols {
+public:
+  Symbol &Make(const Scope &owner, const SourceName &name, const Attrs &attrs,
+      Details &&details) {
+    Symbol &symbol = Get();
+    symbol.owner_ = &owner;
+    symbol.occurrences_.push_back(name);
+    symbol.attrs_ = attrs;
+    symbol.details_ = std::move(details);
+    return symbol;
+  }
+
+private:
+  using blockType = std::array<Symbol, BLOCK_SIZE>;
+  std::list<blockType *> blocks_;
+  std::size_t nextIndex_{0};
+  blockType *currBlock_{nullptr};
+
+  Symbol &Get() {
+    if (nextIndex_ == 0) {
+      blocks_.push_back(new blockType());
+      currBlock_ = blocks_.back();
+    }
+    Symbol &result = (*currBlock_)[nextIndex_];
+    if (++nextIndex_ >= BLOCK_SIZE) {
+      nextIndex_ = 0;  // allocate a new block next time
+    }
+    return result;
+  }
+};
+
 }  // namespace Fortran::semantics
 #endif  // FORTRAN_SEMANTICS_SYMBOL_H_