[llvm-branch-commits] [flang] 2aa4335 - [flang] Fix copy elision assumption.

[Michael Kruse via llvm-branch-commits]

Author: Michael Kruse
Date: 2020-12-14T14:07:05-06:00
New Revision: 2aa43358060c6b34fb9cdc6c4321e958f62331e7

URL: https://github.com/llvm/llvm-project/commit/2aa43358060c6b34fb9cdc6c4321e958f62331e7
DIFF: https://github.com/llvm/llvm-project/commit/2aa43358060c6b34fb9cdc6c4321e958f62331e7.diff

LOG: [flang] Fix copy elision assumption.

Before this patch, the Restorer depended on copy elision to happen.
Without copy elision, the function ScopedSet calls the move constructor
before its dtor. The dtor will prematurely restore the reference to the
original value.

Instead of relying the compiler to not use the Restorer's copy
constructor, delete its copy and assign operators. Hence, callers cannot
move or copy a Restorer object anymore, and have to explicitly provide
the reset state. ScopedSet avoids calling move/copy operations by
relying on unnamed return value optimization, which is mandatory in
C++17.

Reviewed By: klausler

Differential Revision: https://reviews.llvm.org/D88797

Added: 
    

Modified: 
    flang/include/flang/Common/restorer.h
    flang/lib/Semantics/check-declarations.cpp

Removed: 
    


################################################################################
diff  --git a/flang/include/flang/Common/restorer.h b/flang/include/flang/Common/restorer.h
index 47e54237fe43..4d5f5e4e2c81 100644
--- a/flang/include/flang/Common/restorer.h
+++ b/flang/include/flang/Common/restorer.h
@@ -22,9 +22,16 @@
 namespace Fortran::common {
 template <typename A> class Restorer {
 public:
-  explicit Restorer(A &p) : p_{p}, original_{std::move(p)} {}
+  explicit Restorer(A &p, A original) : p_{p}, original_{std::move(original)} {}
   ~Restorer() { p_ = std::move(original_); }
 
+  // Inhibit any recreation of this restorer that would result in two restorers
+  // trying to restore the same reference.
+  Restorer(const Restorer &) = delete;
+  Restorer(Restorer &&that) = delete;
+  const Restorer &operator=(const Restorer &) = delete;
+  const Restorer &operator=(Restorer &&that) = delete;
+
 private:
   A &p_;
   A original_;
@@ -32,15 +39,15 @@ template <typename A> class Restorer {
 
 template <typename A, typename B>
 common::IfNoLvalue<Restorer<A>, B> ScopedSet(A &to, B &&from) {
-  Restorer<A> result{to};
+  A original{std::move(to)};
   to = std::move(from);
-  return result;
+  return Restorer<A>{to, std::move(original)};
 }
 template <typename A, typename B>
 common::IfNoLvalue<Restorer<A>, B> ScopedSet(A &to, const B &from) {
-  Restorer<A> result{to};
+  A original{std::move(to)};
   to = from;
-  return result;
+  return Restorer<A>{to, std::move(original)};
 }
 } // namespace Fortran::common
 #endif // FORTRAN_COMMON_RESTORER_H_

diff  --git a/flang/lib/Semantics/check-declarations.cpp b/flang/lib/Semantics/check-declarations.cpp
index 0d2e2e86241c..dd76f6710070 100644
--- a/flang/lib/Semantics/check-declarations.cpp
+++ b/flang/lib/Semantics/check-declarations.cpp
@@ -1498,7 +1498,7 @@ void CheckHelper::CheckProcBinding(
 
 void CheckHelper::Check(const Scope &scope) {
   scope_ = &scope;
-  common::Restorer<const Symbol *> restorer{innermostSymbol_};
+  common::Restorer<const Symbol *> restorer{innermostSymbol_, innermostSymbol_};
   if (const Symbol * symbol{scope.symbol()}) {
     innermostSymbol_ = symbol;
   }