Skip to content

Reapply PR/87550 #94625

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jun 7, 2024
Merged

Reapply PR/87550 #94625

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bbaa8ef
Reapply "[lldb][lldb-dap] Cleanup breakpoint filters." (#93739)
oontvoo Jun 6, 2024
33edc9e
removed assertion checks
oontvoo Jun 6, 2024
e16b56a
typo
oontvoo Jun 6, 2024
92d09cd
added ns
oontvoo Jun 6, 2024
38cd558
wrap the population of exception_breakpoints in a call_once for safety
oontvoo Jun 7, 2024
9df5a14
remove redundant check
oontvoo Jun 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lldb/include/lldb/API/SBDebugger.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ class LLDB_API SBDebugger {

static const char *GetBroadcasterClass();

static bool SupportsLanguage(lldb::LanguageType language);

lldb::SBBroadcaster GetBroadcaster();

/// Get progress data from a SBEvent whose type is eBroadcastBitProgress.
Expand Down
1 change: 1 addition & 0 deletions lldb/include/lldb/Symbol/TypeSystem.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ class TypeSystem : public PluginInterface,
// TypeSystems can support more than one language
virtual bool SupportsLanguage(lldb::LanguageType language) = 0;

static bool SupportsLanguageStatic(lldb::LanguageType language);
// Type Completion

virtual bool GetCompleteType(lldb::opaque_compiler_type_t type) = 0;
Expand Down
4 changes: 4 additions & 0 deletions lldb/source/API/SBDebugger.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1742,3 +1742,7 @@ bool SBDebugger::InterruptRequested() {
return m_opaque_sp->InterruptRequested();
return false;
}

bool SBDebugger::SupportsLanguage(lldb::LanguageType language) {
return TypeSystem::SupportsLanguageStatic(language);
}
11 changes: 11 additions & 0 deletions lldb/source/Symbol/TypeSystem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,3 +335,14 @@ TypeSystemMap::GetTypeSystemForLanguage(lldb::LanguageType language,
}
return GetTypeSystemForLanguage(language);
}

bool TypeSystem::SupportsLanguageStatic(lldb::LanguageType language) {
if (language == eLanguageTypeUnknown)
return false;

LanguageSet languages =
PluginManager::GetAllTypeSystemSupportedLanguagesForTypes();
if (languages.Empty())
return false;
return languages[language];
}
57 changes: 47 additions & 10 deletions lldb/tools/lldb-dap/DAP.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,7 @@ namespace lldb_dap {
DAP g_dap;

DAP::DAP()
: broadcaster("lldb-dap"),
exception_breakpoints(
{{"cpp_catch", "C++ Catch", lldb::eLanguageTypeC_plus_plus},
{"cpp_throw", "C++ Throw", lldb::eLanguageTypeC_plus_plus},
{"objc_catch", "Objective-C Catch", lldb::eLanguageTypeObjC},
{"objc_throw", "Objective-C Throw", lldb::eLanguageTypeObjC},
{"swift_catch", "Swift Catch", lldb::eLanguageTypeSwift},
{"swift_throw", "Swift Throw", lldb::eLanguageTypeSwift}}),
: broadcaster("lldb-dap"), exception_breakpoints(),
focus_tid(LLDB_INVALID_THREAD_ID), stop_at_entry(false), is_attach(false),
enable_auto_variable_summaries(false),
enable_synthetic_child_debugging(false),
Expand All @@ -65,16 +58,60 @@ DAP::DAP()

DAP::~DAP() = default;

void DAP::PopulateExceptionBreakpoints() {
exception_breakpoints = {};
if (lldb::SBDebugger::SupportsLanguage(lldb::eLanguageTypeC_plus_plus)) {
exception_breakpoints->emplace_back("cpp_catch", "C++ Catch",
lldb::eLanguageTypeC_plus_plus);
exception_breakpoints->emplace_back("cpp_throw", "C++ Throw",
lldb::eLanguageTypeC_plus_plus);
}
if (lldb::SBDebugger::SupportsLanguage(lldb::eLanguageTypeObjC)) {
exception_breakpoints->emplace_back("objc_catch", "Objective-C Catch",
lldb::eLanguageTypeObjC);
exception_breakpoints->emplace_back("objc_throw", "Objective-C Throw",
lldb::eLanguageTypeObjC);
}
if (lldb::SBDebugger::SupportsLanguage(lldb::eLanguageTypeSwift)) {
exception_breakpoints->emplace_back("swift_catch", "Swift Catch",
lldb::eLanguageTypeSwift);
exception_breakpoints->emplace_back("swift_throw", "Swift Throw",
lldb::eLanguageTypeSwift);
}
}

ExceptionBreakpoint *DAP::GetExceptionBreakpoint(const std::string &filter) {
for (auto &bp : exception_breakpoints) {
// PopulateExceptionBreakpoints() is called after g_dap.debugger is created
// in a request-initialize.
//
// But this GetExceptionBreakpoint() method may be called before attaching, in
// which case, we may not have populated the filter yet.
//
// We also cannot call PopulateExceptionBreakpoints() in DAP::DAP() because
// we need SBDebugger::Initialize() to have been called before this.
//
// So just checking the filter list and do lazy-populating seems easiest.
// Two other options include:
// + call g_dap.PopulateExceptionBreakpoints() in lldb-dap.cpp::main()
// right after the call to SBDebugger::Initialize()
// + Just call PopulateExceptionBreakpoints() to get a fresh list everytime
// we query (a bit overkill since it's not likely to change?)
if (!exception_breakpoints.has_value())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could avoid having to duplicate this check (and the comment) by hoisting it into PopulateExceptionBreakpoints and call it unconditionally here.

You can keep an assert to convey the precondition is that PopulateExceptionBreakpoints must have been called:

assert(exception_breakpoints && "PopulateExceptionBreakpoints must be called")

I'm not familiar enough with the code to know if there's a risk of racing on that variable, but if I was implementing the lazy approach, I would wrap the thing into a call_once. If there's no risk of multiple threads calling GetExceptionBreakpoint then this might be overkill and checking the optional is sufficient and marginally more efficient.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I've removed the if() check and wrap the block of code that populates exception_breakpoints in a call_once so the callers of PopulateExceptionBreakpoints don't have to do that

PopulateExceptionBreakpoints();

for (auto &bp : *exception_breakpoints) {
if (bp.filter == filter)
return &bp;
}
return nullptr;
}

ExceptionBreakpoint *DAP::GetExceptionBreakpoint(const lldb::break_id_t bp_id) {
for (auto &bp : exception_breakpoints) {
// See comment in the other GetExceptionBreakpoint().
if (!exception_breakpoints.has_value())
PopulateExceptionBreakpoints();

for (auto &bp : *exception_breakpoints) {
if (bp.bp.GetID() == bp_id)
return &bp;
}
Expand Down
4 changes: 3 additions & 1 deletion lldb/tools/lldb-dap/DAP.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ struct DAP {
std::unique_ptr<std::ofstream> log;
llvm::StringMap<SourceBreakpointMap> source_breakpoints;
FunctionBreakpointMap function_breakpoints;
std::vector<ExceptionBreakpoint> exception_breakpoints;
std::optional<std::vector<ExceptionBreakpoint>> exception_breakpoints;
std::vector<std::string> init_commands;
std::vector<std::string> pre_run_commands;
std::vector<std::string> post_run_commands;
Expand Down Expand Up @@ -228,6 +228,8 @@ struct DAP {

llvm::json::Value CreateTopLevelScopes();

void PopulateExceptionBreakpoints();

/// \return
/// Attempt to determine if an expression is a variable expression or
/// lldb command using a hueristic based on the first term of the
Expand Down
6 changes: 4 additions & 2 deletions lldb/tools/lldb-dap/lldb-dap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include <cstdio>
#include <cstdlib>
#include <cstring>
#include <optional>
#include <sys/stat.h>
#include <sys/types.h>
#if defined(_WIN32)
Expand Down Expand Up @@ -1586,6 +1587,7 @@ void request_initialize(const llvm::json::Object &request) {
bool source_init_file = GetBoolean(arguments, "sourceInitFile", true);

g_dap.debugger = lldb::SBDebugger::Create(source_init_file, log_cb, nullptr);
g_dap.PopulateExceptionBreakpoints();
auto cmd = g_dap.debugger.GetCommandInterpreter().AddMultiwordCommand(
"lldb-dap", "Commands for managing lldb-dap.");
if (GetBoolean(arguments, "supportsStartDebuggingRequest", false)) {
Expand Down Expand Up @@ -1621,7 +1623,7 @@ void request_initialize(const llvm::json::Object &request) {
body.try_emplace("supportsEvaluateForHovers", true);
// Available filters or options for the setExceptionBreakpoints request.
llvm::json::Array filters;
for (const auto &exc_bp : g_dap.exception_breakpoints) {
for (const auto &exc_bp : *g_dap.exception_breakpoints) {
filters.emplace_back(CreateExceptionBreakpointFilter(exc_bp));
}
body.try_emplace("exceptionBreakpointFilters", std::move(filters));
Expand Down Expand Up @@ -2476,7 +2478,7 @@ void request_setExceptionBreakpoints(const llvm::json::Object &request) {
// Keep a list of any exception breakpoint filter names that weren't set
// so we can clear any exception breakpoints if needed.
std::set<std::string> unset_filters;
for (const auto &bp : g_dap.exception_breakpoints)
for (const auto &bp : *g_dap.exception_breakpoints)
unset_filters.insert(bp.filter);

for (const auto &value : *filters) {
Expand Down