Reapply PR/87550 (again) #95571
Merged
Reapply PR/87550 (again) #95571
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Vy Nguyen (oontvoo) ChangesNew fixes:
Full diff: https://github.com/llvm/llvm-project/pull/95571.diff 7 Files Affected:
diff --git a/lldb/include/lldb/API/SBDebugger.h b/lldb/include/lldb/API/SBDebugger.h
index af19b1faf3bf5..84ea9c0f772e1 100644
--- a/lldb/include/lldb/API/SBDebugger.h
+++ b/lldb/include/lldb/API/SBDebugger.h
@@ -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.
diff --git a/lldb/include/lldb/Symbol/TypeSystem.h b/lldb/include/lldb/Symbol/TypeSystem.h
index b4025c173a186..7d48f9b316138 100644
--- a/lldb/include/lldb/Symbol/TypeSystem.h
+++ b/lldb/include/lldb/Symbol/TypeSystem.h
@@ -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;
diff --git a/lldb/source/API/SBDebugger.cpp b/lldb/source/API/SBDebugger.cpp
index 7ef0d6efd4aaa..29da7d33dd80b 100644
--- a/lldb/source/API/SBDebugger.cpp
+++ b/lldb/source/API/SBDebugger.cpp
@@ -1742,3 +1742,7 @@ bool SBDebugger::InterruptRequested() {
return m_opaque_sp->InterruptRequested();
return false;
}
+
+bool SBDebugger::SupportsLanguage(lldb::LanguageType language) {
+ return TypeSystem::SupportsLanguageStatic(language);
+}
diff --git a/lldb/source/Symbol/TypeSystem.cpp b/lldb/source/Symbol/TypeSystem.cpp
index 4956f10a0b0a7..931ce1b0203a9 100644
--- a/lldb/source/Symbol/TypeSystem.cpp
+++ b/lldb/source/Symbol/TypeSystem.cpp
@@ -335,3 +335,14 @@ TypeSystemMap::GetTypeSystemForLanguage(lldb::LanguageType language,
}
return GetTypeSystemForLanguage(language);
}
+
+bool TypeSystem::SupportsLanguageStatic(lldb::LanguageType language) {
+ if (language == eLanguageTypeUnknown || language >= eNumLanguageTypes)
+ return false;
+
+ LanguageSet languages =
+ PluginManager::GetAllTypeSystemSupportedLanguagesForTypes();
+ if (languages.Empty())
+ return false;
+ return languages[language];
+}
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index d419f821999e6..e4fb3181a2d99 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -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),
@@ -65,8 +58,54 @@ DAP::DAP()
DAP::~DAP() = default;
+void DAP::PopulateExceptionBreakpoints() {
+ llvm::call_once(initExceptionBreakpoints, [this]() {
+ exception_breakpoints = std::vector<ExceptionBreakpoint> {};
+
+ 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);
+ }
+ assert(exception_breakpoints.has_value() && "should have been initted");
+ assert(!exception_breakpoints->empty() && "should not be empty");
+ });
+}
+
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 calling PopulateExceptionBreakoints(),which does 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?)
+ PopulateExceptionBreakpoints();
+ assert(exception_breakpoints.has_value() &&
+ "exception_breakpoints must have been populated");
+
+ for (auto &bp : *exception_breakpoints) {
if (bp.filter == filter)
return &bp;
}
@@ -74,7 +113,12 @@ ExceptionBreakpoint *DAP::GetExceptionBreakpoint(const std::string &filter) {
}
ExceptionBreakpoint *DAP::GetExceptionBreakpoint(const lldb::break_id_t bp_id) {
- for (auto &bp : exception_breakpoints) {
+ // See comment in the other GetExceptionBreakpoint().
+ PopulateExceptionBreakpoints();
+ assert(exception_breakpoints.has_value() &&
+ "exception_breakpoints must have been populated");
+
+ for (auto &bp : *exception_breakpoints) {
if (bp.bp.GetID() == bp_id)
return &bp;
}
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index a88ee3e1dec6b..daa0d9f1aa7f0 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -156,7 +156,8 @@ 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;
+ llvm::once_flag initExceptionBreakpoints;
std::vector<std::string> init_commands;
std::vector<std::string> pre_run_commands;
std::vector<std::string> post_run_commands;
@@ -228,6 +229,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
diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp
index 7746afb6cbbf3..470c9f84c6a20 100644
--- a/lldb/tools/lldb-dap/lldb-dap.cpp
+++ b/lldb/tools/lldb-dap/lldb-dap.cpp
@@ -16,6 +16,7 @@
#include <cstdio>
#include <cstdlib>
#include <cstring>
+#include <optional>
#include <sys/stat.h>
#include <sys/types.h>
#if defined(_WIN32)
@@ -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)) {
@@ -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));
@@ -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) {
|
|
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
labath
reviewed
Jun 18, 2024
lldb/tools/lldb-dap/DAP.cpp
Outdated
Comment on lines
83
to
84
| assert(exception_breakpoints.has_value() && "should have been initted"); | ||
| assert(!exception_breakpoints->empty() && "should not be empty"); |
There was a problem hiding this comment.
This seems rather excessive. I realize you were just bitten by this, but the assertions basically amount to
int foo = 2+2;
// some code (not too much)
assert(foo == 4);
which we don't usually do.
There was a problem hiding this comment.
The exceptions_breakpoints could be empty if for some reason all of the if-conditions above returned false. That would be unexpected and I think it's easier to catch the bug right here rather than later
There was a problem hiding this comment.
That's the second assertion, not the first one. The first one is really just a test for the implementation of optional::operator=.
I don't have such strong feelings about the second one, although one could easily argue that the fact that lldb says it does not support any of the mentioned languages is not a bug (at least, not a bug in lldb-dap). Correctness of the code following the assertion does not depend on the vector being non-empty (only on its existence), so this feels like it would be better off as a test.
lldb/tools/lldb-dap/DAP.cpp
Outdated
Comment on lines
89
to
106
| // 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 calling PopulateExceptionBreakoints(),which does 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?) | ||
| PopulateExceptionBreakpoints(); | ||
| assert(exception_breakpoints.has_value() && | ||
| "exception_breakpoints must have been populated"); |
There was a problem hiding this comment.
with the lazy call_once initialization, I think that a better interface would be to have the populate function directly return the exception lists that it has created (maybe then it should be called ArrayRef<ExceptionBreakpoint> GetExceptionBreakpoints()). Then there would be nothing to assert here, as the type system would make sure you return a valid exception list.
There was a problem hiding this comment.
done - i've removed the unnecessary asserts(but keeping the method name to make it clearer that this function's job is to popualte the list)
|
Ran all the tests: (Compared to main branch: ) |
labath
approved these changes
Jun 25, 2024
AlexisPerry
pushed a commit
to llvm-project-tlp/llvm-project
that referenced
this pull request
Jul 9, 2024
New fixes:
- properly init the `std::optional<std::vector>` to an empty vector as
opposed to `{}` (which was effectively `std::nullopt`).
---------
Co-authored-by: Vy Nguyen <oontvoo@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New fixes:
std::optional<std::vector>to an empty vector as opposed to{}(which was effectivelystd::nullopt).