Prevent the Swift scratch AST context to be reset while it is being used.

This patch introduces a Reader-Writer-Lock around the scratch
context. Users of a scratch context acquire a read lock while they are
depending on a scratch context. Only if no readers a around is Target
allowed to reset a Swift scratch context and delete the old one.

The Swift scratch context may need to be replaced when it gets corrupted,
for example due to incompatible ClangImporter options. This locking
mechanism guarantees that this won't happen while a client is using the
context.

In Swift there are three use-cases for ASTContexts with
different requirements and guarantees.

- Module ASTContexts are used for the static type system. They
  are created once for each lldb::Module and live forever.

- Scratch AST Contexts are used for expressions
  (thus far everything is like in the Clang language support)

- Scratch AST Contexts are also used to express the results of
   any dynamic type resolution done by RemoteAST or Archetype
   binding.

Because expressions and dynamic type resolution may trigger the
import of another module, the scratch context may become
unusable. When a scratch context is in a fatal error state,
GetScratchSwiftASTContext() will create a fresh global context,
or even separate scratch contexts for each lldb::Module. But it
will only do this if no client holds on to a read lock on \b
m_scratch_typesystem_lock.

rdar://problem/42399818

apple-llvm-split-commit: 8d2c620ea07618284b8da074ba49a22e2a7becda
apple-llvm-split-dir: lldb/

Author: adrian-prantl

lldb/include/lldb/Core/SwiftASTContextReader.h

@@ -0,0 +1,104 @@
+//===-- SwiftASTContextReader.h ---------------------------------*- C++ -*-===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2018 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef liblldb_SwiftASTContextReader_h_
+#define liblldb_SwiftASTContextReader_h_
+
+#include "llvm/Support/RWMutex.h"
+
+namespace lldb_private {
+
+class SwiftASTContext;
+class ExecutionContext;
+class ExecutionContextRef;
+
+/// This is like llvm::sys::SmartRWMutex<false>, but with a try_lock method.
+///
+/// FIXME: Replace this with a C++14 shared_timed_mutex or a C++17
+/// share_mutex as soon as possible. This implementation still allows
+/// for a race condition in \c try_lock() between the checking of
+/// m_readers and the lock acquisition.
+class SharedMutex {
+  llvm::sys::RWMutexImpl m_impl;
+  unsigned m_readers = 0;
+public:
+  SharedMutex() = default;
+  SharedMutex(const SharedMutex &) = delete;
+  SharedMutex &operator=(const SharedMutex &) = delete;
+  bool lock_shared() { ++m_readers; return m_impl.reader_acquire(); }
+  bool unlock_shared() { --m_readers; return m_impl.reader_release(); }
+  bool try_lock() { return m_readers ? false : m_impl.writer_acquire(); }
+  bool unlock() { return m_impl.writer_release(); }
+};
+
+/// RAII acquisition of a reader lock.
+struct ScopedSharedMutexReader {
+  SharedMutex *m_mutex;
+  ScopedSharedMutexReader(const ScopedSharedMutexReader&) = default;
+  explicit ScopedSharedMutexReader(SharedMutex *m) : m_mutex(m) {
+    if (m_mutex)
+      m_mutex->lock_shared();
+  }
+
+  ~ScopedSharedMutexReader() {
+    if (m_mutex)
+      m_mutex->unlock_shared();
+  }
+};
+
+/// A scratch Swift AST context pointer and its reader lock.
+/// The Swift scratch context may need to be replaced when it gets corrupted,
+/// for example due to incompatible ClangImporter options. This locking
+/// mechanism guarantees that this won't happen while a client is using the
+/// context.
+///
+/// In Swift there are three use-cases for ASTContexts with
+/// different requirements and guarantees.
+///
+/// - Module ASTContexts are used for the static type system. They
+///   are created once for each lldb::Module and live forever.
+///
+/// - Scratch AST Contexts are used for expressions
+///   (thus far everything is like in the Clang language support)
+///
+/// - Scratch AST Contexts are also used to express the results of
+///    any dynamic type resolution done by RemoteAST or Archetype
+///    binding.
+///
+/// Because expressions and dynamic type resolution may trigger the
+/// import of another module, the scratch context may become
+/// unusable. When a scratch context is in a fatal error state,
+/// GetScratchSwiftASTContext() will create a fresh global context,
+/// or even separate scratch contexts for each lldb::Module. But it
+/// will only do this if no client holds on to a read lock on \b
+/// m_scratch_typesystem_lock.
+struct SwiftASTContextReader : ScopedSharedMutexReader {
+  SwiftASTContext *m_ptr = nullptr;
+  SwiftASTContextReader() : ScopedSharedMutexReader(nullptr) {}
+  SwiftASTContextReader(SharedMutex &mutex, SwiftASTContext *ctx)
+      : ScopedSharedMutexReader(&mutex), m_ptr(ctx) {}
+  SwiftASTContextReader(const SwiftASTContextReader &copy)
+      : ScopedSharedMutexReader(copy.m_mutex), m_ptr(copy.m_ptr) {}
+  SwiftASTContext *get() { return m_ptr; }
+  operator bool() const { return m_ptr; }
+  SwiftASTContext *operator->() { return m_ptr; }
+  SwiftASTContext &operator*() { return *m_ptr; }
+};
+
+/// An RAII object that just acquires the reader lock.
+struct SwiftASTContextLock : ScopedSharedMutexReader {
+  SwiftASTContextLock(const ExecutionContextRef *exe_ctx_ref);
+  SwiftASTContextLock(const ExecutionContext *exe_ctx);
+};
+
+} // namespace lldb_private
+#endif

