[Expression] Move IRDynamicChecks to ClangExpressionParser

[xiaobai]

Summary

IRDynamicChecks in its current form is specific to Clang since it deals
with the C language family. It is possible that we may want to
instrument code generated for other languages, but we can factor in a
more general mechanism to do so at a later time.

This decouples ObCLanguageRuntime from Expression!

[JDevlieghere]

include/lldb/Expression/DynamicCheckerFunctions.h

20

Can we skip this boilerplate?

28

Reflow comment.

source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

1309–1310

Spurious newline?

[xiaobai]

Reflow comment
Remove newline

[xiaobai]

include/lldb/Expression/DynamicCheckerFunctions.h

20

Do you mean the whole thing? Or just the \class line?

[JDevlieghere]

include/lldb/Expression/DynamicCheckerFunctions.h

20

Everything up to "Encapsulates dynamic check...", Doxygen is smart enough to associate the comment with the class.

[xiaobai]

Remove documentation boilerplate

[jingham]

This is a little unclear to me. LLVMUserExpression is the class that represents "anything that uses LLVM at its back end." Both the Swift & Clang user expressions are instances of this. Running through IR and inserting little callouts is an LLVMUserExpression type thing. So it seems like this move is in the right direction, but to be really clean needs to represent LLVMUserExpression in the Plugin Hierarchy.

[xiaobai]

In D64591#1581775, @jingham wrote:

This is a little unclear to me. LLVMUserExpression is the class that represents "anything that uses LLVM at its back end." Both the Swift & Clang user expressions are instances of this. Running through IR and inserting little callouts is an LLVMUserExpression type thing. So it seems like this move is in the right direction, but to be really clean needs to represent LLVMUserExpression in the Plugin Hierarchy.

Right, this makes sense to me. My understanding was that LLVMUserExpression would eventually invoke ClangExpressionParser, which actually handles dealing with IR instrumentation. With swift in the picture, things get a little more complicated because you could want to deal with both ObjC code and Swift code. I think that in that case, LLDB would want to use both Clang and Swift to instrument a user expression. I don't think that the abstractions are set up to account for this use case right now, so it would require some planning and refactoring.

[jingham]

Then the IRDynamicCheck part would go with LLVMUserExpression, and the C-specific checks in Clang...

[xiaobai]

In D64591#1581820, @jingham wrote:

Then the IRDynamicCheck part would go with LLVMUserExpression, and the C-specific checks in Clang...

Except IRDynamicChecks has special knowledge of the clang-specific instrumenters. I can think of another way in which DynamicCheckerFunctions and IRDynamicChecks are more generic classes that will work with any language. I can try to explore that option to see what I come up with, maybe the end result will be better?

[jingham]

That would be cleaner.

OTOH, the original reason for these checkers was to help people understand crashes in their expressions more clearly. Supposedly, modern languages "don't have pointers" and can't have bad objects, so the kind of crashes this instrumentation was supposed to help with "can't happen" and checkers for such languages wouldn't be all that helpful...

So while cleaner, maybe generalizing this more fully isn't a high priority change? In which case, just getting them out of generic code seems fine as a stopping point. Your choice.

[xiaobai]

In D64591#1581878, @jingham wrote:

That would be cleaner.

OTOH, the original reason for these checkers was to help people understand crashes in their expressions more clearly. Supposedly, modern languages "don't have pointers" and can't have bad objects, so the kind of crashes this instrumentation was supposed to help with "can't happen" and checkers for such languages wouldn't be all that helpful...

So while cleaner, maybe generalizing this more fully isn't a high priority change? In which case, just getting them out of generic code seems fine as a stopping point. Your choice.

I don't think of it as high priority. That might change at some in the future, but for the time being I think that there are bigger fish to fry.

include/lldb/Expression/DynamicCheckerFunctions.h

+//===-- DynamicCheckerFunctions.h -------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef liblldb_DynamicCheckerFunctions_h_
+#define liblldb_DynamicCheckerFunctions_h_
+
+#include "lldb/lldb-types.h"
+
+namespace lldb_private {
+
+class DiagnosticManager;
+class ExecutionContext;
+
+/// \class DynamicCheckerFunctions DynamicCheckerFunctions.h
+/// "lldb/Expression/DynamicCheckerFunctions.h" Encapsulates dynamic check
+/// functions used by expressions.
+///
+/// Each of the utility functions encapsulated in this class is responsible
+/// for validating some data that an expression is about to use.  Examples
+/// are:
+///
+/// a = *b;     // check that b is a valid pointer
+/// [b init];   // check that b is a valid object to send "init" to
+///
+/// The class installs each checker function into the target process and makes
+/// it available to IRDynamicChecks to use.
+class DynamicCheckerFunctions {
+public:
+  enum DynamicCheckerFunctionsKind {
+    DCF_Clang,
+  };
+
+  DynamicCheckerFunctions(DynamicCheckerFunctionsKind kind) : m_kind(kind) {}
+  virtual ~DynamicCheckerFunctions() = default;
+
+  /// Install the utility functions into a process.  This binds the instance
+  /// of DynamicCheckerFunctions to that process.
+  ///
+  /// \param[in] diagnostic_manager
+  ///     A diagnostic manager to report errors to.
+  ///
+  /// \param[in] exe_ctx
+  ///     The execution context to install the functions into.
+  ///
+  /// \return
+  ///     True on success; false on failure, or if the functions have
+  ///     already been installed.
+  virtual bool Install(DiagnosticManager &diagnostic_manager,
+                       ExecutionContext &exe_ctx) = 0;
+  virtual bool DoCheckersExplainStop(lldb::addr_t addr, Stream &message) = 0;
+
+  DynamicCheckerFunctionsKind GetKind() const { return m_kind; }
+
+private:
+  const DynamicCheckerFunctionsKind m_kind;
+};
+} // namespace lldb_private
+
+#endif // liblldb_DynamicCheckerFunctions_h_

source/Expression/CMakeLists.txt

 if(NOT LLDB_BUILT_STANDALONE)
   set(tablegen_deps intrinsics_gen)
 endif()
 
 add_lldb_library(lldbExpression
   DiagnosticManager.cpp
   DWARFExpression.cpp
   Expression.cpp
   ExpressionVariable.cpp
   FunctionCaller.cpp
-  IRDynamicChecks.cpp
   IRExecutionUnit.cpp
   IRInterpreter.cpp
   IRMemoryMap.cpp
   LLVMUserExpression.cpp
   Materializer.cpp
   REPL.cpp
   UserExpression.cpp
   UtilityFunction.cpp

source/Plugins/ExpressionParser/Clang/CMakeLists.txt

add_lldb_library(lldbPluginExpressionParserClang PLUGIN
   ClangExpressionVariable.cpp
   ClangFunctionCaller.cpp
   ClangHost.cpp
   ClangModulesDeclVendor.cpp
   ClangPersistentVariables.cpp
   ClangUserExpression.cpp
   ClangUtilityFunction.cpp
   IRForTarget.cpp
+  IRDynamicChecks.cpp
 
   DEPENDS
   ${tablegen_deps}
 
   LINK_LIBS
     clangAST
     clangCodeGen
     clangDriver

source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

 #include "ClangASTSource.h"
 #include "ClangDiagnostic.h"
 #include "ClangExpressionDeclMap.h"
 #include "ClangExpressionHelper.h"
 #include "ClangExpressionParser.h"
 #include "ClangHost.h"
 #include "ClangModulesDeclVendor.h"
 #include "ClangPersistentVariables.h"
+#include "IRDynamicChecks.h"
 #include "IRForTarget.h"
 #include "ModuleDependencyCollector.h"
 
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Disassembler.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/StreamFile.h"
-#include "lldb/Expression/IRDynamicChecks.h"
 #include "lldb/Expression/IRExecutionUnit.h"
 #include "lldb/Expression/IRInterpreter.h"
 #include "lldb/Host/File.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Symbol/ClangASTContext.h"
 #include "lldb/Symbol/SymbolVendor.h"
 #include "lldb/Target/ExecutionContext.h"
 #include "lldb/Target/Language.h"
if (!process && execution_policy == eExecutionPolicyTopLevel) {
                          "target, but the target can't be run");
       return err;
     }
 
     if (execution_policy == eExecutionPolicyAlways ||
         (execution_policy != eExecutionPolicyTopLevel && !can_interpret)) {
       if (m_expr.NeedsValidation() && process) {
         if (!process->GetDynamicCheckers()) {
-          DynamicCheckerFunctions *dynamic_checkers =
-              new DynamicCheckerFunctions();
+          ClangDynamicCheckerFunctions *dynamic_checkers =
+              new ClangDynamicCheckerFunctions();
 
           DiagnosticManager install_diagnostics;
 
           if (!dynamic_checkers->Install(install_diagnostics, exe_ctx)) {
             if (install_diagnostics.Diagnostics().size())
               err.SetErrorString(install_diagnostics.GetString().c_str());
             else
               err.SetErrorString("couldn't install checkers, unknown error");
 
             return err;
           }
 
           process->SetDynamicCheckers(dynamic_checkers);
 
           if (log)
             log->Printf("== [ClangExpressionParser::PrepareForExecution] "
                         "Finished installing dynamic checkers ==");
         }
 
-        IRDynamicChecks ir_dynamic_checks(*process->GetDynamicCheckers(),
+        if (auto *checker_funcs = llvm::dyn_cast<ClangDynamicCheckerFunctions>(
+                process->GetDynamicCheckers())) {
+          IRDynamicChecks ir_dynamic_checks(*checker_funcs,
                                             function_name.AsCString());
 
           llvm::Module *module = execution_unit_sp->GetModule();
           if (!module || !ir_dynamic_checks.runOnModule(*module)) {
             err.SetErrorToGenericError();
             err.SetErrorString("Couldn't add dynamic checks to the expression");
             return err;
           }
 
           if (custom_passes.LatePasses) {
             if (log)
               log->Printf("%s - Running Late IR Passes from LanguageRuntime on "
                           "expression module '%s'",
                           __FUNCTION__, m_expr.FunctionName());
 
             custom_passes.LatePasses->run(*module);
           }
         }
       }
+    }
 
     if (execution_policy == eExecutionPolicyAlways ||
         execution_policy == eExecutionPolicyTopLevel || !can_interpret) {
       execution_unit_sp->GetRunnableInfo(err, func_addr, func_end);
     }
   } else {
     execution_unit_sp->GetRunnableInfo(err, func_addr, func_end);
   }

source/Plugins/ExpressionParser/Clang/IRDynamicChecks.h

 //===-- IRDynamicChecks.h ---------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef liblldb_IRDynamicChecks_h_
 #define liblldb_IRDynamicChecks_h_
 
+#include "lldb/Expression/DynamicCheckerFunctions.h"
 #include "lldb/lldb-types.h"
 #include "llvm/Pass.h"
 
 namespace llvm {
 class BasicBlock;
-class CallInst;
-class Constant;
-class Function;
-class Instruction;
 class Module;
-class DataLayout;
-class Value;
 }
 
 namespace lldb_private {
 
-class ClangExpressionDeclMap;
 class ExecutionContext;
 class Stream;
 
-/// \class DynamicCheckerFunctions IRDynamicChecks.h
-/// "lldb/Expression/IRDynamicChecks.h" Encapsulates dynamic check functions
-/// used by expressions.
-///
-/// Each of the utility functions encapsulated in this class is responsible
-/// for validating some data that an expression is about to use.  Examples
-/// are:
-///
-/// a = *b;     // check that b is a valid pointer [b init];   // check that b
-/// is a valid object to send "init" to
-///
-/// The class installs each checker function into the target process and makes
-/// it available to IRDynamicChecks to use.
-class DynamicCheckerFunctions {
+class ClangDynamicCheckerFunctions
+    : public lldb_private::DynamicCheckerFunctions {
 public:
   /// Constructor
-  DynamicCheckerFunctions();
+  ClangDynamicCheckerFunctions();
 
   /// Destructor
-  ~DynamicCheckerFunctions();
+  virtual ~ClangDynamicCheckerFunctions();
+
+  static bool classof(const DynamicCheckerFunctions *checker_funcs) {
+    return checker_funcs->GetKind() == DCF_Clang;
+  }
 
   /// Install the utility functions into a process.  This binds the instance
   /// of DynamicCheckerFunctions to that process.
   ///
   /// \param[in] diagnostic_manager
   ///     A diagnostic manager to report errors to.
   ///
   /// \param[in] exe_ctx
   ///     The execution context to install the functions into.
   ///
   /// \return
   ///     True on success; false on failure, or if the functions have
   ///     already been installed.
   bool Install(DiagnosticManager &diagnostic_manager,
-               ExecutionContext &exe_ctx);
+               ExecutionContext &exe_ctx) override;
 
-  bool DoCheckersExplainStop(lldb::addr_t addr, Stream &message);
+  bool DoCheckersExplainStop(lldb::addr_t addr, Stream &message) override;
 
   std::shared_ptr<UtilityFunction> m_valid_pointer_check;
   std::shared_ptr<UtilityFunction> m_objc_object_check;
 };
 
 /// \class IRDynamicChecks IRDynamicChecks.h
 /// "lldb/Expression/IRDynamicChecks.h" Adds dynamic checks to a user-entered
 /// expression to reduce its likelihood of crashing
public:
   ///     The checker functions for the target process.
   ///
   /// \param[in] func_name
   ///     The name of the function to prepare for execution in the target.
   ///
   /// \param[in] decl_map
   ///     The mapping used to look up entities in the target process. In
   ///     this case, used to find objc_msgSend
-  IRDynamicChecks(DynamicCheckerFunctions &checker_functions,
+  IRDynamicChecks(ClangDynamicCheckerFunctions &checker_functions,
                   const char *func_name = "$__lldb_expr");
 
   /// Destructor
   ~IRDynamicChecks() override;
 
   /// Run this IR transformer on a single module
   ///
   /// \param[in] M
private:
   /// \param[in] BB
   ///     The basic block currently being processed.
   ///
   /// \return
   ///     True on success; false otherwise
   bool FindDataLoads(llvm::Module &M, llvm::BasicBlock &BB);
 
   std::string m_func_name; ///< The name of the function to add checks to
-  DynamicCheckerFunctions
+  ClangDynamicCheckerFunctions
       &m_checker_functions; ///< The checker functions for the process
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_IRDynamicChecks_h_

source/Plugins/ExpressionParser/Clang/IRDynamicChecks.cpp

 //===-- IRDynamicChecks.cpp -------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Value.h"
 #include "llvm/Support/raw_ostream.h"
 
-#include "lldb/Expression/IRDynamicChecks.h"
+#include "IRDynamicChecks.h"
 
 #include "lldb/Expression/UtilityFunction.h"
 #include "lldb/Target/ExecutionContext.h"
 #include "lldb/Target/ObjCLanguageRuntime.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/StackFrame.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/ConstString.h"

 
 static const char g_valid_pointer_check_text[] =
     "extern \"C\" void\n"
     "_$__lldb_valid_pointer_check (unsigned char *$__lldb_arg_ptr)\n"
     "{\n"
     "    unsigned char $__lldb_local_val = *$__lldb_arg_ptr;\n"
     "}";
 
-DynamicCheckerFunctions::DynamicCheckerFunctions() = default;
+ClangDynamicCheckerFunctions::ClangDynamicCheckerFunctions()
+    : DynamicCheckerFunctions(DCF_Clang) {}
 
-DynamicCheckerFunctions::~DynamicCheckerFunctions() = default;
+ClangDynamicCheckerFunctions::~ClangDynamicCheckerFunctions() = default;
 
-bool DynamicCheckerFunctions::Install(DiagnosticManager &diagnostic_manager,
-                                      ExecutionContext &exe_ctx) {
+bool ClangDynamicCheckerFunctions::Install(
+    DiagnosticManager &diagnostic_manager, ExecutionContext &exe_ctx) {
   Status error;
   m_valid_pointer_check.reset(
       exe_ctx.GetTargetRef().GetUtilityFunctionForLanguage(
           g_valid_pointer_check_text, lldb::eLanguageTypeC,
           VALID_POINTER_CHECK_NAME, error));
   if (error.Fail())
     return false;
 
if (objc_language_runtime) {
       if (!m_objc_object_check->Install(diagnostic_manager, exe_ctx))
         return false;
     }
   }
 
   return true;
 }
 
-bool DynamicCheckerFunctions::DoCheckersExplainStop(lldb::addr_t addr,
+bool ClangDynamicCheckerFunctions::DoCheckersExplainStop(lldb::addr_t addr,
                                                          Stream &message) {
   // FIXME: We have to get the checkers to know why they scotched the call in
   // more detail,
   // so we can print a better message here.
   if (m_valid_pointer_check && m_valid_pointer_check->ContainsAddress(addr)) {
     message.Printf("Attempted to dereference an invalid pointer.");
     return true;
   } else if (m_objc_object_check &&
              m_objc_object_check->ContainsAddress(addr)) {
bool InspectInstruction(llvm::Instruction &i) override {
 
     return true;
   }
 
 private:
   llvm::FunctionCallee m_objc_object_check_func;
 };
 
-IRDynamicChecks::IRDynamicChecks(DynamicCheckerFunctions &checker_functions,
-                                 const char *func_name)
+IRDynamicChecks::IRDynamicChecks(
+    ClangDynamicCheckerFunctions &checker_functions, const char *func_name)
     : ModulePass(ID), m_func_name(func_name),
       m_checker_functions(checker_functions) {}
 
 IRDynamicChecks::~IRDynamicChecks() = default;
 
 bool IRDynamicChecks::runOnModule(llvm::Module &M) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
 

source/Target/Process.cpp

 #include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Breakpoint/StoppointCallbackContext.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/StreamFile.h"
 #include "lldb/Expression/DiagnosticManager.h"
-#include "lldb/Expression/IRDynamicChecks.h"
+#include "lldb/Expression/DynamicCheckerFunctions.h"
 #include "lldb/Expression/UserExpression.h"
 #include "lldb/Expression/UtilityFunction.h"
 #include "lldb/Host/ConnectionFileDescriptor.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Host/OptionParser.h"
 #include "lldb/Host/Pipe.h"

source/Target/ThreadPlanCallUserExpression.cpp

 //===-- ThreadPlanCallUserExpression.cpp -------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Target/ThreadPlanCallUserExpression.h"
 
 
 #include "lldb/Breakpoint/Breakpoint.h"
 #include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Core/Address.h"
 #include "lldb/Expression/DiagnosticManager.h"
-#include "lldb/Expression/IRDynamicChecks.h"
+#include "lldb/Expression/DynamicCheckerFunctions.h"
 #include "lldb/Expression/UserExpression.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Target/LanguageRuntime.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/RegisterContext.h"
 #include "lldb/Target/StopInfo.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/Thread.h"