Internal expressions shouldn't increment the result variable numbering.

jimingham · jimingham · commit 67d67ebe8f25 · 2020-03-23T13:30:37.000-07:00
There an option: EvaluateExpressionOptions::SetResultIsInternal to indicate whether the result number should be returned to the pool or not. It got broken when the PersistentExpressionState was refactored. This fixes the issue and provides a test of the behavior. Differential Revision: https://reviews.llvm.org/D76532
diff --git a/lldb/include/lldb/Expression/ExpressionVariable.h b/lldb/include/lldb/Expression/ExpressionVariable.h
@@ -221,11 +221,7 @@ class PersistentExpressionState : public ExpressionVariableList {
                            uint32_t addr_byte_size) = 0;
 
   /// Return a new persistent variable name with the specified prefix.
-  ConstString GetNextPersistentVariableName(Target &target,
-                                            llvm::StringRef prefix);
-
-  virtual llvm::StringRef
-  GetPersistentVariablePrefix(bool is_error = false) const = 0;
+  virtual ConstString GetNextPersistentVariableName(bool is_error = false) = 0;
 
   virtual void
   RemovePersistentVariable(lldb::ExpressionVariableSP variable) = 0;
@@ -237,6 +233,10 @@ class PersistentExpressionState : public ExpressionVariableList {
 
   void RegisterExecutionUnit(lldb::IRExecutionUnitSP &execution_unit_sp);
 
+protected:
+  virtual llvm::StringRef
+  GetPersistentVariablePrefix(bool is_error = false) const = 0;
+
 private:
   LLVMCastKind m_kind;
 
diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h
@@ -1100,11 +1100,6 @@ class Target : public std::enable_shared_from_this<Target>,
 
   lldb::ExpressionVariableSP GetPersistentVariable(ConstString name);
 
-  /// Return the next available number for numbered persistent variables.
-  unsigned GetNextPersistentVariableIndex() {
-    return m_next_persistent_variable_index++;
-  }
-
   lldb::addr_t GetPersistentSymbol(ConstString name);
 
   /// This method will return the address of the starting function for
diff --git a/lldb/source/Core/ValueObject.cpp b/lldb/source/Core/ValueObject.cpp
@@ -3270,9 +3270,7 @@ ValueObjectSP ValueObject::Persist() {
   if (!persistent_state)
     return nullptr;
 
-  auto prefix = persistent_state->GetPersistentVariablePrefix();
-  ConstString name =
-      persistent_state->GetNextPersistentVariableName(*target_sp, prefix);
+  ConstString name = persistent_state->GetNextPersistentVariableName();
 
   ValueObjectSP const_result_sp =
       ValueObjectConstResult::Create(target_sp.get(), GetValue(), name);
diff --git a/lldb/source/Expression/ExpressionVariable.cpp b/lldb/source/Expression/ExpressionVariable.cpp
@@ -76,13 +76,3 @@ void PersistentExpressionState::RegisterExecutionUnit(
     }
   }
 }
-
-ConstString PersistentExpressionState::GetNextPersistentVariableName(
-    Target &target, llvm::StringRef Prefix) {
-  llvm::SmallString<64> name;
-  {
-    llvm::raw_svector_ostream os(name);
-    os << Prefix << target.GetNextPersistentVariableIndex();
-  }
-  return ConstString(name);
-}
diff --git a/lldb/source/Expression/Materializer.cpp b/lldb/source/Expression/Materializer.cpp
@@ -881,11 +881,9 @@ class EntityResultVariable : public Materializer::Entity {
       return;
     }
 
-    ConstString name =
-        m_delegate
-            ? m_delegate->GetName()
-            : persistent_state->GetNextPersistentVariableName(
-                  *target_sp, persistent_state->GetPersistentVariablePrefix());
+    ConstString name = m_delegate
+                           ? m_delegate->GetName()
+                           : persistent_state->GetNextPersistentVariableName();
 
     lldb::ExpressionVariableSP ret = persistent_state->CreatePersistentVariable(
         exe_scope, name, m_type, map.GetByteOrder(), map.GetAddressByteSize());
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.cpp
@@ -108,3 +108,14 @@ ClangPersistentVariables::GetClangASTImporter() {
   }
   return m_ast_importer_sp;
 }
