llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Zequan Wu (ZequanWu) <details> <summary>Changes</summary> This adds a layer between `SounceBreakpoint`/`FunctionBreakpoint` and `BreakpointBase` to have better separation and encapsulation so we are not directly operating on `SBBreakpoint`. I basically moved the `SBBreakpoint` and the methods that requires it from `BreakpointBase` to `Breakpoint`. This allows adding support for data watchpoint easier by sharing the logic inside `BreakpointBase`. --- Patch is 27.36 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/80753.diff 13 Files Affected: - (added) lldb/tools/lldb-dap/Breakpoint.cpp (+182) - (added) lldb/tools/lldb-dap/Breakpoint.h (+34) - (modified) lldb/tools/lldb-dap/BreakpointBase.cpp (-113) - (modified) lldb/tools/lldb-dap/BreakpointBase.h (+6-6) - (modified) lldb/tools/lldb-dap/CMakeLists.txt (+1) - (modified) lldb/tools/lldb-dap/FunctionBreakpoint.cpp (+2-10) - (modified) lldb/tools/lldb-dap/FunctionBreakpoint.h (+2-2) - (modified) lldb/tools/lldb-dap/JSONUtils.cpp (+3-43) - (modified) lldb/tools/lldb-dap/JSONUtils.h (+3-2) - (modified) lldb/tools/lldb-dap/SourceBreakpoint.cpp (+2-10) - (modified) lldb/tools/lldb-dap/SourceBreakpoint.h (+3-3) - (modified) lldb/tools/lldb-dap/lldb-dap.cpp (+9-8) - (modified) llvm/utils/gn/secondary/lldb/tools/lldb-dap/BUILD.gn (+1) ``````````diff diff --git a/lldb/tools/lldb-dap/Breakpoint.cpp b/lldb/tools/lldb-dap/Breakpoint.cpp new file mode 100644 index 0000000000000..4ccf353b06ccc --- /dev/null +++ b/lldb/tools/lldb-dap/Breakpoint.cpp @@ -0,0 +1,182 @@ +//===-- Breakpoint.cpp ------------------------------------------*- C++ -*-===// +// +// 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 "Breakpoint.h" +#include "DAP.h" +#include "JSONUtils.h" +#include "llvm/ADT/StringExtras.h" + +using namespace lldb_dap; + +void Breakpoint::SetCondition() { bp.SetCondition(condition.c_str()); } + +void Breakpoint::SetHitCondition() { + uint64_t hitCount = 0; + if (llvm::to_integer(hitCondition, hitCount)) + bp.SetIgnoreCount(hitCount - 1); +} + +// logMessage will be divided into array of LogMessagePart as two kinds: +// 1. raw print text message, and +// 2. interpolated expression for evaluation which is inside matching curly +// braces. +// +// The function tries to parse logMessage into a list of LogMessageParts +// for easy later access in BreakpointHitCallback. +void Breakpoint::SetLogMessage() { + logMessageParts.clear(); + + // Contains unmatched open curly braces indices. + std::vector<int> unmatched_curly_braces; + + // Contains all matched curly braces in logMessage. + // Loop invariant: matched_curly_braces_ranges are sorted by start index in + // ascending order without any overlap between them. + std::vector<std::pair<int, int>> matched_curly_braces_ranges; + + lldb::SBError error; + // Part1 - parse matched_curly_braces_ranges. + // locating all curly braced expression ranges in logMessage. + // The algorithm takes care of nested and imbalanced curly braces. + for (size_t i = 0; i < logMessage.size(); ++i) { + if (logMessage[i] == '{') { + unmatched_curly_braces.push_back(i); + } else if (logMessage[i] == '}') { + if (unmatched_curly_braces.empty()) + // Nothing to match. + continue; + + int last_unmatched_index = unmatched_curly_braces.back(); + unmatched_curly_braces.pop_back(); + + // Erase any matched ranges included in the new match. + while (!matched_curly_braces_ranges.empty()) { + assert(matched_curly_braces_ranges.back().first != + last_unmatched_index && + "How can a curley brace be matched twice?"); + if (matched_curly_braces_ranges.back().first < last_unmatched_index) + break; + + // This is a nested range let's earse it. + assert((size_t)matched_curly_braces_ranges.back().second < i); + matched_curly_braces_ranges.pop_back(); + } + + // Assert invariant. + assert(matched_curly_braces_ranges.empty() || + matched_curly_braces_ranges.back().first < last_unmatched_index); + matched_curly_braces_ranges.emplace_back(last_unmatched_index, i); + } + } + + // Part2 - parse raw text and expresions parts. + // All expression ranges have been parsed in matched_curly_braces_ranges. + // The code below uses matched_curly_braces_ranges to divide logMessage + // into raw text parts and expression parts. + int last_raw_text_start = 0; + for (const std::pair<int, int> &curly_braces_range : + matched_curly_braces_ranges) { + // Raw text before open curly brace. + assert(curly_braces_range.first >= last_raw_text_start); + size_t raw_text_len = curly_braces_range.first - last_raw_text_start; + if (raw_text_len > 0) { + error = AppendLogMessagePart( + llvm::StringRef(logMessage.c_str() + last_raw_text_start, + raw_text_len), + /*is_expr=*/false); + if (error.Fail()) { + NotifyLogMessageError(error.GetCString()); + return; + } + } + + // Expression between curly braces. + assert(curly_braces_range.second > curly_braces_range.first); + size_t expr_len = curly_braces_range.second - curly_braces_range.first - 1; + error = AppendLogMessagePart( + llvm::StringRef(logMessage.c_str() + curly_braces_range.first + 1, + expr_len), + /*is_expr=*/true); + if (error.Fail()) { + NotifyLogMessageError(error.GetCString()); + return; + } + + last_raw_text_start = curly_braces_range.second + 1; + } + // Trailing raw text after close curly brace. + assert(last_raw_text_start >= 0); + if (logMessage.size() > (size_t)last_raw_text_start) { + error = AppendLogMessagePart( + llvm::StringRef(logMessage.c_str() + last_raw_text_start, + logMessage.size() - last_raw_text_start), + /*is_expr=*/false); + if (error.Fail()) { + NotifyLogMessageError(error.GetCString()); + return; + } + } + + bp.SetCallback(BreakpointBase::BreakpointHitCallback, this); +} + +void Breakpoint::CreateJsonObject(llvm::json::Object &object) { + // Each breakpoint location is treated as a separate breakpoint for VS code. + // They don't have the notion of a single breakpoint with multiple locations. + if (!bp.IsValid()) + return; + object.try_emplace("verified", bp.GetNumResolvedLocations() > 0); + object.try_emplace("id", bp.GetID()); + // VS Code DAP doesn't currently allow one breakpoint to have multiple + // locations so we just report the first one. If we report all locations + // then the IDE starts showing the wrong line numbers and locations for + // other source file and line breakpoints in the same file. + + // Below we search for the first resolved location in a breakpoint and report + // this as the breakpoint location since it will have a complete location + // that is at least loaded in the current process. + lldb::SBBreakpointLocation bp_loc; + const auto num_locs = bp.GetNumLocations(); + for (size_t i = 0; i < num_locs; ++i) { + bp_loc = bp.GetLocationAtIndex(i); + if (bp_loc.IsResolved()) + break; + } + // If not locations are resolved, use the first location. + if (!bp_loc.IsResolved()) + bp_loc = bp.GetLocationAtIndex(0); + auto bp_addr = bp_loc.GetAddress(); + + if (bp_addr.IsValid()) { + std::string formatted_addr = + "0x" + llvm::utohexstr(bp_addr.GetLoadAddress(g_dap.target)); + object.try_emplace("instructionReference", formatted_addr); + auto line_entry = bp_addr.GetLineEntry(); + const auto line = line_entry.GetLine(); + if (line != UINT32_MAX) + object.try_emplace("line", line); + const auto column = line_entry.GetColumn(); + if (column != 0) + object.try_emplace("column", column); + object.try_emplace("source", CreateSource(line_entry)); + } +} + +bool Breakpoint::MatchesName(const char *name) { return bp.MatchesName(name); } + +void Breakpoint::SetBreakpoint() { + // See comments in BreakpointBase::GetBreakpointLabel() for details of why + // we add a label to our breakpoints. + bp.AddName(GetBreakpointLabel()); + if (!condition.empty()) + SetCondition(); + if (!hitCondition.empty()) + SetHitCondition(); + if (!logMessage.empty()) + SetLogMessage(); +} diff --git a/lldb/tools/lldb-dap/Breakpoint.h b/lldb/tools/lldb-dap/Breakpoint.h new file mode 100644 index 0000000000000..a668e29f3d1e8 --- /dev/null +++ b/lldb/tools/lldb-dap/Breakpoint.h @@ -0,0 +1,34 @@ +//===-- Breakpoint.h --------------------------------------------*- C++ -*-===// +// +// 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_BREAKPOINT_H +#define LLDB_TOOLS_LLDB_DAP_BREAKPOINT_H + +#include "BreakpointBase.h" + +namespace lldb_dap { + +struct Breakpoint : public BreakpointBase { + // The LLDB breakpoint associated wit this source breakpoint + lldb::SBBreakpoint bp; + + Breakpoint() = default; + Breakpoint(const llvm::json::Object &obj) : BreakpointBase(obj){}; + Breakpoint(lldb::SBBreakpoint bp): bp(bp) {} + + void SetCondition() override; + void SetHitCondition() override; + void SetLogMessage() override; + void CreateJsonObject(llvm::json::Object &object) override; + + bool MatchesName(const char *name); + void SetBreakpoint(); +}; +} // namespace lldb_dap + +#endif diff --git a/lldb/tools/lldb-dap/BreakpointBase.cpp b/lldb/tools/lldb-dap/BreakpointBase.cpp index fb4b27fbe315f..088ccc6b27f8e 100644 --- a/lldb/tools/lldb-dap/BreakpointBase.cpp +++ b/lldb/tools/lldb-dap/BreakpointBase.cpp @@ -8,7 +8,6 @@ #include "BreakpointBase.h" #include "DAP.h" -#include "JSONUtils.h" #include "llvm/ADT/StringExtras.h" using namespace lldb_dap; @@ -18,14 +17,6 @@ BreakpointBase::BreakpointBase(const llvm::json::Object &obj) hitCondition(std::string(GetString(obj, "hitCondition"))), logMessage(std::string(GetString(obj, "logMessage"))) {} -void BreakpointBase::SetCondition() { bp.SetCondition(condition.c_str()); } - -void BreakpointBase::SetHitCondition() { - uint64_t hitCount = 0; - if (llvm::to_integer(hitCondition, hitCount)) - bp.SetIgnoreCount(hitCount - 1); -} - lldb::SBError BreakpointBase::AppendLogMessagePart(llvm::StringRef part, bool is_expr) { if (is_expr) { @@ -164,110 +155,6 @@ lldb::SBError BreakpointBase::FormatLogText(llvm::StringRef text, return error; } -// logMessage will be divided into array of LogMessagePart as two kinds: -// 1. raw print text message, and -// 2. interpolated expression for evaluation which is inside matching curly -// braces. -// -// The function tries to parse logMessage into a list of LogMessageParts -// for easy later access in BreakpointHitCallback. -void BreakpointBase::SetLogMessage() { - logMessageParts.clear(); - - // Contains unmatched open curly braces indices. - std::vector<int> unmatched_curly_braces; - - // Contains all matched curly braces in logMessage. - // Loop invariant: matched_curly_braces_ranges are sorted by start index in - // ascending order without any overlap between them. - std::vector<std::pair<int, int>> matched_curly_braces_ranges; - - lldb::SBError error; - // Part1 - parse matched_curly_braces_ranges. - // locating all curly braced expression ranges in logMessage. - // The algorithm takes care of nested and imbalanced curly braces. - for (size_t i = 0; i < logMessage.size(); ++i) { - if (logMessage[i] == '{') { - unmatched_curly_braces.push_back(i); - } else if (logMessage[i] == '}') { - if (unmatched_curly_braces.empty()) - // Nothing to match. - continue; - - int last_unmatched_index = unmatched_curly_braces.back(); - unmatched_curly_braces.pop_back(); - - // Erase any matched ranges included in the new match. - while (!matched_curly_braces_ranges.empty()) { - assert(matched_curly_braces_ranges.back().first != - last_unmatched_index && - "How can a curley brace be matched twice?"); - if (matched_curly_braces_ranges.back().first < last_unmatched_index) - break; - - // This is a nested range let's earse it. - assert((size_t)matched_curly_braces_ranges.back().second < i); - matched_curly_braces_ranges.pop_back(); - } - - // Assert invariant. - assert(matched_curly_braces_ranges.empty() || - matched_curly_braces_ranges.back().first < last_unmatched_index); - matched_curly_braces_ranges.emplace_back(last_unmatched_index, i); - } - } - - // Part2 - parse raw text and expresions parts. - // All expression ranges have been parsed in matched_curly_braces_ranges. - // The code below uses matched_curly_braces_ranges to divide logMessage - // into raw text parts and expression parts. - int last_raw_text_start = 0; - for (const std::pair<int, int> &curly_braces_range : - matched_curly_braces_ranges) { - // Raw text before open curly brace. - assert(curly_braces_range.first >= last_raw_text_start); - size_t raw_text_len = curly_braces_range.first - last_raw_text_start; - if (raw_text_len > 0) { - error = AppendLogMessagePart( - llvm::StringRef(logMessage.c_str() + last_raw_text_start, - raw_text_len), - /*is_expr=*/false); - if (error.Fail()) { - NotifyLogMessageError(error.GetCString()); - return; - } - } - - // Expression between curly braces. - assert(curly_braces_range.second > curly_braces_range.first); - size_t expr_len = curly_braces_range.second - curly_braces_range.first - 1; - error = AppendLogMessagePart( - llvm::StringRef(logMessage.c_str() + curly_braces_range.first + 1, - expr_len), - /*is_expr=*/true); - if (error.Fail()) { - NotifyLogMessageError(error.GetCString()); - return; - } - - last_raw_text_start = curly_braces_range.second + 1; - } - // Trailing raw text after close curly brace. - assert(last_raw_text_start >= 0); - if (logMessage.size() > (size_t)last_raw_text_start) { - error = AppendLogMessagePart( - llvm::StringRef(logMessage.c_str() + last_raw_text_start, - logMessage.size() - last_raw_text_start), - /*is_expr=*/false); - if (error.Fail()) { - NotifyLogMessageError(error.GetCString()); - return; - } - } - - bp.SetCallback(BreakpointBase::BreakpointHitCallback, this); -} - void BreakpointBase::NotifyLogMessageError(llvm::StringRef error) { std::string message = "Log message has error: "; message += error; diff --git a/lldb/tools/lldb-dap/BreakpointBase.h b/lldb/tools/lldb-dap/BreakpointBase.h index 41787f7861021..a4184bfe65d27 100644 --- a/lldb/tools/lldb-dap/BreakpointBase.h +++ b/lldb/tools/lldb-dap/BreakpointBase.h @@ -9,7 +9,6 @@ #ifndef LLDB_TOOLS_LLDB_DAP_BREAKPOINTBASE_H #define LLDB_TOOLS_LLDB_DAP_BREAKPOINTBASE_H -#include "JSONUtils.h" #include "lldb/API/SBBreakpoint.h" #include "llvm/Support/JSON.h" #include <string> @@ -35,15 +34,16 @@ struct BreakpointBase { // interpolated. std::string logMessage; std::vector<LogMessagePart> logMessageParts; - // The LLDB breakpoint associated wit this source breakpoint - lldb::SBBreakpoint bp; BreakpointBase() = default; BreakpointBase(const llvm::json::Object &obj); + virtual ~BreakpointBase() = default; + + virtual void SetCondition() = 0; + virtual void SetHitCondition() = 0; + virtual void SetLogMessage() = 0; + virtual void CreateJsonObject(llvm::json::Object &object) = 0; - void SetCondition(); - void SetHitCondition(); - void SetLogMessage(); void UpdateBreakpoint(const BreakpointBase &request_bp); // Format \param text and return formatted text in \param formatted. diff --git a/lldb/tools/lldb-dap/CMakeLists.txt b/lldb/tools/lldb-dap/CMakeLists.txt index 554567eb3b0e2..f8c0e4ecf36c2 100644 --- a/lldb/tools/lldb-dap/CMakeLists.txt +++ b/lldb/tools/lldb-dap/CMakeLists.txt @@ -24,6 +24,7 @@ tablegen(LLVM Options.inc -gen-opt-parser-defs) add_public_tablegen_target(LLDBDAPOptionsTableGen) add_lldb_tool(lldb-dap lldb-dap.cpp + Breakpoint.cpp BreakpointBase.cpp ExceptionBreakpoint.cpp FifoFiles.cpp diff --git a/lldb/tools/lldb-dap/FunctionBreakpoint.cpp b/lldb/tools/lldb-dap/FunctionBreakpoint.cpp index d4bdb976500ec..21743bf908706 100644 --- a/lldb/tools/lldb-dap/FunctionBreakpoint.cpp +++ b/lldb/tools/lldb-dap/FunctionBreakpoint.cpp @@ -12,21 +12,13 @@ namespace lldb_dap { FunctionBreakpoint::FunctionBreakpoint(const llvm::json::Object &obj) - : BreakpointBase(obj), functionName(std::string(GetString(obj, "name"))) {} + : Breakpoint(obj), functionName(std::string(GetString(obj, "name"))) {} void FunctionBreakpoint::SetBreakpoint() { if (functionName.empty()) return; bp = g_dap.target.BreakpointCreateByName(functionName.c_str()); - // See comments in BreakpointBase::GetBreakpointLabel() for details of why - // we add a label to our breakpoints. - bp.AddName(GetBreakpointLabel()); - if (!condition.empty()) - SetCondition(); - if (!hitCondition.empty()) - SetHitCondition(); - if (!logMessage.empty()) - SetLogMessage(); + Breakpoint::SetBreakpoint(); } } // namespace lldb_dap diff --git a/lldb/tools/lldb-dap/FunctionBreakpoint.h b/lldb/tools/lldb-dap/FunctionBreakpoint.h index fc23e94e12876..b15ff1931a6b2 100644 --- a/lldb/tools/lldb-dap/FunctionBreakpoint.h +++ b/lldb/tools/lldb-dap/FunctionBreakpoint.h @@ -9,11 +9,11 @@ #ifndef LLDB_TOOLS_LLDB_DAP_FUNCTIONBREAKPOINT_H #define LLDB_TOOLS_LLDB_DAP_FUNCTIONBREAKPOINT_H -#include "BreakpointBase.h" +#include "Breakpoint.h" namespace lldb_dap { -struct FunctionBreakpoint : public BreakpointBase { +struct FunctionBreakpoint : public Breakpoint { std::string functionName; FunctionBreakpoint() = default; diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp index a8b438d9d6df3..878449a91aa66 100644 --- a/lldb/tools/lldb-dap/JSONUtils.cpp +++ b/lldb/tools/lldb-dap/JSONUtils.cpp @@ -364,54 +364,14 @@ llvm::json::Value CreateScope(const llvm::StringRef name, // }, // "required": [ "verified" ] // } -llvm::json::Value CreateBreakpoint(lldb::SBBreakpoint &bp, +llvm::json::Value CreateBreakpoint(BreakpointBase *bp, std::optional<llvm::StringRef> request_path, std::optional<uint32_t> request_line, std::optional<uint32_t> request_column) { - // Each breakpoint location is treated as a separate breakpoint for VS code. - // They don't have the notion of a single breakpoint with multiple locations. llvm::json::Object object; - if (!bp.IsValid()) - return llvm::json::Value(std::move(object)); - - object.try_emplace("verified", bp.GetNumResolvedLocations() > 0); - object.try_emplace("id", bp.GetID()); - // VS Code DAP doesn't currently allow one breakpoint to have multiple - // locations so we just report the first one. If we report all locations - // then the IDE starts showing the wrong line numbers and locations for - // other source file and line breakpoints in the same file. - - // Below we search for the first resolved location in a breakpoint and report - // this as the breakpoint location since it will have a complete location - // that is at least loaded in the current process. - lldb::SBBreakpointLocation bp_loc; - const auto num_locs = bp.GetNumLocations(); - for (size_t i = 0; i < num_locs; ++i) { - bp_loc = bp.GetLocationAtIndex(i); - if (bp_loc.IsResolved()) - break; - } - // If not locations are resolved, use the first location. - if (!bp_loc.IsResolved()) - bp_loc = bp.GetLocationAtIndex(0); - auto bp_addr = bp_loc.GetAddress(); - if (request_path) object.try_emplace("source", CreateSource(*request_path)); - - if (bp_addr.IsValid()) { - std::string formatted_addr = - "0x" + llvm::utohexstr(bp_addr.GetLoadAddress(g_dap.target)); - object.try_emplace("instructionReference", formatted_addr); - auto line_entry = bp_addr.GetLineEntry(); - const auto line = line_entry.GetLine(); - if (line != UINT32_MAX) - object.try_emplace("line", line); - const auto column = line_entry.GetColumn(); - if (column != 0) - object.try_emplace("column", column); - object.try_emplace("source", CreateSource(line_entry)); - } + bp->CreateJsonObject(object); // We try to add request_line as a fallback if (request_line) object.try_emplace("line", *request_line); @@ -506,7 +466,7 @@ llvm::json::Value CreateModule(lldb::SBModule &module) { return llvm::json::Value(std::move(object)); } -void AppendBreakpoint(lldb::SBBreakpoint &bp, llvm::json::Array &breakpoints, +void AppendBreakpoint(BreakpointBase *bp, llvm::json::Array &breakpoints, std::optional<llvm::StringRef> request_path, std::optional<uint32_t> request_line) { breakpoints.emplace_back(CreateBreakpoint(bp, request_path, request_line)); diff --git a/lldb/tools/lldb-dap/JSONUtils.h b/lldb/tools/lldb-dap/JSONUtils.h index 62338548890c0..1515f5ba2e5f4 100644 --- a/lldb/tools/l... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/80753 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
