https://github.com/JDevlieghere updated https://github.com/llvm/llvm-project/pull/140393
>From 82cd419423f74777e248743534f2da48ae6b72c9 Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere <jo...@devlieghere.com> Date: Sat, 17 May 2025 11:34:48 -0700 Subject: [PATCH 1/3] [lldb-dap] Move the Variables struct into its own file (NFC) Move the Variables struct out of DAP.h and into its own file to reduce the complexity of the latter. This PR also makes the members that are implementation details private. --- lldb/tools/lldb-dap/CMakeLists.txt | 1 + lldb/tools/lldb-dap/DAP.cpp | 93 ------------------------- lldb/tools/lldb-dap/DAP.h | 51 +------------- lldb/tools/lldb-dap/Variables.cpp | 105 +++++++++++++++++++++++++++++ lldb/tools/lldb-dap/Variables.h | 71 +++++++++++++++++++ 5 files changed, 178 insertions(+), 143 deletions(-) create mode 100644 lldb/tools/lldb-dap/Variables.cpp create mode 100644 lldb/tools/lldb-dap/Variables.h diff --git a/lldb/tools/lldb-dap/CMakeLists.txt b/lldb/tools/lldb-dap/CMakeLists.txt index 5dedee8a87f41..f8e81eaff8606 100644 --- a/lldb/tools/lldb-dap/CMakeLists.txt +++ b/lldb/tools/lldb-dap/CMakeLists.txt @@ -23,6 +23,7 @@ add_lldb_library(lldbDAP RunInTerminal.cpp SourceBreakpoint.cpp Transport.cpp + Variables.cpp Watchpoint.cpp Handler/ResponseHandler.cpp diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp index 0d5eba6c40961..af46838f5671c 100644 --- a/lldb/tools/lldb-dap/DAP.cpp +++ b/lldb/tools/lldb-dap/DAP.cpp @@ -1021,45 +1021,6 @@ lldb::SBError DAP::WaitForProcessToStop(std::chrono::seconds seconds) { return error; } -void Variables::Clear() { - locals.Clear(); - globals.Clear(); - registers.Clear(); - referenced_variables.clear(); -} - -int64_t Variables::GetNewVariableReference(bool is_permanent) { - if (is_permanent) - return next_permanent_var_ref++; - return next_temporary_var_ref++; -} - -bool Variables::IsPermanentVariableReference(int64_t var_ref) { - return var_ref >= PermanentVariableStartIndex; -} - -lldb::SBValue Variables::GetVariable(int64_t var_ref) const { - if (IsPermanentVariableReference(var_ref)) { - auto pos = referenced_permanent_variables.find(var_ref); - if (pos != referenced_permanent_variables.end()) - return pos->second; - } else { - auto pos = referenced_variables.find(var_ref); - if (pos != referenced_variables.end()) - return pos->second; - } - return lldb::SBValue(); -} - -int64_t Variables::InsertVariable(lldb::SBValue variable, bool is_permanent) { - int64_t var_ref = GetNewVariableReference(is_permanent); - if (is_permanent) - referenced_permanent_variables.insert(std::make_pair(var_ref, variable)); - else - referenced_variables.insert(std::make_pair(var_ref, variable)); - return var_ref; -} - bool StartDebuggingRequestHandler::DoExecute( lldb::SBDebugger debugger, char **command, lldb::SBCommandReturnObject &result) { @@ -1296,60 +1257,6 @@ DAP::GetInstructionBPFromStopReason(lldb::SBThread &thread) { return inst_bp; } -lldb::SBValueList *Variables::GetTopLevelScope(int64_t variablesReference) { - switch (variablesReference) { - case VARREF_LOCALS: - return &locals; - case VARREF_GLOBALS: - return &globals; - case VARREF_REGS: - return ®isters; - default: - return nullptr; - } -} - -lldb::SBValue Variables::FindVariable(uint64_t variablesReference, - llvm::StringRef name) { - lldb::SBValue variable; - if (lldb::SBValueList *top_scope = GetTopLevelScope(variablesReference)) { - bool is_duplicated_variable_name = name.contains(" @"); - // variablesReference is one of our scopes, not an actual variable it is - // asking for a variable in locals or globals or registers - int64_t end_idx = top_scope->GetSize(); - // Searching backward so that we choose the variable in closest scope - // among variables of the same name. - for (int64_t i = end_idx - 1; i >= 0; --i) { - lldb::SBValue curr_variable = top_scope->GetValueAtIndex(i); - std::string variable_name = CreateUniqueVariableNameForDisplay( - curr_variable, is_duplicated_variable_name); - if (variable_name == name) { - variable = curr_variable; - break; - } - } - } else { - // This is not under the globals or locals scope, so there are no - // duplicated names. - - // We have a named item within an actual variable so we need to find it - // withing the container variable by name. - lldb::SBValue container = GetVariable(variablesReference); - variable = container.GetChildMemberWithName(name.data()); - if (!variable.IsValid()) { - if (name.starts_with("[")) { - llvm::StringRef index_str(name.drop_front(1)); - uint64_t index = 0; - if (!index_str.consumeInteger(0, index)) { - if (index_str == "]") - variable = container.GetChildAtIndex(index); - } - } - } - } - return variable; -} - protocol::Capabilities DAP::GetCapabilities() { protocol::Capabilities capabilities; diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h index 8f24c6cf82924..033203309b4c6 100644 --- a/lldb/tools/lldb-dap/DAP.h +++ b/lldb/tools/lldb-dap/DAP.h @@ -20,6 +20,7 @@ #include "Protocol/ProtocolTypes.h" #include "SourceBreakpoint.h" #include "Transport.h" +#include "Variables.h" #include "lldb/API/SBBroadcaster.h" #include "lldb/API/SBCommandInterpreter.h" #include "lldb/API/SBDebugger.h" @@ -30,8 +31,6 @@ #include "lldb/API/SBMutex.h" #include "lldb/API/SBTarget.h" #include "lldb/API/SBThread.h" -#include "lldb/API/SBValue.h" -#include "lldb/API/SBValueList.h" #include "lldb/lldb-types.h" #include "llvm/ADT/DenseMap.h" #include "llvm/ADT/DenseSet.h" @@ -52,10 +51,6 @@ #include <thread> #include <vector> -#define VARREF_LOCALS (int64_t)1 -#define VARREF_GLOBALS (int64_t)2 -#define VARREF_REGS (int64_t)3 -#define VARREF_FIRST_VAR_IDX (int64_t)4 #define NO_TYPENAME "<no-type>" namespace lldb_dap { @@ -88,50 +83,6 @@ enum class PacketStatus { enum class ReplMode { Variable = 0, Command, Auto }; -struct Variables { - /// Variable_reference start index of permanent expandable variable. - static constexpr int64_t PermanentVariableStartIndex = (1ll << 32); - - lldb::SBValueList locals; - lldb::SBValueList globals; - lldb::SBValueList registers; - - int64_t next_temporary_var_ref{VARREF_FIRST_VAR_IDX}; - int64_t next_permanent_var_ref{PermanentVariableStartIndex}; - - /// Variables that are alive in this stop state. - /// Will be cleared when debuggee resumes. - llvm::DenseMap<int64_t, lldb::SBValue> referenced_variables; - /// Variables that persist across entire debug session. - /// These are the variables evaluated from debug console REPL. - llvm::DenseMap<int64_t, lldb::SBValue> referenced_permanent_variables; - - /// Check if \p var_ref points to a variable that should persist for the - /// entire duration of the debug session, e.g. repl expandable variables - static bool IsPermanentVariableReference(int64_t var_ref); - - /// \return a new variableReference. - /// Specify is_permanent as true for variable that should persist entire - /// debug session. - int64_t GetNewVariableReference(bool is_permanent); - - /// \return the expandable variable corresponding with variableReference - /// value of \p value. - /// If \p var_ref is invalid an empty SBValue is returned. - lldb::SBValue GetVariable(int64_t var_ref) const; - - /// Insert a new \p variable. - /// \return variableReference assigned to this expandable variable. - int64_t InsertVariable(lldb::SBValue variable, bool is_permanent); - - lldb::SBValueList *GetTopLevelScope(int64_t variablesReference); - - lldb::SBValue FindVariable(uint64_t variablesReference, llvm::StringRef name); - - /// Clear all scope variables and non-permanent expandable variables. - void Clear(); -}; - struct StartDebuggingRequestHandler : public lldb::SBCommandPluginInterface { DAP &dap; explicit StartDebuggingRequestHandler(DAP &d) : dap(d) {}; diff --git a/lldb/tools/lldb-dap/Variables.cpp b/lldb/tools/lldb-dap/Variables.cpp new file mode 100644 index 0000000000000..777e3183d8c0d --- /dev/null +++ b/lldb/tools/lldb-dap/Variables.cpp @@ -0,0 +1,105 @@ +//===-- Variables.cpp ---------------------------------------------------*-===// +// +// 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 "Variables.h" +#include "JSONUtils.h" + +using namespace lldb_dap; + +lldb::SBValueList *Variables::GetTopLevelScope(int64_t variablesReference) { + switch (variablesReference) { + case VARREF_LOCALS: + return &locals; + case VARREF_GLOBALS: + return &globals; + case VARREF_REGS: + return ®isters; + default: + return nullptr; + } +} + +void Variables::Clear() { + locals.Clear(); + globals.Clear(); + registers.Clear(); + m_referencedvariables.clear(); +} + +int64_t Variables::GetNewVariableReference(bool is_permanent) { + if (is_permanent) + return m_next_permanent_var_ref++; + return m_next_temporary_var_ref++; +} + +bool Variables::IsPermanentVariableReference(int64_t var_ref) { + return var_ref >= PermanentVariableStartIndex; +} + +lldb::SBValue Variables::GetVariable(int64_t var_ref) const { + if (IsPermanentVariableReference(var_ref)) { + auto pos = m_referencedpermanent_variables.find(var_ref); + if (pos != m_referencedpermanent_variables.end()) + return pos->second; + } else { + auto pos = m_referencedvariables.find(var_ref); + if (pos != m_referencedvariables.end()) + return pos->second; + } + return lldb::SBValue(); +} + +int64_t Variables::InsertVariable(lldb::SBValue variable, bool is_permanent) { + int64_t var_ref = GetNewVariableReference(is_permanent); + if (is_permanent) + m_referencedpermanent_variables.insert(std::make_pair(var_ref, variable)); + else + m_referencedvariables.insert(std::make_pair(var_ref, variable)); + return var_ref; +} + +lldb::SBValue Variables::FindVariable(uint64_t variablesReference, + llvm::StringRef name) { + lldb::SBValue variable; + if (lldb::SBValueList *top_scope = GetTopLevelScope(variablesReference)) { + bool is_duplicated_variable_name = name.contains(" @"); + // variablesReference is one of our scopes, not an actual variable it is + // asking for a variable in locals or globals or registers + int64_t end_idx = top_scope->GetSize(); + // Searching backward so that we choose the variable in closest scope + // among variables of the same name. + for (int64_t i = end_idx - 1; i >= 0; --i) { + lldb::SBValue curr_variable = top_scope->GetValueAtIndex(i); + std::string variable_name = CreateUniqueVariableNameForDisplay( + curr_variable, is_duplicated_variable_name); + if (variable_name == name) { + variable = curr_variable; + break; + } + } + } else { + // This is not under the globals or locals scope, so there are no + // duplicated names. + + // We have a named item within an actual variable so we need to find it + // withing the container variable by name. + lldb::SBValue container = GetVariable(variablesReference); + variable = container.GetChildMemberWithName(name.data()); + if (!variable.IsValid()) { + if (name.starts_with("[")) { + llvm::StringRef index_str(name.drop_front(1)); + uint64_t index = 0; + if (!index_str.consumeInteger(0, index)) { + if (index_str == "]") + variable = container.GetChildAtIndex(index); + } + } + } + } + return variable; +} diff --git a/lldb/tools/lldb-dap/Variables.h b/lldb/tools/lldb-dap/Variables.h new file mode 100644 index 0000000000000..0ed84b36aef99 --- /dev/null +++ b/lldb/tools/lldb-dap/Variables.h @@ -0,0 +1,71 @@ +//===-- Variables.h -----------------------------------------------------*-===// +// +// 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 +// +//===----------------------------------------------------------------------===// + +#ifndef LLDB_TOOLS_LLDB_DAP_VARIABLES_H +#define LLDB_TOOLS_LLDB_DAP_VARIABLES_H + +#include "lldb/API/SBValue.h" +#include "lldb/API/SBValueList.h" +#include "llvm/ADT/DenseMap.h" + +#define VARREF_FIRST_VAR_IDX (int64_t)4 +#define VARREF_LOCALS (int64_t)1 +#define VARREF_GLOBALS (int64_t)2 +#define VARREF_REGS (int64_t)3 + +namespace lldb_dap { + +struct Variables { + lldb::SBValueList locals; + lldb::SBValueList globals; + lldb::SBValueList registers; + + /// Check if \p var_ref points to a variable that should persist for the + /// entire duration of the debug session, e.g. repl expandable variables + static bool IsPermanentVariableReference(int64_t var_ref); + + /// \return a new variableReference. + /// Specify is_permanent as true for variable that should persist entire + /// debug session. + int64_t GetNewVariableReference(bool is_permanent); + + /// \return the expandable variable corresponding with variableReference + /// value of \p value. + /// If \p var_ref is invalid an empty SBValue is returned. + lldb::SBValue GetVariable(int64_t var_ref) const; + + /// Insert a new \p variable. + /// \return variableReference assigned to this expandable variable. + int64_t InsertVariable(lldb::SBValue variable, bool is_permanent); + + lldb::SBValueList *GetTopLevelScope(int64_t variablesReference); + + lldb::SBValue FindVariable(uint64_t variablesReference, llvm::StringRef name); + + /// Clear all scope variables and non-permanent expandable variables. + void Clear(); + +private: + /// Variable_reference start index of permanent expandable variable. + static constexpr int64_t PermanentVariableStartIndex = (1ll << 32); + + /// Variables that are alive in this stop state. + /// Will be cleared when debuggee resumes. + llvm::DenseMap<int64_t, lldb::SBValue> m_referencedvariables; + + /// Variables that persist across entire debug session. + /// These are the variables evaluated from debug console REPL. + llvm::DenseMap<int64_t, lldb::SBValue> m_referencedpermanent_variables; + + int64_t m_next_temporary_var_ref{VARREF_FIRST_VAR_IDX}; + int64_t m_next_permanent_var_ref{PermanentVariableStartIndex}; +}; + +} // namespace lldb_dap + +#endif >From 62d94ae00b12006aacae7108f67a3b944fd97bdf Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere <jo...@devlieghere.com> Date: Sat, 17 May 2025 11:58:48 -0700 Subject: [PATCH 2/3] Add Variables unit tests --- lldb/unittests/DAP/CMakeLists.txt | 1 + lldb/unittests/DAP/VariablesTest.cpp | 84 ++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+) create mode 100644 lldb/unittests/DAP/VariablesTest.cpp diff --git a/lldb/unittests/DAP/CMakeLists.txt b/lldb/unittests/DAP/CMakeLists.txt index af7d11e2e95e2..429a12e9fb505 100644 --- a/lldb/unittests/DAP/CMakeLists.txt +++ b/lldb/unittests/DAP/CMakeLists.txt @@ -6,6 +6,7 @@ add_lldb_unittest(DAPTests ProtocolTypesTest.cpp TestBase.cpp TransportTest.cpp + VariablesTest.cpp LINK_LIBS lldbDAP diff --git a/lldb/unittests/DAP/VariablesTest.cpp b/lldb/unittests/DAP/VariablesTest.cpp new file mode 100644 index 0000000000000..1ee98c0d1f8e0 --- /dev/null +++ b/lldb/unittests/DAP/VariablesTest.cpp @@ -0,0 +1,84 @@ +//===-- VariablesTest.cpp -------------------------------------------------===// +// +// 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 "Variables.h" +#include "lldb/API/SBValue.h" +#include "lldb/API/SBValueList.h" +#include "gtest/gtest.h" + +using namespace lldb_dap; + +class VariablesTest : public ::testing::Test { +protected: + Variables vars; +}; + +TEST_F(VariablesTest, GetNewVariableReference_UniqueAndRanges) { + const int64_t temp1 = vars.GetNewVariableReference(false); + const int64_t temp2 = vars.GetNewVariableReference(false); + const int64_t perm1 = vars.GetNewVariableReference(true); + const int64_t perm2 = vars.GetNewVariableReference(true); + + EXPECT_NE(temp1, temp2); + EXPECT_NE(perm1, perm2); + EXPECT_LT(temp1, perm1); + EXPECT_LT(temp2, perm1); +} + +TEST_F(VariablesTest, InsertAndGetVariable_Temporary) { + lldb::SBValue dummy; + const int64_t ref = vars.InsertVariable(dummy, false); + lldb::SBValue out = vars.GetVariable(ref); + + EXPECT_TRUE(out.IsValid() == dummy.IsValid()); +} + +TEST_F(VariablesTest, InsertAndGetVariable_Permanent) { + lldb::SBValue dummy; + const int64_t ref = vars.InsertVariable(dummy, true); + lldb::SBValue out = vars.GetVariable(ref); + + EXPECT_TRUE(out.IsValid() == dummy.IsValid()); +} + +TEST_F(VariablesTest, IsPermanentVariableReference) { + const int64_t perm = vars.GetNewVariableReference(true); + const int64_t temp = vars.GetNewVariableReference(false); + + EXPECT_TRUE(Variables::IsPermanentVariableReference(perm)); + EXPECT_FALSE(Variables::IsPermanentVariableReference(temp)); +} + +TEST_F(VariablesTest, Clear_RemovesTemporaryKeepsPermanent) { + lldb::SBValue dummy; + const int64_t temp = vars.InsertVariable(dummy, false); + const int64_t perm = vars.InsertVariable(dummy, true); + vars.Clear(); + + EXPECT_FALSE(vars.GetVariable(temp).IsValid()); + EXPECT_TRUE(vars.GetVariable(perm).IsValid() == dummy.IsValid()); +} + +TEST_F(VariablesTest, GetTopLevelScope_ReturnsCorrectScope) { + vars.locals.Append(lldb::SBValue()); + vars.globals.Append(lldb::SBValue()); + vars.registers.Append(lldb::SBValue()); + + EXPECT_EQ(vars.GetTopLevelScope(VARREF_LOCALS), &vars.locals); + EXPECT_EQ(vars.GetTopLevelScope(VARREF_GLOBALS), &vars.globals); + EXPECT_EQ(vars.GetTopLevelScope(VARREF_REGS), &vars.registers); + EXPECT_EQ(vars.GetTopLevelScope(9999), nullptr); +} + +TEST_F(VariablesTest, FindVariable_LocalsByName) { + lldb::SBValue dummy; + vars.locals.Append(dummy); + lldb::SBValue found = vars.FindVariable(VARREF_LOCALS, ""); + + EXPECT_TRUE(found.IsValid() == dummy.IsValid()); +} >From e3f677bd7b03ccd089731c6c6f0177bf990cd347 Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere <jo...@devlieghere.com> Date: Sat, 17 May 2025 12:03:18 -0700 Subject: [PATCH 3/3] Improve test readability --- lldb/unittests/DAP/VariablesTest.cpp | 29 ++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/lldb/unittests/DAP/VariablesTest.cpp b/lldb/unittests/DAP/VariablesTest.cpp index 1ee98c0d1f8e0..6b14fc6c3945d 100644 --- a/lldb/unittests/DAP/VariablesTest.cpp +++ b/lldb/unittests/DAP/VariablesTest.cpp @@ -15,14 +15,15 @@ using namespace lldb_dap; class VariablesTest : public ::testing::Test { protected: + enum : bool { Permanent = true, Temporary = false }; Variables vars; }; TEST_F(VariablesTest, GetNewVariableReference_UniqueAndRanges) { - const int64_t temp1 = vars.GetNewVariableReference(false); - const int64_t temp2 = vars.GetNewVariableReference(false); - const int64_t perm1 = vars.GetNewVariableReference(true); - const int64_t perm2 = vars.GetNewVariableReference(true); + const int64_t temp1 = vars.GetNewVariableReference(Temporary); + const int64_t temp2 = vars.GetNewVariableReference(Temporary); + const int64_t perm1 = vars.GetNewVariableReference(Permanent); + const int64_t perm2 = vars.GetNewVariableReference(Permanent); EXPECT_NE(temp1, temp2); EXPECT_NE(perm1, perm2); @@ -32,23 +33,23 @@ TEST_F(VariablesTest, GetNewVariableReference_UniqueAndRanges) { TEST_F(VariablesTest, InsertAndGetVariable_Temporary) { lldb::SBValue dummy; - const int64_t ref = vars.InsertVariable(dummy, false); + const int64_t ref = vars.InsertVariable(dummy, Temporary); lldb::SBValue out = vars.GetVariable(ref); - EXPECT_TRUE(out.IsValid() == dummy.IsValid()); + EXPECT_EQ(out.IsValid(), dummy.IsValid()); } TEST_F(VariablesTest, InsertAndGetVariable_Permanent) { lldb::SBValue dummy; - const int64_t ref = vars.InsertVariable(dummy, true); + const int64_t ref = vars.InsertVariable(dummy, Permanent); lldb::SBValue out = vars.GetVariable(ref); - EXPECT_TRUE(out.IsValid() == dummy.IsValid()); + EXPECT_EQ(out.IsValid(), dummy.IsValid()); } TEST_F(VariablesTest, IsPermanentVariableReference) { - const int64_t perm = vars.GetNewVariableReference(true); - const int64_t temp = vars.GetNewVariableReference(false); + const int64_t perm = vars.GetNewVariableReference(Permanent); + const int64_t temp = vars.GetNewVariableReference(Temporary); EXPECT_TRUE(Variables::IsPermanentVariableReference(perm)); EXPECT_FALSE(Variables::IsPermanentVariableReference(temp)); @@ -56,12 +57,12 @@ TEST_F(VariablesTest, IsPermanentVariableReference) { TEST_F(VariablesTest, Clear_RemovesTemporaryKeepsPermanent) { lldb::SBValue dummy; - const int64_t temp = vars.InsertVariable(dummy, false); - const int64_t perm = vars.InsertVariable(dummy, true); + const int64_t temp = vars.InsertVariable(dummy, Temporary); + const int64_t perm = vars.InsertVariable(dummy, Permanent); vars.Clear(); EXPECT_FALSE(vars.GetVariable(temp).IsValid()); - EXPECT_TRUE(vars.GetVariable(perm).IsValid() == dummy.IsValid()); + EXPECT_EQ(vars.GetVariable(perm).IsValid(), dummy.IsValid()); } TEST_F(VariablesTest, GetTopLevelScope_ReturnsCorrectScope) { @@ -80,5 +81,5 @@ TEST_F(VariablesTest, FindVariable_LocalsByName) { vars.locals.Append(dummy); lldb::SBValue found = vars.FindVariable(VARREF_LOCALS, ""); - EXPECT_TRUE(found.IsValid() == dummy.IsValid()); + EXPECT_EQ(found.IsValid(), dummy.IsValid()); } _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits