Inject only relevant local variables in the expression evaluation context

[teemperor]

Summary

In r259902, LLDB started injecting all the locals in every expression
evaluation. This fixed a bunch of issues, but also caused others, mostly
performance regressions on some codebases. The regressions were bad
enough that we added a setting in r274783 to control the behavior and
we have been shipping with the setting off to avoid the perf regressions.

This patch changes the logic injecting the local variables to only inject
the ones present in the expression typed by the user. The approach is
fairly simple and just scans the typed expression for every local name.
Hopefully this gives us the best of both world as it just realizes the
types of the variables really used by the expression.

Landing this requires the 2 other issues I pointed out today to be addressed
but I wanted to gather comments right away.

Original patch by Frédéric Riss!

[clayborg]

source/Expression/ExpressionSourceCode.cpp

174–179 ↗

(On Diff #145553)

Might be clearer as:

const int prev = from-1;
if (prev >= 0 && clang::isIdentifierBody(body[prev]))
  continue;
const int next = from + var.size()
if (next == body.size() || clang::isIdentifierBody(body[next]))
  continue;

[clayborg]

I forgot to increment from before continuing in my example. No need to update code if you don't feel it is clearer

[jingham]

I'm still a little sad we can't get this to happen correctly in clang's lookup, but this is a clever way to get the benefit of this workaround without paying all the cost, and is a fine temporary solution.

The implementation looks okay to me either way.

[labath]

One could probably concoct an example (using macros, token pasting and such) where this would get it wrong, but that's probably not worth supporting.

source/Expression/ExpressionSourceCode.cpp

193 ↗

(On Diff #145553)

s/AsCString/GetStringRef (saves a strlen operation).

[teemperor]

Shafik asked me to take a look at this and see if I can fix the completion tests.

[teemperor]

• Disable the variable filtering when during tab completion for now, as it seems our Clang lookup doesn't pull in external types which breaks the completion tests (and others too). This is only a temporary fix to unblock D59960.
• Some minor cleanup and documentation.

packages/Python/lldbsuite/test/lang/cpp/member-and-local-vars-with-same-name/TestMembersAndLocalsWithSameName.py

def test_when_stopped_in_function(self):
             self.process.GetState() == lldb.eStateStopped,
             PROCESS_STOPPED)
         thread = lldbutil.get_stopped_thread(
             self.process, lldb.eStopReasonBreakpoint)
         self.assertTrue(thread.IsValid())
         frame = thread.GetSelectedFrame()
         self.assertTrue(frame.IsValid())
 
+        self.enable_expression_log()
         val = frame.EvaluateExpression("a")
+        self.disable_expression_log_and_check_for_locals(['a'])
         self.assertTrue(val.IsValid())
         self.assertEqual(val.GetValueAsUnsigned(), 12345)
 
         val = frame.EvaluateExpression("b")
         self.assertTrue(val.IsValid())
         self.assertEqual(val.GetValueAsUnsigned(), 54321)
 
         val = frame.EvaluateExpression("c")
def test_when_stopped_in_function(self):
         val = frame.EvaluateExpression("b")
         self.assertTrue(val.IsValid())
         self.assertEqual(val.GetValueAsUnsigned(), 10002)
 
         val = frame.EvaluateExpression("c")
         self.assertTrue(val.IsValid())
         self.assertEqual(val.GetValueAsUnsigned(), 10003)
 
+        self.enable_expression_log()
+        val = frame.EvaluateExpression("c-b")
+        self.disable_expression_log_and_check_for_locals(['c','b'])
+        self.assertTrue(val.IsValid())
+        self.assertEqual(val.GetValueAsUnsigned(), 1)
+
         self.process.Continue()
         self.assertTrue(
             self.process.GetState() == lldb.eStateStopped,
             PROCESS_STOPPED)
         thread = lldbutil.get_stopped_thread(
             self.process, lldb.eStopReasonBreakpoint)
         self.assertTrue(thread.IsValid())
         frame = thread.GetSelectedFrame()
         self.assertTrue(frame.IsValid())
 
         val = frame.EvaluateExpression("a")
         self.assertTrue(val.IsValid())
         self.assertEqual(val.GetValueAsUnsigned(), 1)
 
         val = frame.EvaluateExpression("b")
         self.assertTrue(val.IsValid())
         self.assertEqual(val.GetValueAsUnsigned(), 2)
 
         val = frame.EvaluateExpression("c")
         self.assertTrue(val.IsValid())
         self.assertEqual(val.GetValueAsUnsigned(), 778899)
 
+        self.enable_expression_log()
+        val = frame.EvaluateExpression("a+b")
+        self.disable_expression_log_and_check_for_locals(['a','b'])
+        self.assertTrue(val.IsValid())
+        self.assertEqual(val.GetValueAsUnsigned(), 3)
+
+
     def _load_exe(self):
         self.build()
 
         cwd = os.getcwd()
 
         src_file = os.path.join(cwd, "main.cpp")
         self.src_file_spec = lldb.SBFileSpec(src_file)
         self.assertTrue(self.src_file_spec.IsValid(), "breakpoint file")
 
         # Get the path of the executable
         exe_path = self.getBuildArtifact("a.out")
 
         # Load the executable
         self.target = self.dbg.CreateTarget(exe_path)
         self.assertTrue(self.target.IsValid(), VALID_TARGET)
 
     def _test_globals(self):
         thread = lldbutil.get_stopped_thread(
             self.process, lldb.eStopReasonBreakpoint)
         self.assertTrue(thread.IsValid())
         frame = thread.GetSelectedFrame()
         self.assertTrue(frame.IsValid())
 
+        self.enable_expression_log()
         val = frame.EvaluateExpression("a")
+        self.disable_expression_log_and_check_for_locals([])
         self.assertTrue(val.IsValid())
         self.assertEqual(val.GetValueAsUnsigned(), 112233)
 
         val = frame.EvaluateExpression("b")
         self.assertTrue(val.IsValid())
         self.assertEqual(val.GetValueAsUnsigned(), 445566)
 
         val = frame.EvaluateExpression("c")
         self.assertTrue(val.IsValid())
         self.assertEqual(val.GetValueAsUnsigned(), 778899)
+
+    def enable_expression_log(self):
+        log_file = os.path.join(self.getBuildDir(), "expr.log")
+        self.runCmd("log enable  -f '%s' lldb expr" % (log_file))
+
+    def disable_expression_log_and_check_for_locals(self, variables):
+        log_file = os.path.join(self.getBuildDir(), "expr.log")
+        self.runCmd("log disable lldb expr")
+        local_var_regex = re.compile(r".*__lldb_local_vars::(.*);")
+        matched = []
+        with open(log_file, 'r') as log:
+            for line in log:
+                if line.find('LLDB_BODY_START') != -1:
+                    break
+                m = re.match(local_var_regex, line)
+                if m:
+                    self.assertIn(m.group(1), variables)
+                    matched.append(m.group(1))
+        self.assertEqual([item for item in variables if item not in matched],
+                         [])

source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h

public:
   /// \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 static_method, 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.
   static bool GetOriginalBodyBounds(std::string transformed_text,
                                     lldb::LanguageType wrapping_language,
                                     size_t &start_loc, size_t &end_loc);

source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp

 //===-- ClangExpressionSourceCode.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 "ClangExpressionSourceCode.h"
 
+#include "clang/Basic/CharInfo.h"
+#include "llvm/ADT/StringRef.h"
+
 #include "Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.h"
 #include "Plugins/ExpressionParser/Clang/ClangPersistentVariables.h"
 #include "lldb/Symbol/Block.h"
 #include "lldb/Symbol/CompileUnit.h"
 #include "lldb/Symbol/DebugMacros.h"
 #include "lldb/Symbol/TypeSystem.h"
 #include "lldb/Symbol/VariableList.h"
 #include "lldb/Target/ExecutionContext.h"
case DebugMacroEntry::INDIRECT:
       break;
     default:
       // This is an unknown/invalid entry. Ignore.
       break;
     }
   }
 }
 
+/// Checks if the expression body contains the given variable as a token.
+/// \param body The expression body.
+/// \param var The variable token we are looking for.
+/// \return True iff the expression body containes the variable as a token.
+static bool ExprBodyContainsVar(llvm::StringRef body, llvm::StringRef var) {
+  assert(var.find_if([](char c) { return !clang::isIdentifierBody(c); }) ==
+             llvm::StringRef::npos &&
+         "variable contains non-identifier chars?");
+
+  size_t start = 0;
+  // Iterate over all occurences of the variable string in our expression.
+  while ((start = body.find(var, start)) != llvm::StringRef::npos) {
+    // We found our variable name in the expression. Check that the token
+    // that contains our needle is equal to our variable and not just contains
+    // the character sequence by accident.
+    // Prevents situations where we for example inlcude the variable 'FOO' in an
+    // expression like 'FOObar + 1'.
+    bool has_characters_before =
+        start != 0 && clang::isIdentifierBody(body[start - 1]);
+    bool has_characters_after =
+        start + var.size() < body.size() &&
+        clang::isIdentifierBody(body[start + var.size()]);
+
+    // Our token just contained the variable name as a substring. Continue
+    // searching the rest of the expression.
+    if (has_characters_before || has_characters_after) {
+      ++start;
+      continue;
+    }
+    return true;
+  }
+  return false;
+}
+
 static void AddLocalVariableDecls(const lldb::VariableListSP &var_list_sp,
-                                  StreamString &stream) {
+                                  StreamString &stream,
+                                  const std::string &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();
     if (!var_name || var_name == "this" || var_name == ".block_descriptor")
       continue;
 
+    if (!expr.empty() && !ExprBodyContainsVar(expr, var_name.GetStringRef()))
+      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,
-                                   llvm::ArrayRef<std::string> modules) const {
+bool ClangExpressionSourceCode::GetText(
+    std::string &text, lldb::LanguageType wrapping_language, bool static_method,
+    ExecutionContext &exe_ctx, bool add_locals, bool force_add_all_locals,
   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_specific_defines = "typedef bool BOOL;\n";
     }
if (sc.comp_unit && sc.line_entry.IsValid()) {
       }
     }
 
     if (add_locals) {
       if (Language::LanguageIsCPlusPlus(frame->GetLanguage())) {
         if (target->GetInjectLocalVariables(&exe_ctx)) {
           lldb::VariableListSP var_list_sp =
               frame->GetInScopeVariableList(false, true);
-          AddLocalVariableDecls(var_list_sp, lldb_local_var_decls);
+          AddLocalVariableDecls(var_list_sp, lldb_local_var_decls,
+                                force_add_all_locals ? "" : m_body);
         }
       }
     }
   }
 
   if (m_wrap) {
     switch (wrapping_language) {
     default:

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;
 
   std::vector<std::string> GetModulesToImport(ExecutionContext &exe_ctx);
   void UpdateLanguageForExpr(DiagnosticManager &diagnostic_manager,
                              ExecutionContext &exe_ctx,
-                             std::vector<std::string> modules_to_import);
+                             std::vector<std::string> modules_to_import,
+                             bool for_completion);
   bool SetupPersistentState(DiagnosticManager &diagnostic_manager,
                                    ExecutionContext &exe_ctx);
   bool PrepareForParsing(DiagnosticManager &diagnostic_manager,
-                         ExecutionContext &exe_ctx);
+                         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;

source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp

if (target->GetEnableAutoImportClangModules()) {
         }
       }
     }
   }
 }
 
 void ClangUserExpression::UpdateLanguageForExpr(
     DiagnosticManager &diagnostic_manager, ExecutionContext &exe_ctx,
-    std::vector<std::string> modules_to_import) {
+    std::vector<std::string> modules_to_import, bool for_completion) {
   m_expr_lang = lldb::LanguageType::eLanguageTypeUnknown;
 
   std::string prefix = m_expr_prefix;
 
   if (m_options.GetExecutionPolicy() == eExecutionPolicyTopLevel) {
     m_transformed_text = m_expr_text;
   } else {
     std::unique_ptr<ClangExpressionSourceCode> source_code(
         ClangExpressionSourceCode::CreateWrapped(prefix.c_str(),
                                             m_expr_text.c_str()));
 
     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;
 
     if (!source_code->GetText(m_transformed_text, m_expr_lang,
                               m_in_static_method, exe_ctx, !m_ctx_obj,
-                              modules_to_import)) {
+                              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;
ClangUserExpression::GetModulesToImport(ExecutionContext &exe_ctx) {
   for (const SourceModule &m : sc.comp_unit->GetImportedModules())
     if (!m.path.empty() && m.path.front() == "std")
       return {"std"};
 
   return {};
 }
 
 bool ClangUserExpression::PrepareForParsing(
-    DiagnosticManager &diagnostic_manager, ExecutionContext &exe_ctx) {
+    DiagnosticManager &diagnostic_manager, ExecutionContext &exe_ctx,
+    bool for_completion) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
 
   InstallContext(exe_ctx);
 
   if (!SetupPersistentState(diagnostic_manager, exe_ctx))
     return false;
 
   Status err;
bool ClangUserExpression::PrepareForParsing(
   SetupDeclVendor(exe_ctx, m_target);
 
   std::vector<std::string> used_modules = GetModulesToImport(exe_ctx);
   m_imported_cpp_modules = !used_modules.empty();
 
   LLDB_LOG(log, "List of imported modules in expression: {0}",
            llvm::make_range(used_modules.begin(), used_modules.end()));
 
-  UpdateLanguageForExpr(diagnostic_manager, exe_ctx, used_modules);
+  UpdateLanguageForExpr(diagnostic_manager, exe_ctx, used_modules,
+                        for_completion);
   return true;
 }
 
 bool ClangUserExpression::Parse(DiagnosticManager &diagnostic_manager,
                                 ExecutionContext &exe_ctx,
                                 lldb_private::ExecutionPolicy execution_policy,
                                 bool keep_result_in_memory,
                                 bool generate_debug_info) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
 
-  if (!PrepareForParsing(diagnostic_manager, exe_ctx))
+  if (!PrepareForParsing(diagnostic_manager, exe_ctx, /*for_completion*/ false))
     return false;
 
   if (log)
     log->Printf("Parsing the following code:\n%s", m_transformed_text.c_str());
 
   ////////////////////////////////////
   // Set up the target and compiler
   //
bool ClangUserExpression::Complete(ExecutionContext &exe_ctx,
                                    unsigned complete_pos) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
 
   // We don't want any visible feedback when completing an expression. Mostly
   // because the results we get from an incomplete invocation are probably not
   // correct.
   DiagnosticManager diagnostic_manager;
 
-  if (!PrepareForParsing(diagnostic_manager, exe_ctx))
+  if (!PrepareForParsing(diagnostic_manager, exe_ctx, /*for_completion*/ true))
     return false;
 
   if (log)
     log->Printf("Parsing the following code:\n%s", m_transformed_text.c_str());
 
   //////////////////////////
   // Parse the expression
   //