+
+ConstString
+ClangPersistentVariables::GetNextPersistentVariableName(bool is_error) {
+  llvm::SmallString<64> name;
+  {
+    llvm::raw_svector_ostream os(name);
+    os << GetPersistentVariablePrefix(is_error)
+       << m_next_persistent_variable_id++;
+  }
+  return ConstString(name);
+}
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h b/lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h
@@ -51,9 +51,7 @@ class ClangPersistentVariables : public PersistentExpressionState {
 
   void RemovePersistentVariable(lldb::ExpressionVariableSP variable) override;
 
-  llvm::StringRef GetPersistentVariablePrefix(bool is_error) const override {
-    return "$";
-  }
+  virtual ConstString GetNextPersistentVariableName(bool is_error = false);
 
   /// Returns the next file name that should be used for user expressions.
   std::string GetNextExprFileName() {
@@ -80,6 +78,12 @@ class ClangPersistentVariables : public PersistentExpressionState {
     return m_hand_loaded_clang_modules;
   }
 
+protected:
+  llvm::StringRef
+  GetPersistentVariablePrefix(bool is_error = false) const override {
+    return "$";
+  }
+
 private:
   /// The counter used by GetNextExprFileName.
   uint32_t m_next_user_file_id = 0;
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
@@ -924,9 +924,7 @@ void ClangUserExpression::ClangUserExpressionHelper::CommitPersistentDecls() {
 }
 
 ConstString ClangUserExpression::ResultDelegate::GetName() {
-  auto prefix = m_persistent_state->GetPersistentVariablePrefix();
-  return m_persistent_state->GetNextPersistentVariableName(*m_target_sp,
-                                                           prefix);
+  return m_persistent_state->GetNextPersistentVariableName(false);
 }
 
 void ClangUserExpression::ResultDelegate::DidDematerialize(
diff --git a/lldb/source/Target/ABI.cpp b/lldb/source/Target/ABI.cpp
@@ -97,10 +97,8 @@ ValueObjectSP ABI::GetReturnValueObject(Thread &thread, CompilerType &ast_type,
     if (!persistent_expression_state)
       return {};
 
-    auto prefix = persistent_expression_state->GetPersistentVariablePrefix();
     ConstString persistent_variable_name =
-        persistent_expression_state->GetNextPersistentVariableName(target,
-                                                                   prefix);
+        persistent_expression_state->GetNextPersistentVariableName();
 
     lldb::ValueObjectSP const_valobj_sp;
 
diff --git a/lldb/test/API/commands/expression/result_numbering/Makefile b/lldb/test/API/commands/expression/result_numbering/Makefile
@@ -0,0 +1,4 @@
+C_SOURCES := main.c
+CFLAGS_EXTRAS := -std=c99
+
+include Makefile.rules
diff --git a/lldb/test/API/commands/expression/result_numbering/TestResultNumbering.py b/lldb/test/API/commands/expression/result_numbering/TestResultNumbering.py
@@ -0,0 +1,48 @@
+"""
+Make sure running internal expressions doesn't
+influence the result variable numbering.
+"""
+
+
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+
+
+class TestExpressionResultNumbering(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    NO_DEBUG_INFO_TESTCASE = True
+
+    def test_sample_rename_this(self):
+        self.build()
+        self.main_source_file = lldb.SBFileSpec("main.c")
+        self.do_numbering_test()
+
+    def do_numbering_test(self):
+        (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(self,
+                                   "Set a breakpoint here", self.main_source_file)
+
+        bkpt = target.BreakpointCreateBySourceRegex("Add conditions to this breakpoint",
+                                                    self.main_source_file)
+        self.assertEqual(bkpt.GetNumLocations(), 1, "Set the breakpoint")
+
+        bkpt.SetCondition("call_me(value) < 6")
+
+        # Get the number of the last expression:
+        result = thread.frames[0].EvaluateExpression("call_me(200)")
+        self.assertTrue(result.GetError().Success(), "Our expression succeeded")
+        name = result.GetName()
+        ordinal = int(name[1:])
+        
+        process.Continue()
+
+        # The condition evaluation had to run a 4 expressions, but we haven't
+        # run any user expressions.
+        result = thread.frames[0].EvaluateExpression("call_me(200)")
+        self.assertTrue(result.GetError().Success(), "Our expression succeeded the second time")
+        after_name = result.GetName()
+        after_ordinal = int(after_name[1:])
+        self.assertEqual(ordinal + 1, after_ordinal) 
diff --git a/lldb/test/API/commands/expression/result_numbering/main.c b/lldb/test/API/commands/expression/result_numbering/main.c
@@ -0,0 +1,18 @@
+#include <stdio.h>
+
+int
+call_me(int input)
+{
+  return input;
+}
+
+int
+main()
+{
+  int value = call_me(0); // Set a breakpoint here
+  while (value < 10)
+    {
+      printf("Add conditions to this breakpoint: %d.\n", value++);
+    }
+  return 0;
+}

Original file line number	Diff line number	Diff line change
`@@ -76,13 +76,3 @@ void PersistentExpressionState::RegisterExecutionUnit(`
`76`	`76`	`}`
`77`	`77`	`}`
`78`	`78`	`}`
`79`		`-`
`80`		`-ConstString PersistentExpressionState::GetNextPersistentVariableName(`
`81`		`- Target &target, llvm::StringRef Prefix) {`
`82`		`- llvm::SmallString<64> name;`
`83`		`- {`
`84`		`- llvm::raw_svector_ostream os(name);`
`85`		`- os << Prefix << target.GetNextPersistentVariableIndex();`
`86`		`- }`
`87`		`- return ConstString(name);`
`88`		`-}`
Original file line number	Diff line number	Diff line change
`@@ -924,9 +924,7 @@ void ClangUserExpression::ClangUserExpressionHelper::CommitPersistentDecls() {`
`924`	`924`	`}`
`925`	`925`
`926`	`926`	`ConstString ClangUserExpression::ResultDelegate::GetName() {`
`927`		`- auto prefix = m_persistent_state->GetPersistentVariablePrefix();`
`928`		`- return m_persistent_state->GetNextPersistentVariableName(*m_target_sp,`
`929`		`- prefix);`
	`927`	`+ return m_persistent_state->GetNextPersistentVariableName(false);`
`930`	`928`	`}`
`931`	`929`
`932`	`930`	`void ClangUserExpression::ResultDelegate::DidDematerialize(`