Internal expressions shouldn't increment the result variable numbering.

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

lldb/include/lldb/Expression/ExpressionVariable.h

@@ -221,11 +221,7 @@ public:
                            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 @@ public:
 
   void RegisterExecutionUnit(lldb::IRExecutionUnitSP &execution_unit_sp);
 
+protected:
+  virtual llvm::StringRef
+  GetPersistentVariablePrefix(bool is_error = false) const = 0;
+
 private:
   LLVMCastKind m_kind;
 

lldb/include/lldb/Target/Target.h

@@ -1100,11 +1100,6 @@ public:
 
   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

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);

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);
-}

lldb/source/Expression/Materializer.cpp

@@ -881,11 +881,9 @@ public:
       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());

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);
+}

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

@@ -51,9 +51,7 @@ public:
 
   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 @@ public:
     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;

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(

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;
 

lldb/test/API/commands/expression/result_numbering/Makefile

@@ -0,0 +1,4 @@
+C_SOURCES := main.c
+CFLAGS_EXTRAS := -std=c99
+
+include Makefile.rules

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) 

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;
+}