Author: Jim Ingham Date: 2020-03-23T13:30:37-07:00 New Revision: 67d67ebe8f2535c5de75c62820f7713f87d07307
URL: https://github.com/llvm/llvm-project/commit/67d67ebe8f2535c5de75c62820f7713f87d07307 DIFF: https://github.com/llvm/llvm-project/commit/67d67ebe8f2535c5de75c62820f7713f87d07307.diff LOG: 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 Added: lldb/test/API/commands/expression/result_numbering/Makefile lldb/test/API/commands/expression/result_numbering/TestResultNumbering.py lldb/test/API/commands/expression/result_numbering/main.c Modified: lldb/include/lldb/Expression/ExpressionVariable.h lldb/include/lldb/Target/Target.h lldb/source/Core/ValueObject.cpp lldb/source/Expression/ExpressionVariable.cpp lldb/source/Expression/Materializer.cpp lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.cpp lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp lldb/source/Target/ABI.cpp Removed: ################################################################################ diff --git a/lldb/include/lldb/Expression/ExpressionVariable.h b/lldb/include/lldb/Expression/ExpressionVariable.h index c523176e003f..60062d212bad 100644 --- 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 index 77cda4998192..cc74fe0f3d74 100644 --- 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 index e1d0ca941108..4c9c44ea15f4 100644 --- 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 index 7c27c0f249ec..d95f0745cf4b 100644 --- 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 index 33c061effca4..8e96891257e4 100644 --- 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 index 3cbedf80755a..42afac9edb0d 100644 --- 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 index 12268b6549aa..f6298911dd6c 100644 --- 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 index 6d781934c174..b246fc374d1c 100644 --- 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 index cb7eca280a39..4320eb93adfc 100644 --- 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 new file mode 100644 index 000000000000..695335e068c0 --- /dev/null +++ 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 new file mode 100644 index 000000000000..cd6b9c43775c --- /dev/null +++ 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 new file mode 100644 index 000000000000..0f5853c99fb1 --- /dev/null +++ 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; +} _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits