llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: John Harrison (ashgti) <details> <summary>Changes</summary> Adjusting `VariableReferenceStorage` to only need to track permanent vs temporary storage by making `VariableStore` the common base class. Moved the subclasses of `VariableStore` into the Variables.cpp file, since they're no long referenced externally. Expanding on the tests by adding an updated core dump with variables in the argument scope we can use to validate variable storage. --- Patch is 301.14 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/183176.diff 11 Files Affected: - (modified) lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp (+2-2) - (modified) lldb/tools/lldb-dap/Handler/ScopesRequestHandler.cpp (+1-2) - (modified) lldb/tools/lldb-dap/Handler/SetVariableRequestHandler.cpp (+1-1) - (modified) lldb/tools/lldb-dap/Handler/VariablesRequestHandler.cpp (-1) - (modified) lldb/tools/lldb-dap/Protocol/DAPTypes.h (+1-3) - (modified) lldb/tools/lldb-dap/Variables.cpp (+373-263) - (modified) lldb/tools/lldb-dap/Variables.h (+18-69) - (modified) lldb/unittests/DAP/DAPTypesTest.cpp (+7-13) - (modified) lldb/unittests/DAP/Inputs/linux-x86_64.core.yaml (+834-33) - (modified) lldb/unittests/DAP/Inputs/linux-x86_64.out.yaml (+550-80) - (modified) lldb/unittests/DAP/VariablesTest.cpp (+103-16) ``````````diff diff --git a/lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp index 77d9f03554a2a..f2117d81f315c 100644 --- a/lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/EvaluateRequestHandler.cpp @@ -133,8 +133,8 @@ EvaluateRequestHandler::Run(const EvaluateArguments &arguments) const { body.type = desc.display_type_name; if (value.MightHaveChildren() || ValuePointsToCode(value)) - body.variablesReference = dap.reference_storage.InsertVariable( - value, /*is_permanent=*/is_repl_context); + body.variablesReference = + dap.reference_storage.Insert(value, /*is_permanent=*/is_repl_context); if (lldb::addr_t addr = value.GetLoadAddress(); addr != LLDB_INVALID_ADDRESS) body.memoryReference = EncodeMemoryReference(addr); diff --git a/lldb/tools/lldb-dap/Handler/ScopesRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/ScopesRequestHandler.cpp index 3786d583e0e87..b2b006baf8a5e 100644 --- a/lldb/tools/lldb-dap/Handler/ScopesRequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/ScopesRequestHandler.cpp @@ -36,8 +36,7 @@ ScopesRequestHandler::Run(const ScopesArguments &args) const { frame.GetThread().SetSelectedFrame(frame.GetFrameID()); } - std::vector<protocol::Scope> scopes = - dap.reference_storage.CreateScopes(frame); + std::vector<protocol::Scope> scopes = dap.reference_storage.Insert(frame); return ScopesResponseBody{std::move(scopes)}; } diff --git a/lldb/tools/lldb-dap/Handler/SetVariableRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/SetVariableRequestHandler.cpp index 725d5de094c95..8fe424b55c9b1 100644 --- a/lldb/tools/lldb-dap/Handler/SetVariableRequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/SetVariableRequestHandler.cpp @@ -67,7 +67,7 @@ SetVariableRequestHandler::Run(const SetVariableArguments &args) const { // is_permanent is false because debug console does not support // setVariable request. const var_ref_t new_var_ref = - dap.reference_storage.InsertVariable(variable, /*is_permanent=*/false); + dap.reference_storage.Insert(variable, /*is_permanent=*/false); if (variable.MightHaveChildren()) { body.variablesReference = new_var_ref; if (desc.type_obj.IsArrayType()) diff --git a/lldb/tools/lldb-dap/Handler/VariablesRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/VariablesRequestHandler.cpp index 26719b3a14d4d..46dda9e4c5d08 100644 --- a/lldb/tools/lldb-dap/Handler/VariablesRequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/VariablesRequestHandler.cpp @@ -13,7 +13,6 @@ #include "Protocol/DAPTypes.h" #include "Protocol/ProtocolRequests.h" #include "Variables.h" -#include "llvm/Support/ErrorExtras.h" using namespace llvm; using namespace lldb_dap::protocol; diff --git a/lldb/tools/lldb-dap/Protocol/DAPTypes.h b/lldb/tools/lldb-dap/Protocol/DAPTypes.h index 5c5d279656d83..5564489cf2e69 100644 --- a/lldb/tools/lldb-dap/Protocol/DAPTypes.h +++ b/lldb/tools/lldb-dap/Protocol/DAPTypes.h @@ -28,8 +28,7 @@ namespace lldb_dap::protocol { enum ReferenceKind : uint8_t { eReferenceKindTemporary = 0, eReferenceKindPermanent = 1, - eReferenceKindScope = 1 << 1, - eReferenceKindInvalid = 0xFF + eReferenceKindInvalid = 0xFF, }; /// The var_ref_t hold two values, the `ReferenceKind` and the @@ -63,7 +62,6 @@ struct var_ref_t { switch (current_kind) { case eReferenceKindTemporary: case eReferenceKindPermanent: - case eReferenceKindScope: return current_kind; default: return eReferenceKindInvalid; diff --git a/lldb/tools/lldb-dap/Variables.cpp b/lldb/tools/lldb-dap/Variables.cpp index 1f429b087ac0e..7891ee64ac9fc 100644 --- a/lldb/tools/lldb-dap/Variables.cpp +++ b/lldb/tools/lldb-dap/Variables.cpp @@ -1,4 +1,4 @@ -//===-- 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. @@ -15,7 +15,6 @@ #include "ProtocolUtils.h" #include "SBAPIExtras.h" #include "lldb/API/SBFrame.h" -#include "lldb/API/SBProcess.h" #include "lldb/API/SBValue.h" #include "lldb/API/SBValueList.h" #include "llvm/ADT/Sequence.h" @@ -27,285 +26,394 @@ using namespace lldb_dap; using namespace lldb_dap::protocol; -namespace lldb_dap { - -protocol::Scope CreateScope(ScopeKind kind, var_ref_t variablesReference, - bool expensive) { - protocol::Scope scope; - scope.variablesReference = variablesReference; - scope.expensive = expensive; +namespace { + +/// A Variable store for fetching variables within a specific scope (locals, +/// globals, or registers) for a given stack frame. +class ScopeStore final : public VariableStore { +public: + explicit ScopeStore(ScopeKind kind, const lldb::SBFrame &frame) + : m_frame(frame), m_kind(kind) {} + + std::vector<protocol::Variable> + GetVariables(VariableReferenceStorage &storage, + const protocol::Configuration &config, + const protocol::VariablesArguments &args) override { + LoadVariables(); + if (m_kind == lldb_dap::eScopeKindRegisters) + SetRegistersFormat(); + + const bool format_hex = args.format ? args.format->hex : false; + std::vector<Variable> variables; + if (m_kind == eScopeKindLocals) + AddReturnValue(storage, config, variables, format_hex); + + const uint64_t count = args.count; + const uint32_t start_idx = 0; + const uint32_t num_children = m_children.GetSize(); + const uint32_t end_idx = start_idx + ((count == 0) ? num_children : count); + + // We first find out which variable names are duplicated. + std::map<llvm::StringRef, uint32_t> variable_name_counts; + for (auto i = start_idx; i < end_idx; ++i) { + lldb::SBValue variable = m_children.GetValueAtIndex(i); + if (!variable.IsValid()) + break; + variable_name_counts[GetNonNullVariableName(variable)]++; + } - // TODO: Support "arguments" and "return value" scope. - // At the moment lldb-dap includes the arguments and return_value into the - // "locals" scope. - // VS Code only expands the first non-expensive scope. This causes friction - // if we add the arguments above the local scope, as the locals scope will not - // be expanded if we enter a function with arguments. It becomes more - // annoying when the scope has arguments, return_value and locals. - switch (kind) { - case eScopeKindLocals: - scope.presentationHint = protocol::Scope::eScopePresentationHintLocals; - scope.name = "Locals"; - break; - case eScopeKindGlobals: - scope.name = "Globals"; - break; - case eScopeKindRegisters: - scope.presentationHint = protocol::Scope::eScopePresentationHintRegisters; - scope.name = "Registers"; - break; + // Now we construct the result with unique display variable names. + for (auto i = start_idx; i < end_idx; ++i) { + lldb::SBValue variable = m_children.GetValueAtIndex(i); + + if (!variable.IsValid()) + break; + + const var_ref_t frame_var_ref = + storage.Insert(variable, /*is_permanent=*/false); + if (LLVM_UNLIKELY(frame_var_ref.AsUInt32() >= + var_ref_t::k_variables_reference_threshold)) { + DAP_LOG(storage.log, + "warning: scopes variablesReference threshold reached. " + "current: {} threshold: {}, maximum {}.", + frame_var_ref.AsUInt32(), + var_ref_t::k_variables_reference_threshold, + var_ref_t::k_max_variables_references); + } + + if (LLVM_UNLIKELY(frame_var_ref.Kind() == eReferenceKindInvalid)) + break; + + variables.emplace_back(CreateVariable( + variable, frame_var_ref, format_hex, + config.enableAutoVariableSummaries, + config.enableSyntheticChildDebugging, + variable_name_counts[GetNonNullVariableName(variable)] > 1)); + } + return variables; } - return scope; -} - -std::vector<Variable> -ScopeStore::GetVariables(VariableReferenceStorage &storage, - const Configuration &config, - const VariablesArguments &args) { - LoadVariables(); - if (m_kind == lldb_dap::eScopeKindRegisters) - SetRegistersFormat(); - - const bool format_hex = args.format ? args.format->hex : false; - std::vector<Variable> variables; - if (m_kind == eScopeKindLocals) - AddReturnValue(storage, config, variables, format_hex); - - const uint64_t count = args.count; - const uint32_t start_idx = 0; - const uint32_t num_children = m_children.GetSize(); - const uint32_t end_idx = start_idx + ((count == 0) ? num_children : count); - - // We first find out which variable names are duplicated. - std::map<llvm::StringRef, uint32_t> variable_name_counts; - for (auto i = start_idx; i < end_idx; ++i) { - lldb::SBValue variable = m_children.GetValueAtIndex(i); - if (!variable.IsValid()) - break; - variable_name_counts[GetNonNullVariableName(variable)]++; + lldb::SBValue FindVariable(llvm::StringRef name) override { + LoadVariables(); + + lldb::SBValue variable; + const bool is_name_duplicated = name.contains(" @"); + // variablesReference is one of our scopes, not an actual variable it is + // asking for a variable in locals or globals or registers. + const uint32_t end_idx = m_children.GetSize(); + // Searching backward so that we choose the variable in closest scope + // among variables of the same name. + for (const uint32_t i : llvm::reverse(llvm::seq<uint32_t>(0, end_idx))) { + lldb::SBValue curr_variable = m_children.GetValueAtIndex(i); + std::string variable_name = + CreateUniqueVariableNameForDisplay(curr_variable, is_name_duplicated); + if (variable_name == name) { + variable = curr_variable; + break; + } + } + return variable; } - // Now we construct the result with unique display variable names. - for (auto i = start_idx; i < end_idx; ++i) { - lldb::SBValue variable = m_children.GetValueAtIndex(i); + lldb::SBValue GetVariable() const override { return lldb::SBValue(); } - if (!variable.IsValid()) - break; - - const var_ref_t frame_var_ref = - storage.InsertVariable(variable, /*is_permanent=*/false); - if (LLVM_UNLIKELY(frame_var_ref.AsUInt32() >= - var_ref_t::k_variables_reference_threshold)) { - DAP_LOG(storage.log, - "warning: scopes variablesReference threshold reached. " - "current: {} threshold: {}, maximum {}.", - frame_var_ref.AsUInt32(), - var_ref_t::k_variables_reference_threshold, - var_ref_t::k_max_variables_references); - } +private: + void LoadVariables() { + if (m_variables_loaded) + return; - if (LLVM_UNLIKELY(frame_var_ref.Kind() == eReferenceKindInvalid)) + m_variables_loaded = true; + switch (m_kind) { + case eScopeKindLocals: + m_children = m_frame.GetVariables(/*arguments=*/true, + /*locals=*/true, + /*statics=*/false, + /*in_scope_only=*/true); break; - - variables.emplace_back(CreateVariable( - variable, frame_var_ref, format_hex, config.enableAutoVariableSummaries, - config.enableSyntheticChildDebugging, - variable_name_counts[GetNonNullVariableName(variable)] > 1)); - } - return variables; -} - -lldb::SBValue ScopeStore::FindVariable(llvm::StringRef name) { - LoadVariables(); - - lldb::SBValue variable; - const bool is_name_duplicated = name.contains(" @"); - // variablesReference is one of our scopes, not an actual variable it is - // asking for a variable in locals or globals or registers. - const uint32_t end_idx = m_children.GetSize(); - // Searching backward so that we choose the variable in closest scope - // among variables of the same name. - for (const uint32_t i : llvm::reverse(llvm::seq<uint32_t>(0, end_idx))) { - lldb::SBValue curr_variable = m_children.GetValueAtIndex(i); - std::string variable_name = - CreateUniqueVariableNameForDisplay(curr_variable, is_name_duplicated); - if (variable_name == name) { - variable = curr_variable; + case eScopeKindGlobals: + m_children = m_frame.GetVariables(/*arguments=*/false, + /*locals=*/false, + /*statics=*/true, + /*in_scope_only=*/true); break; + case eScopeKindRegisters: + m_children = m_frame.GetRegisters(); } } - return variable; -} -void ScopeStore::LoadVariables() { - if (m_variables_loaded) - return; - - m_variables_loaded = true; - switch (m_kind) { - case eScopeKindLocals: - m_children = m_frame.GetVariables(/*arguments=*/true, - /*locals=*/true, - /*statics=*/false, - /*in_scope_only=*/true); - break; - case eScopeKindGlobals: - m_children = m_frame.GetVariables(/*arguments=*/false, - /*locals=*/false, - /*statics=*/true, - /*in_scope_only=*/true); - break; - case eScopeKindRegisters: - m_children = m_frame.GetRegisters(); + void SetRegistersFormat() { + // Change the default format of any pointer sized registers in the first + // register set to be the lldb::eFormatAddressInfo so we show the pointer + // and resolve what the pointer resolves to. Only change the format if the + // format was set to the default format or if it was hex as some registers + // have formats set for them. + const uint32_t addr_size = + m_frame.GetThread().GetProcess().GetAddressByteSize(); + for (lldb::SBValue reg : m_children.GetValueAtIndex(0)) { + const lldb::Format format = reg.GetFormat(); + if (format == lldb::eFormatDefault || format == lldb::eFormatHex) { + if (reg.GetByteSize() == addr_size) + reg.SetFormat(lldb::eFormatAddressInfo); + } + } } -} -void ScopeStore::SetRegistersFormat() { - // Change the default format of any pointer sized registers in the first - // register set to be the lldb::eFormatAddressInfo so we show the pointer - // and resolve what the pointer resolves to. Only change the format if the - // format was set to the default format or if it was hex as some registers - // have formats set for them. - const uint32_t addr_size = - m_frame.GetThread().GetProcess().GetAddressByteSize(); - for (lldb::SBValue reg : m_children.GetValueAtIndex(0)) { - const lldb::Format format = reg.GetFormat(); - if (format == lldb::eFormatDefault || format == lldb::eFormatHex) { - if (reg.GetByteSize() == addr_size) - reg.SetFormat(lldb::eFormatAddressInfo); + void AddReturnValue(VariableReferenceStorage &storage, + const protocol::Configuration &config, + std::vector<protocol::Variable> &variables, + bool format_hex) { + assert(m_kind == eScopeKindLocals && + "Return Value Should only be in local scope"); + if (m_children.GetSize() == 0) { + // Check for an error in the SBValueList that might explain why we don't + // have locals. If we have an error display it as the sole value in the + // the locals. + + // "error" owns the error string so we must keep it alive as long as we + // want to use the returns "const char *". + lldb::SBError error = m_children.GetError(); + if (const char *var_err = error.GetCString()) { + // Create a fake variable named "error" to explain why variables were + // not available. This new error will help let users know when there was + // a problem that kept variables from being available for display and + // allow users to fix this issue instead of seeing no variables. The + // errors are only set when there is a problem that the user could + // fix, so no error will show up when you have no debug info, only when + // we do have debug info and something that is fixable can be done. + Variable err_var; + err_var.name = "<error>"; + err_var.type = "const char *"; + err_var.value = var_err; + variables.push_back(std::move(err_var)); + } + return; } - } -} -void ScopeStore::AddReturnValue(VariableReferenceStorage &storage, - const Configuration &config, - std::vector<Variable> &variables, - bool format_hex) { - assert(m_kind == eScopeKindLocals && - "Return Value Should only be in local scope"); - if (m_children.GetSize() == 0) { - // Check for an error in the SBValueList that might explain why we don't - // have locals. If we have an error display it as the sole value in the - // the locals. - - // "error" owns the error string so we must keep it alive as long as we - // want to use the returns "const char *". - lldb::SBError error = m_children.GetError(); - if (const char *var_err = error.GetCString()) { - // Create a fake variable named "error" to explain why variables were - // not available. This new error will help let users know when there was - // a problem that kept variables from being available for display and - // allow users to fix this issue instead of seeing no variables. The - // errors are only set when there is a problem that the user could - // fix, so no error will show up when you have no debug info, only when - // we do have debug info and something that is fixable can be done. - Variable err_var; - err_var.name = "<error>"; - err_var.type = "const char *"; - err_var.value = var_err; - variables.push_back(std::move(err_var)); + // Show return value if there is any ( in the local top frame ) + lldb::SBThread selected_thread = m_frame.GetThread(); + lldb::SBValue stop_return_value = selected_thread.GetStopReturnValue(); + + if (stop_return_value.IsValid() && + (selected_thread.GetSelectedFrame().GetFrameID() == 0)) { + auto renamed_return_value = stop_return_value.Clone("(Return Value)"); + var_ref_t return_var_ref{var_ref_t::k_no_child}; + + if (stop_return_value.MightHaveChildren() || + stop_return_value.IsSynthetic()) { + return_var_ref = storage.Insert(stop_return_value, + /*is_permanent=*/false); + } + variables.emplace_back( + CreateVariable(renamed_return_value, return_var_ref, format_hex, + config.enableAutoVariableSummaries, + config.enableSyntheticChildDebugging, false)); } - return; } - // Show return value if there is any ( in the local top frame ) - lldb::SBThread selected_thread = m_frame.GetThread(); - lldb::SBValue stop_return_value = selected_thread.GetStopReturnValue(); + lldb::SBFrame m_frame; + lldb::SBValueList m_children; + ScopeKind m_kind; + bool m_variables_loaded = false; +}; + +/// Variable store for expandable values. +/// +/// Manages children variables of complex types (structs, arrays, pointers, +/// etc.) that can be expanded in the debugger UI. +class ExpandableValueStore final : public VariableStore { + +public: + explicit ExpandableValueStore(const lldb::SBValue &value) : m_value(value) {} + + std::vector<protocol::Variable> + GetVariables(VariableReferenceStorage &storage, + const protocol::Configuration &config, + const protocol::VariablesArguments &args) override { + const var_ref_t var_ref = args.variablesReference; + const uint64_t count = args.count; + const uint64_t start = args.start; + const bool hex = args.format ? args.format->hex : false; + + lldb::SBValue variable = storage.GetVariable(var_ref); + if (!variable) + return {}; + + // We are expanding a variable that has children, so we will return its + // children. + ... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/183176 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
