[lldb][NFC] Make ClangExpressionSourceCode's wrapping logic more consistent

[teemperor]

Summary

ClangExpressionSourceCode has different ways to wrap the user expression based on
which context the expression is executed in. For example, if we're in a C++ member
function we put the expression inside a fake member function of a fake class to make the
evaluation possible. Similar things are done for Objective-C instance/static methods.
There is also a default wrapping where we put the expression in a normal function
just to make it possible to execute it.

The way we currently define which kind of wrapping the expression needs is based on
the wrapping_language we keep passing to the ClangExpressionSourceCode
instance. We repurposed the language type enum for that variable to distinguish the
cases above with the following mapping:

• language = C_plus_plus -> member function wrapping
• language = ObjC -> instance/static method wrapping (is_static distinguished between those two).
• language = C -> normal function wrapping
• all other cases like C_plus_plus11, Haskell etc. make our class a no-op that does mostly nothing.

That mapping is currently not documented and just confusing as the language
is unrelated to the expression language (and in the ClangUserExpression we even pretend
that it *is* the actual language, but luckily never used it for anything). Some of the code
in ClangExpressionSourceCode is also obviously thinking that this is the actual language of
the expression as it checks for non-existent cases such as ObjC_plus_plus which is
not part of the mapping.

This patch makes a new enum to describe the four cases above (with instance/static Objective-C
methods now being their own case). It also make that enum just a member of
ClangExpressionSourceCode instead of having to pass the same value to the class repeatedly.
This gets also rid of all the switch-case-checks for 'unknown' language such as C_plus_plus11 as this
is no longer necessary.

[labath]

Makes sense to me.

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h

33–40

If I understand the code correctly, a c++ static member function is wrapped using the "function" approach. I think this is potentially confusing, so it would be good if the comments elaborated on it more (e.g. add "non-static" to the CppMemberFunction description, and explicitly mention static member functions in the "function" description).

[teemperor]

• Clarified that 'Function' is also used for static member functions.

[shafik]

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h

33–40

It might be worth adding that we don't need to find the this for the C++ case unlike a non-static member function. This happens in LookUpLldbClass

[shafik]

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h

41

I would actually feel better have a separate enumerator for C++ static member functions and just having it fall-through when used. It would be self-documenting.

[labath]

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h

41

If it doesn't complicate the callers (i.e., they always know, or can easily find out, whether they are dealing with a static member or a non-member), I think that would be great.

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h

 
 class ClangExpressionSourceCode : public ExpressionSourceCode {
 public:
   /// The file name we use for the wrapper code that we inject before
   /// the user expression.
   static const llvm::StringRef g_prefix_file_name;
   static const char *g_expression_prefix;
 
+  /// The possible ways an expression can be wrapped.
+  enum class WrapKind {
+    /// Wrapped in a non-static member function of a C++ class.
+    CppMemberFunction,
+    /// Wrapped in an instance Objective-C method.
+    ObjCInstanceMethod,
+    /// Wrapped in a static Objective-C method.
+    ObjCStaticMethod,
+    /// Wrapped in a non-member function.
+    /// Note that this is also used for static member functions of a C++ class.
+    Function
+  };
+
   static ClangExpressionSourceCode *CreateWrapped(llvm::StringRef filename,
                                                   llvm::StringRef prefix,
-                                                  llvm::StringRef body) {
+                                                  llvm::StringRef body,
+                                                  WrapKind wrap_kind) {
     return new ClangExpressionSourceCode(filename, "$__lldb_expr", prefix, body,
-                                         Wrap);
+                                         Wrap, wrap_kind);
   }
 
   /// Generates the source code that will evaluate the expression.
   ///
   /// \param text output parameter containing the source code string.
-  /// \param wrapping_language If the expression is supossed to be wrapped,
-  ///        then this is the language that should be used for that.
-  /// \param static_method True iff the expression is valuated inside a static
-  ///        Objective-C method.
   /// \param exe_ctx The execution context in which the expression will be
   ///        evaluated.
   /// \param add_locals True iff local variables should be injected into the
   ///        expression source code.
   /// \param force_add_all_locals True iff all local variables should be
   ///        injected even if they are not used in the expression.
   /// \param modules A list of (C++) modules that the expression should import.
   ///
   /// \return true iff the source code was successfully generated.
