Internal expressions should not increment the next result variable numbering

[jingham]

Summary

For instance, breakpoints run an expression when evaluating a condition. If each of these expression evaluations incremented the result numbering, you could end up running an expression, and getting:

(lldb) expression foo
$1 = ...

Then do a continue, and then:

(lldb) expression foo
$14532 =

That would be disconcerting...

There was code (but no test, alas) to make this work, but when we refactored the PersistentExpression class, this got broken.

This patch goes back to using the original method of computing the number. But I also changed the API's a bit. When you went to get a result variable name in the current method you had to do:

auto prefix = persistent_expression_state->GetPersistentVariablePrefix();
 ConstString persistent_variable_name =
    persistent_expression_state->GetNextPersistentVariableName(target,
                                                               prefix);

The persistent variable prefix is only ever used to make the persistent variable name, so having this be done in two steps is unnecessary. It was also odd that you needed the target - that was only because we the "next result variable index" had been errantly moved into the target. So I made GetNextPersistentVariableName the public interface, and passed in only the is_error bool. I left the GetPersistentVariablePrefix in as a protected method - it doesn't do anything for C, but it is used for swift.

I also added a test.

lldb/include/lldb/Expression/ExpressionVariable.h

public:
 
   virtual lldb::ExpressionVariableSP
   CreatePersistentVariable(ExecutionContextScope *exe_scope,
                            ConstString name, const CompilerType &type,
                            lldb::ByteOrder byte_order,
                            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;
 
   virtual llvm::Optional<CompilerType>
   GetCompilerTypeFromPersistentDecl(ConstString type_name) = 0;
 
   virtual lldb::addr_t LookupSymbol(ConstString name);
 
   void RegisterExecutionUnit(lldb::IRExecutionUnitSP &execution_unit_sp);
 
+protected:
+  virtual llvm::StringRef
+  GetPersistentVariablePrefix(bool is_error = false) const = 0;
+
 private:
   LLVMCastKind m_kind;
 
   typedef std::set<lldb::IRExecutionUnitSP> ExecutionUnitSet;
   ExecutionUnitSet
       m_execution_units; ///< The execution units that contain valuable symbols.
 
   typedef llvm::DenseMap<const char *, lldb::addr_t> SymbolMap;
   SymbolMap
       m_symbol_map; ///< The addresses of the symbols in m_execution_units.
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_EXPRESSION_EXPRESSIONVARIABLE_H

lldb/include/lldb/Target/Target.h

public:
   lldb::ExpressionResults EvaluateExpression(
       llvm::StringRef expression, ExecutionContextScope *exe_scope,
       lldb::ValueObjectSP &result_valobj_sp,
       const EvaluateExpressionOptions &options = EvaluateExpressionOptions(),
       std::string *fixed_expression = nullptr, ValueObject *ctx_obj = nullptr);
 
   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
   /// this binary, e.g. main() or its equivalent.  This can be used as
   /// an address of a function that is not called once a binary has
   /// started running - e.g. as a return address for inferior function
   /// calls that are unambiguous completion of the function call, not
   /// called during the course of the inferior function code running.

lldb/source/Core/ValueObject.cpp

ValueObjectSP ValueObject::Persist() {
 
   PersistentExpressionState *persistent_state =
       target_sp->GetPersistentExpressionStateForLanguage(
           GetPreferredDisplayLanguage());
 
   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);
 
   ExpressionVariableSP persistent_var_sp =
       persistent_state->CreatePersistentVariable(const_result_sp);
   persistent_var_sp->m_live_sp = persistent_var_sp->m_frozen_sp;
   persistent_var_sp->m_flags |= ExpressionVariable::EVIsProgramReference;

lldb/source/Expression/ExpressionVariable.cpp

if (global_var.m_remote_addr != LLDB_INVALID_ADDRESS) {
       Mangled mangler(global_var.m_name);
       mangler.GetDemangledName();
       m_symbol_map[global_var.m_name.GetCString()] = global_var.m_remote_addr;
       LLDB_LOGF(log, "  Symbol: %s at 0x%" PRIx64 ".",
                 global_var.m_name.GetCString(), global_var.m_remote_addr);
     }
   }
 }
-
-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

void Dematerialize(lldb::StackFrameSP &frame_sp, IRMemoryMap &map,
 
     if (!persistent_state) {
       err.SetErrorString("Couldn't dematerialize a result variable: "
                          "corresponding type system doesn't handle persistent "
                          "variables");
       return;
     }
 
-    ConstString name =
-        m_delegate
+    ConstString name = m_delegate
                            ? m_delegate->GetName()
-            : persistent_state->GetNextPersistentVariableName(
-                  *target_sp, persistent_state->GetPersistentVariablePrefix());
+                           : persistent_state->GetNextPersistentVariableName();
 
     lldb::ExpressionVariableSP ret = persistent_state->CreatePersistentVariable(
         exe_scope, name, m_type, map.GetByteOrder(), map.GetAddressByteSize());
 
     if (!ret) {
       err.SetErrorStringWithFormat("couldn't dematerialize a result variable: "
                                    "failed to make persistent variable %s",
                                    name.AsCString());

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

public:
 
   lldb::ExpressionVariableSP CreatePersistentVariable(
       ExecutionContextScope *exe_scope, ConstString name,
       const CompilerType &compiler_type, lldb::ByteOrder byte_order,
       uint32_t addr_byte_size) override;
 
   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() {
     std::string name;
     name.append("<user expression ");
     name.append(std::to_string(m_next_user_file_id++));
     name.append(">");
     return name;
public:
   void AddHandLoadedClangModule(ClangModulesDeclVendor::ModuleID module) {
     m_hand_loaded_clang_modules.push_back(module);
   }
 
   const ClangModulesDeclVendor::ModuleVector &GetHandLoadedClangModules() {
     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;
   // The counter used by GetNextPersistentVariableName
   uint32_t m_next_persistent_variable_id = 0;
 
   struct PersistentDecl {
     /// The persistent decl.

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

 
 std::shared_ptr<ClangASTImporter>
 ClangPersistentVariables::GetClangASTImporter() {
   if (!m_ast_importer_sp) {
     m_ast_importer_sp = std::make_shared<ClangASTImporter>();
   }
   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/ClangUserExpression.cpp

 
 void ClangUserExpression::ClangUserExpressionHelper::CommitPersistentDecls() {
   if (m_result_synthesizer_up) {
     m_result_synthesizer_up->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::ExpressionVariableSP &variable) {
   m_variable = variable;
 }
 
 void ClangUserExpression::ResultDelegate::RegisterPersistentState(
     PersistentExpressionState *persistent_state) {
   m_persistent_state = persistent_state;
 }
 
 lldb::ExpressionVariableSP &ClangUserExpression::ResultDelegate::GetVariable() {
   return m_variable;
 }

lldb/source/Target/ABI.cpp

if (persistent) {
     Target &target = *thread.CalculateTarget();
     PersistentExpressionState *persistent_expression_state =
         target.GetPersistentExpressionStateForLanguage(
             ast_type.GetMinimumLanguage());
 
     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;
 
     // Check in case our value is already a constant value
     if (return_valobj_sp->GetIsConstant()) {
       const_valobj_sp = return_valobj_sp;
       const_valobj_sp->SetName(persistent_variable_name);
     } else

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

+C_SOURCES := main.c
+CFLAGS_EXTRAS := -std=c99
+
+include Makefile.rules

lldb/test/API/commands/expression/result_numbering/TestResultNumbering.py

+"""
+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

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