lldb/include/lldb/Core/ValueObject.h

@@ -10,6 +10,7 @@
 #ifndef liblldb_ValueObject_h_
 #define liblldb_ValueObject_h_
 
+#include "lldb/Core/SwiftASTContextReader.h"
 #include "lldb/Core/Value.h"
 #include "lldb/Symbol/CompilerType.h"
 #include "lldb/Symbol/Type.h" // for TypeImpl
@@ -615,7 +616,7 @@ class ValueObject : public UserID {
   virtual bool HasSyntheticValue();
 
   SwiftASTContext *GetSwiftASTContext();
-  SwiftASTContext *GetScratchSwiftASTContext();
+  SwiftASTContextReader GetScratchSwiftASTContext();
 
   virtual bool IsSynthetic() { return false; }
 

lldb/include/lldb/Core/ValueObjectDynamicValue.h

@@ -123,6 +123,10 @@ class ValueObjectDynamicValue : public ValueObject {
 
   bool HasDynamicValueTypeInfo() override { return true; }
 
+  /// Determine whether the scratch AST context has been replaced
+  /// since this dynamic type has been created.
+  bool DynamicValueTypeInfoNeedsUpdate();
+
   CompilerType GetCompilerTypeImpl() override;
 
   Address m_address; ///< The variable that this value object is based upon

lldb/include/lldb/Target/Target.h

@@ -71,6 +71,7 @@ typedef enum LoadCWDlldbinitFile {
   eLoadCWDlldbinitWarn
 } LoadCWDlldbinitFile;
 
+
 //----------------------------------------------------------------------
 // TargetProperties
 //----------------------------------------------------------------------
@@ -1130,29 +1131,18 @@ class Target : public std::enable_shared_from_this<Target>,
 
   lldb::ClangASTImporterSP GetClangASTImporter();
 
-  /// Convenience wrapper that extracts the scope's module first.
-  SwiftASTContext *GetScratchSwiftASTContext(Status &error,
-                                             ExecutionContextScope &exe_scope,
-                                             bool create_on_demand = true);
-
-#ifdef __clang_analyzer__
-  // See GetScratchTypeSystemForLanguage()
-  SwiftASTContext *GetScratchSwiftASTContext(
-      Status &error, Module *lldb_module, bool create_on_demand = true,
-      const char *extra_options = nullptr) __attribute__((always_inline)) {
-    SwiftASTContext *ret = GetScratchSwiftASTContextImpl(
-        error, lldb_module, create_on_demand, extra_options);
-
-    return ret ? ret : nullptr;
+  /// Get the lock guarding the scratch typesystem from being re-initialized.
+  SharedMutex &GetSwiftScratchContextLock() {
+    return m_scratch_typesystem_lock;
   }
 
-  SwiftASTContext *GetScratchSwiftASTContextImpl(Status &error,
-                                                 Module *lldb_module,
-                                                 bool create_on_demand = true);
-#else
-  SwiftASTContext *GetScratchSwiftASTContext(Status &error, Module *lldb_module,
-                                             bool create_on_demand = true);
-#endif
+  /// Convenience wrapper that extracts the scope's module first.
+  SwiftASTContextReader
+  GetScratchSwiftASTContext(Status &error, ExecutionContextScope &exe_scope,
+                            bool create_on_demand = true);
+  SwiftASTContextReader
+  GetScratchSwiftASTContext(Status &error, Module *lldb_module,
+                            bool create_on_demand = true);
 
 private:
   void DisplayFallbackSwiftContextErrors(SwiftASTContext *swift_ast_ctx);
@@ -1417,6 +1407,9 @@ class Target : public std::enable_shared_from_this<Target>,
   llvm::DenseMap<ModuleLanguage, lldb::TypeSystemSP>
       m_scratch_typesystem_for_module;
 
+  /// Guards the scratch typesystem from being re-initialized.
+  SharedMutex m_scratch_typesystem_lock;
+
   static void ImageSearchPathsChanged(const PathMappingList &path_list,
                                       void *baton);
 

lldb/source/Core/ValueObject.cpp

@@ -155,6 +155,15 @@ bool ValueObject::UpdateValueIfNeeded(bool update_format) {
 
   bool did_change_formats = false;
 
+  // Swift: Check whether the dynamic type system became stale.
+  if (m_dynamic_value) {
+    auto *dyn_val = static_cast<ValueObjectDynamicValue *>(m_dynamic_value);
+    if (dyn_val->DynamicValueTypeInfoNeedsUpdate()) {
+      dyn_val->SetNeedsUpdate();
+      SetNeedsUpdate();
+    }
+  }
+  
   if (update_format)
     did_change_formats = UpdateFormatsIfNeeded();
 
@@ -1742,13 +1751,13 @@ SwiftASTContext *ValueObject::GetSwiftASTContext() {
     auto ts = module_sp->GetTypeSystemForLanguage(lldb::eLanguageTypeSwift);
     return llvm::dyn_cast_or_null<SwiftASTContext>(ts);
   }
-  return GetScratchSwiftASTContext();
+  return GetScratchSwiftASTContext().get();
 }
 
-SwiftASTContext *ValueObject::GetScratchSwiftASTContext() {
+SwiftASTContextReader ValueObject::GetScratchSwiftASTContext() {
   lldb::TargetSP target_sp(GetTargetSP());
   if (!target_sp)
-    return nullptr;
+    return {};
 
   Status error;
   ExecutionContext ctx = GetExecutionContextRef().Lock(false);
@@ -2831,9 +2840,16 @@ void ValueObject::LogValueObject(Log *log,
   }
 }
 
+static const ExecutionContextRef *GetSwiftExeCtx(ValueObject &valobj) {
+  return (valobj.GetPreferredDisplayLanguage() == eLanguageTypeSwift)
+             ? &valobj.GetExecutionContextRef()
+             : nullptr;
+}
+
 void ValueObject::Dump(Stream &s) { Dump(s, DumpValueObjectOptions(*this)); }
 
 void ValueObject::Dump(Stream &s, const DumpValueObjectOptions &options) {
+  auto swift_scratch_ctx_lock = SwiftASTContextLock(GetSwiftExeCtx(*this));
   ValueObjectPrinter printer(this, &s, options);
   printer.PrintValueObject();
 }

lldb/source/Core/ValueObjectDynamicValue.cpp

@@ -145,6 +145,8 @@ bool ValueObjectDynamicValue::UpdateValue() {
     m_data.SetByteOrder(target->GetArchitecture().GetByteOrder());
     m_data.SetAddressByteSize(target->GetArchitecture().GetAddressByteSize());
   }
+ 
+  auto swift_scratch_ctx_lock = SwiftASTContextLock(&exe_ctx);
 
   // First make sure our Type and/or Address haven't changed:
   Process *process = exe_ctx.GetProcessPtr();
@@ -409,3 +411,15 @@ void ValueObjectDynamicValue::SetLanguageFlags(uint64_t flags) {
   else
     this->ValueObject::SetLanguageFlags(flags);
 }
+
+bool ValueObjectDynamicValue::DynamicValueTypeInfoNeedsUpdate() {
+  if (GetPreferredDisplayLanguage() != eLanguageTypeSwift)
+    return false;
+
+  if (!m_dynamic_type_info.HasType())
+    return false;
+
+  auto *cached_ctx =
+      static_cast<SwiftASTContext *>(m_value.GetCompilerType().GetTypeSystem());
+  return cached_ctx == GetScratchSwiftASTContext().get();
+}

lldb/source/Expression/Materializer.cpp

@@ -922,10 +922,12 @@ class EntityResultVariable : public Materializer::Entity {
 
     Status type_system_error;
     TypeSystem *type_system;
-    
+
     if (lang == lldb::eLanguageTypeSwift)
+      // We already acquired the lock in the UserExpression.
       type_system =
-          target_sp->GetScratchSwiftASTContext(type_system_error, *frame_sp);
+          target_sp->GetScratchSwiftASTContext(type_system_error, *frame_sp)
+              .get();
     else
       type_system = target_sp->GetScratchTypeSystemForLanguage(
         &type_system_error, m_type.GetMinimumLanguage());

lldb/source/Expression/UserExpression.cpp

@@ -261,6 +261,11 @@ lldb::ExpressionResults UserExpression::Evaluate(
     execution_results = lldb::eExpressionParseError;
     if (fixed_expression && !fixed_expression->empty() &&
         options.GetAutoApplyFixIts()) {
+      // Swift: temporarily release scratch context lock held by
+      // SwiftUserExpression, so the context can be restored if necessary.
+      if (expr == *fixed_expression)
+        user_expression_sp = nullptr;
+
       lldb::UserExpressionSP fixed_expression_sp(
           target->GetUserExpressionForLanguage(exe_ctx,
                                                fixed_expression->c_str(),