-  bool GetText(std::string &text, lldb::LanguageType wrapping_language,
-               bool static_method, ExecutionContext &exe_ctx, bool add_locals,
+  bool GetText(std::string &text, ExecutionContext &exe_ctx, bool add_locals,
                bool force_add_all_locals,
                llvm::ArrayRef<std::string> modules) const;
 
   // Given a string returned by GetText, find the beginning and end of the body
   // passed to CreateWrapped. Return true if the bounds could be found.  This
   // will also work on text with FixItHints applied.
   bool GetOriginalBodyBounds(std::string transformed_text,
-                             lldb::LanguageType wrapping_language,
                              size_t &start_loc, size_t &end_loc);
 
 protected:
   ClangExpressionSourceCode(llvm::StringRef filename, llvm::StringRef name,
                             llvm::StringRef prefix, llvm::StringRef body,
-                            Wrapping wrap);
+                            Wrapping wrap, WrapKind wrap_kind);
 
 private:
+  void AddLocalVariableDecls(const lldb::VariableListSP &var_list_sp,
+                             StreamString &stream,
+                             const std::string &expr) const;
+
   /// String marking the start of the user expression.
   std::string m_start_marker;
   /// String marking the end of the user expression.
   std::string m_end_marker;
+  /// How the expression has been wrapped.
+  const WrapKind m_wrap_kind;
 };
 
 } // namespace lldb_private
 
 #endif

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp

default:
       // This is an unknown/invalid entry. Ignore.
       break;
     }
   }
 }
 
 lldb_private::ClangExpressionSourceCode::ClangExpressionSourceCode(
     llvm::StringRef filename, llvm::StringRef name, llvm::StringRef prefix,
-    llvm::StringRef body, Wrapping wrap)
-    : ExpressionSourceCode(name, prefix, body, wrap) {
+    llvm::StringRef body, Wrapping wrap, WrapKind wrap_kind)
+    : ExpressionSourceCode(name, prefix, body, wrap), m_wrap_kind(wrap_kind) {
   // Use #line markers to pretend that we have a single-line source file
   // containing only the user expression. This will hide our wrapper code
   // from the user when we render diagnostics with Clang.
   m_start_marker = "#line 1 \"" + filename.str() + "\"\n";
   m_end_marker = "\n;\n#line 1 \"<lldb wrapper suffix>\"\n";
 }
 
 namespace {
while (!exit) {
     // Extract the token string from our source code and store it.
     std::string token_str = body.substr(start, token.getLength());
     if (token_str.empty())
       continue;
     m_tokens.insert(token_str);
   }
 }
 
-static void AddLocalVariableDecls(const lldb::VariableListSP &var_list_sp,
-                                  StreamString &stream,
-                                  const std::string &expr,
-                                  lldb::LanguageType wrapping_language) {
+void ClangExpressionSourceCode::AddLocalVariableDecls(
+    const lldb::VariableListSP &var_list_sp, StreamString &stream,
+    const std::string &expr) const {
   TokenVerifier tokens(expr);
 
   for (size_t i = 0; i < var_list_sp->GetSize(); i++) {
     lldb::VariableSP var_sp = var_list_sp->GetVariableAtIndex(i);
 
     ConstString var_name = var_sp->GetName();
 
 
     // We can check for .block_descriptor w/o checking for langauge since this
     // is not a valid identifier in either C or C++.
     if (!var_name || var_name == ".block_descriptor")
       continue;
 
     if (!expr.empty() && !tokens.hasToken(var_name.GetStringRef()))
       continue;
 
-    if ((var_name == "self" || var_name == "_cmd") &&
-        (wrapping_language == lldb::eLanguageTypeObjC ||
-         wrapping_language == lldb::eLanguageTypeObjC_plus_plus))
+    const bool is_objc = m_wrap_kind == WrapKind::ObjCInstanceMethod ||
+                         m_wrap_kind == WrapKind::ObjCStaticMethod;
+    if ((var_name == "self" || var_name == "_cmd") && is_objc)
       continue;
 
-    if (var_name == "this" &&
-        wrapping_language == lldb::eLanguageTypeC_plus_plus)
+    if (var_name == "this" && m_wrap_kind == WrapKind::CppMemberFunction)
       continue;
 
     stream.Printf("using $__lldb_local_vars::%s;\n", var_name.AsCString());
   }
 }
 
 bool ClangExpressionSourceCode::GetText(
-    std::string &text, lldb::LanguageType wrapping_language, bool static_method,
-    ExecutionContext &exe_ctx, bool add_locals, bool force_add_all_locals,
-    llvm::ArrayRef<std::string> modules) const {
+    std::string &text, ExecutionContext &exe_ctx, bool add_locals,
+    bool force_add_all_locals, llvm::ArrayRef<std::string> modules) const {
   const char *target_specific_defines = "typedef signed char BOOL;\n";
   std::string module_macros;
 
   Target *target = exe_ctx.GetTargetPtr();
   if (target) {
     if (target->GetArchitecture().GetMachine() == llvm::Triple::aarch64 ||
         target->GetArchitecture().GetMachine() == llvm::Triple::aarch64_32) {
       target_specific_defines = "typedef bool BOOL;\n";
if (sc.comp_unit && sc.line_entry.IsValid()) {
       }
     }
 
     if (add_locals)
       if (target->GetInjectLocalVariables(&exe_ctx)) {
         lldb::VariableListSP var_list_sp =
             frame->GetInScopeVariableList(false, true);
         AddLocalVariableDecls(var_list_sp, lldb_local_var_decls,
-                              force_add_all_locals ? "" : m_body,
-                              wrapping_language);
+                              force_add_all_locals ? "" : m_body);
       }
   }
 
   if (m_wrap) {
-    switch (wrapping_language) {
-    default:
-      return false;
-    case lldb::eLanguageTypeC:
-    case lldb::eLanguageTypeC_plus_plus:
-    case lldb::eLanguageTypeObjC:
-      break;
-    }
-
     // Generate a list of @import statements that will import the specified
     // module into our expression.
     std::string module_imports;
     for (const std::string &module : modules) {
       module_imports.append("@import ");
       module_imports.append(module);
       module_imports.append(";\n");
     }
 
     StreamString wrap_stream;
 
     wrap_stream.Printf("%s\n%s\n%s\n%s\n%s\n", module_macros.c_str(),
                        debug_macros_stream.GetData(), g_expression_prefix,
                        target_specific_defines, m_prefix.c_str());
 
     // First construct a tagged form of the user expression so we can find it
     // later:
     std::string tagged_body;
-    switch (wrapping_language) {
-    default:
-      tagged_body = m_body;
-      break;
-    case lldb::eLanguageTypeC:
-    case lldb::eLanguageTypeC_plus_plus:
-    case lldb::eLanguageTypeObjC:
-      tagged_body.append(m_start_marker);
-      tagged_body.append(m_body);
-      tagged_body.append(m_end_marker);
-      break;
-    }
-    switch (wrapping_language) {
-    default:
-      break;
-    case lldb::eLanguageTypeC:
+
+    switch (m_wrap_kind) {
+    case WrapKind::Function:
       wrap_stream.Printf("%s"
                          "void                           \n"
                          "%s(void *$__lldb_arg)          \n"
                          "{                              \n"
                          "    %s;                        \n"
                          "%s"
                          "}                              \n",
                          module_imports.c_str(), m_name.c_str(),
                          lldb_local_var_decls.GetData(), tagged_body.c_str());
       break;
-    case lldb::eLanguageTypeC_plus_plus:
+    case WrapKind::CppMemberFunction:
       wrap_stream.Printf("%s"
                          "void                                   \n"
                          "$__lldb_class::%s(void *$__lldb_arg)   \n"
                          "{                                      \n"
                          "    %s;                                \n"
                          "%s"
                          "}                                      \n",
                          module_imports.c_str(), m_name.c_str(),
                          lldb_local_var_decls.GetData(), tagged_body.c_str());
       break;
-    case lldb::eLanguageTypeObjC:
-      if (static_method) {
-        wrap_stream.Printf(
-            "%s"
-            "@interface $__lldb_objc_class ($__lldb_category)        \n"
-            "+(void)%s:(void *)$__lldb_arg;                          \n"
-            "@end                                                    \n"
-            "@implementation $__lldb_objc_class ($__lldb_category)   \n"
-            "+(void)%s:(void *)$__lldb_arg                           \n"
-            "{                                                       \n"
-            "    %s;                                                 \n"
-            "%s"
-            "}                                                       \n"
-            "@end                                                    \n",
-            module_imports.c_str(), m_name.c_str(), m_name.c_str(),
-            lldb_local_var_decls.GetData(), tagged_body.c_str());
-      } else {
-        wrap_stream.Printf(
-            "%s"
-            "@interface $__lldb_objc_class ($__lldb_category)       \n"
-            "-(void)%s:(void *)$__lldb_arg;                         \n"
-            "@end                                                   \n"
-            "@implementation $__lldb_objc_class ($__lldb_category)  \n"
-            "-(void)%s:(void *)$__lldb_arg                          \n"
-            "{                                                      \n"
-            "    %s;                                                \n"
-            "%s"
-            "}                                                      \n"
-            "@end                                                   \n",
-            module_imports.c_str(), m_name.c_str(), m_name.c_str(),
-            lldb_local_var_decls.GetData(), tagged_body.c_str());
-      }
+    case WrapKind::ObjCInstanceMethod:
+          "@interface $__lldb_objc_class ($__lldb_category)       \n"
+          "-(void)%s:(void *)$__lldb_arg;                         \n"
+          "@end                                                   \n"
+          "@implementation $__lldb_objc_class ($__lldb_category)  \n"
+          "-(void)%s:(void *)$__lldb_arg                          \n"
+          "{                                                      \n"
+          "    %s;                                                \n"
+          "}                                                      \n"
+          "@end                                                   \n",
+      break;
+
+    case WrapKind::ObjCStaticMethod:
+          "@interface $__lldb_objc_class ($__lldb_category)        \n"
+          "+(void)%s:(void *)$__lldb_arg;                          \n"
+          "@end                                                    \n"
+          "@implementation $__lldb_objc_class ($__lldb_category)   \n"
+          "+(void)%s:(void *)$__lldb_arg                           \n"
+          "{                                                       \n"
+          "    %s;                                                 \n"
+          "}                                                       \n"
+          "@end                                                    \n",
       break;
     }
 
     text = std::string(wrap_stream.GetString());
   } else {
     text.append(m_body);
   }
 
   return true;
 }
 
 bool ClangExpressionSourceCode::GetOriginalBodyBounds(
-    std::string transformed_text, lldb::LanguageType wrapping_language,
-    size_t &start_loc, size_t &end_loc) {
-  switch (wrapping_language) {
-  default:
-    return false;
-  case lldb::eLanguageTypeC:
-  case lldb::eLanguageTypeC_plus_plus:
-  case lldb::eLanguageTypeObjC:
-    break;
-  }
-
+    std::string transformed_text, size_t &start_loc, size_t &end_loc) {
   start_loc = transformed_text.find(m_start_marker);
   if (start_loc == std::string::npos)
     return false;
   start_loc += m_start_marker.size();
   end_loc = transformed_text.find(m_end_marker);
   return end_loc != std::string::npos;
 }

lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h

private:
   bool AddArguments(ExecutionContext &exe_ctx, std::vector<lldb::addr_t> &args,
                     lldb::addr_t struct_address,
                     DiagnosticManager &diagnostic_manager) override;
 
   void CreateSourceCode(DiagnosticManager &diagnostic_manager,
                         ExecutionContext &exe_ctx,
                         std::vector<std::string> modules_to_import,
                         bool for_completion);
-  void UpdateLanguageForExpr();
+  /// Defines how the current expression should be wrapped.
+  ClangExpressionSourceCode::WrapKind GetWrapKind() const;
   bool SetupPersistentState(DiagnosticManager &diagnostic_manager,
                                    ExecutionContext &exe_ctx);
   bool PrepareForParsing(DiagnosticManager &diagnostic_manager,
                          ExecutionContext &exe_ctx, bool for_completion);
 
   ClangUserExpressionHelper m_type_system_helper;
 
   class ResultDelegate : public Materializer::PersistentVariableDelegate {
   public:
     ResultDelegate(lldb::TargetSP target) : m_target_sp(target) {}
     ConstString GetName() override;
     void DidDematerialize(lldb::ExpressionVariableSP &variable) override;
 
     void RegisterPersistentState(PersistentExpressionState *persistent_state);
     lldb::ExpressionVariableSP &GetVariable();
 
   private:
     PersistentExpressionState *m_persistent_state;
     lldb::ExpressionVariableSP m_variable;
     lldb::TargetSP m_target_sp;
   };
 
-  /// The language type of the current expression.
-  lldb::LanguageType m_expr_lang = lldb::eLanguageTypeUnknown;
   /// The include directories that should be used when parsing the expression.
   std::vector<std::string> m_include_directories;
 
   /// The absolute character position in the transformed source code where the
   /// user code (as typed by the user) starts. If the variable is empty, then we
   /// were not able to calculate this position.
   llvm::Optional<size_t> m_user_expression_start_pos;
   ResultDelegate m_result_delegate;

lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp

if (!error_stream.Empty()) {
     return;
   }
 
   diagnostic_manager.PutString(eDiagnosticSeverityError,
                                "Unknown error while loading modules needed for "
                                "current compilation unit.");
 }
 
-void ClangUserExpression::UpdateLanguageForExpr() {
-  m_expr_lang = lldb::LanguageType::eLanguageTypeUnknown;
-  if (m_options.GetExecutionPolicy() == eExecutionPolicyTopLevel)
-    return;
+ClangExpressionSourceCode::WrapKind ClangUserExpression::GetWrapKind() const {
+  assert(m_options.GetExecutionPolicy() != eExecutionPolicyTopLevel &&
+         "Top level expressions aren't wrapped.");
+  using Kind = ClangExpressionSourceCode::WrapKind;
   if (m_in_cplusplus_method)
-    m_expr_lang = lldb::eLanguageTypeC_plus_plus;
-  else if (m_in_objectivec_method)
-    m_expr_lang = lldb::eLanguageTypeObjC;
-  else
-    m_expr_lang = lldb::eLanguageTypeC;
+    return Kind::CppMemberFunction;
+  else if (m_in_objectivec_method) {
+    if (m_in_static_method)
+      return Kind::ObjCStaticMethod;
+    return Kind::ObjCInstanceMethod;
+  }
+  // Not in any kind of 'special' function, so just wrap it in a normal C
+  // function.
+  return Kind::Function;
 }
 
 void ClangUserExpression::CreateSourceCode(
     DiagnosticManager &diagnostic_manager, ExecutionContext &exe_ctx,
     std::vector<std::string> modules_to_import, bool for_completion) {
 
   m_filename = m_clang_state->GetNextExprFileName();
   std::string prefix = m_expr_prefix;
 
   if (m_options.GetExecutionPolicy() == eExecutionPolicyTopLevel) {
     m_transformed_text = m_expr_text;
   } else {
     m_source_code.reset(ClangExpressionSourceCode::CreateWrapped(
-        m_filename, prefix, m_expr_text));
+        m_filename, prefix, m_expr_text, GetWrapKind()));
 
-    if (!m_source_code->GetText(m_transformed_text, m_expr_lang,
-                                m_in_static_method, exe_ctx, !m_ctx_obj,
+    if (!m_source_code->GetText(m_transformed_text, exe_ctx, !m_ctx_obj,
                                 for_completion, modules_to_import)) {
       diagnostic_manager.PutString(eDiagnosticSeverityError,
                                    "couldn't construct expression body");
       return;
     }
 
     // Find and store the start position of the original code inside the
     // transformed code. We need this later for the code completion.
     std::size_t original_start;
     std::size_t original_end;
     bool found_bounds = m_source_code->GetOriginalBodyBounds(
-        m_transformed_text, m_expr_lang, original_start, original_end);
+        m_transformed_text, original_start, original_end);
     if (found_bounds)
       m_user_expression_start_pos = original_start;
   }
 }
 
 static bool SupportsCxxModuleImport(lldb::LanguageType language) {
   switch (language) {
   case lldb::eLanguageTypeC_plus_plus:
bool ClangUserExpression::PrepareForParsing(
   m_include_directories = module_config.GetIncludeDirs();
 
   LLDB_LOG(log, "List of imported modules in expression: {0}",
            llvm::make_range(imported_modules.begin(), imported_modules.end()));
   LLDB_LOG(log, "List of include directories gathered for modules: {0}",
            llvm::make_range(m_include_directories.begin(),
                             m_include_directories.end()));
 
-  UpdateLanguageForExpr();
   CreateSourceCode(diagnostic_manager, exe_ctx, imported_modules,
                    for_completion);
   return true;
 }
 
 bool ClangUserExpression::Parse(DiagnosticManager &diagnostic_manager,
                                 ExecutionContext &exe_ctx,
                                 lldb_private::ExecutionPolicy execution_policy,
bool ClangUserExpression::Parse(DiagnosticManager &diagnostic_manager,
   if (num_errors) {
     if (diagnostic_manager.HasFixIts()) {
       if (parser.RewriteExpression(diagnostic_manager)) {
         size_t fixed_start;
         size_t fixed_end;
         m_fixed_text = diagnostic_manager.GetFixedExpression();
         // Retrieve the original expression in case we don't have a top level
         // expression (which has no surrounding source code).
-        if (m_source_code &&
-            m_source_code->GetOriginalBodyBounds(m_fixed_text, m_expr_lang,
-                                                 fixed_start, fixed_end))
+        if (m_source_code && m_source_code->GetOriginalBodyBounds(
+                                 m_fixed_text, fixed_start, fixed_end))
           m_fixed_text =
               m_fixed_text.substr(fixed_start, fixed_end - fixed_start);
       }
     }
     return false;
   }
 
   //////////////////////////////////////////////////////////////////////////////