[lldb][lldb-dap] Cleanup breakpoint filters. #87550

oontvoo · 2024-04-03T20:16:28Z

Details:

remove Swift breakpoint filter because this version of LLDB does not support Swift.
only return objc filters when working on macos.

llvmbot · 2024-04-03T20:17:05Z

@llvm/pr-subscribers-lldb

Author: Vy Nguyen (oontvoo)

Changes

Details:

remove Swift breakpoint filter because this version of LLDB does not support Swift.
only return objc filters when working on macos.

Full diff: https://github.com/llvm/llvm-project/pull/87550.diff

2 Files Affected:

(modified) lldb/tools/lldb-dap/DAP.cpp (+1-3)
(modified) lldb/tools/lldb-dap/lldb-dap.cpp (+7)

diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index b254ddfef0d5ff..52e607350407ab 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -36,9 +36,7 @@ DAP::DAP()
           {{"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}}),
+           {"objc_throw", "Objective-C Throw", lldb::eLanguageTypeObjC}}),
       focus_tid(LLDB_INVALID_THREAD_ID), sent_terminated_event(false),
       stop_at_entry(false), is_attach(false),
       enable_auto_variable_summaries(false),
diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp
index 55f8c920e60016..5f5014eaab90f0 100644
--- a/lldb/tools/lldb-dap/lldb-dap.cpp
+++ b/lldb/tools/lldb-dap/lldb-dap.cpp
@@ -1628,7 +1628,14 @@ 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;
+  std::string triple =
+      std::string(g_dap.debugger.GetSelectedPlatform().GetTriple());
   for (const auto &exc_bp : g_dap.exception_breakpoints) {
+    // Skipping objc breakpoint filters if not working on macos.
+    if (exc_bp.language == lldb::eLanguageTypeObjC &&
+        triple.find("macos") == std::string::npos) {
+      continue;
+    }
     filters.emplace_back(CreateExceptionBreakpointFilter(exc_bp));
   }
   body.try_emplace("exceptionBreakpointFilters", std::move(filters));

jimingham · 2024-04-03T21:46:01Z

lldb/tools/lldb-dap/lldb-dap.cpp

  for (const auto &exc_bp : g_dap.exception_breakpoints) {
+    // Skipping objc breakpoint filters if not working on macos.


GNUStep is still a thing. It's likely the exception throw breakpoint is the only part of our ObjC support that would work with GNUStep, however, so maybe this doesn't matter all that much.

clayborg

I would be nice if we can detect if we support Swift dynamically. Internally in LLDB, we can ask for a TypeSystem by language using:

llvm::Expected<lldb::TypeSystemSP>
TypeSystemMap::GetTypeSystemForLanguage(lldb::LanguageType language,
                                        Module *module, bool can_create);
llvm::Expected<lldb::TypeSystemSP>
TypeSystemMap::GetTypeSystemForLanguage(lldb::LanguageType language,
                                        Target *target, bool can_create);

These functions will return respectively:

TypeSystem::CreateInstance(language, module);
TypeSystem::CreateInstance(language, target);

We should be able to add a new static method to the TypeSystem.h/.cpp that can answer if a language is supported with something like:

static bool TypeSystem::SupportsLanguage(lldb::LanguageType language);

Each TypeSystem plugin can register a new callback that can answer if they support a language. Then we can add a static function on SBDebugger that would expose this function with something like:

static bool SBDebugger::SupportsLanguage(lldb::LanguageType language);

I will make comments in the DAP.cpp where this would be used.

clayborg · 2024-04-04T02:49:44Z

lldb/tools/lldb-dap/DAP.cpp

          {{"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}}),
+           {"objc_throw", "Objective-C Throw", lldb::eLanguageTypeObjC}}),


See my comments for this patch in the review. It would be great to determine if a language is supported by LLDB dynamically and only add support for languages that the current LLDB supports. So we would probably need to default construct the exception_breakpoints() and then inside this constructor or before we try to use exception_breakpoints, we would need to populate it by doing something like:

if (g_dap.debugger.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 (g_dap.debugger.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 (g_dap.debugger.SupportsLanguage(lldb:: eLanguageTypeSwift)) { exception_breakpoints.emplace_back({"swift_catch", "Swift Catch", lldb:: eLanguageTypeSwift}) exception_breakpoints.emplace_back("swift_throw", "Swift Throw", lldb:: eLanguageTypeSwift}) }

Thanks for the detailed suggestions! Makes sense!

For ObjC, just to clarify, the intention of hiding the filters was NOT because the debugger didn't support it - the intention was to avoid clustering/confusing users - that is, if they're not debugging ObjC, there's no reason to show the objc filters. (And we infer this by checking if they're debugging on a mac - it's theoretically possible they're cross debugging objc from a non-mac but in practice I don't think it's likely )
Does this seem reasonable?

The ObjC Language runtime is able to detect whether ObjC is loaded into the program. It runs that detector on each library load, so it keeps an accurate notion of this. Ditto for the swift language runtime. It would be better to consult that than guess based on platform. There may not currently be a way to ask that from the SB API's but we can certainly make one...

The ObjC Language runtime is able to detect whether ObjC is loaded into the program. It runs that detector on each library load, so it keeps an accurate notion of this. Ditto for the swift language runtime. It would be better to consult that than guess based on platform.

This code runs before the runtimes (objc/swift) so it probably won't be able to query that, though?

P.S: it was pointed out to me that changed capabilities can be pushed as an event. So I guess we could init the list of filters based on whether the languages are supported. Then when the runtime(s) become available, we can update the list after querying for which language is being debugged. WDYT?

ObjC tends to only use exceptions for error reporting, not for message passing, so a lot of ObjC developers leave this breakpoint on all the time. Having to wait till the first load event that brings in ObjC would make that tedious.

Btw, it's possible to register typesystems/languages at initialization via runtime plugins. I think it would be ideal if the breakpoint list could get updated/refreshed after the target has been loaded (maybe at the first stop point)

adrian-prantl · 2024-04-05T20:59:26Z

I would be nice if we can detect if we support Swift dynamically. Internally in LLDB, we can ask for a TypeSystem by language using:

See also how lldb/test/Shell/Process/UnsupportedLanguage.test works

bulbazord · 2024-04-05T23:10:47Z

lldb/tools/lldb-dap/lldb-dap.cpp

  for (const auto &exc_bp : g_dap.exception_breakpoints) {
+    // Skipping objc breakpoint filters if not working on macos.
+    if (exc_bp.language == lldb::eLanguageTypeObjC &&
+        triple.find("macos") == std::string::npos) {


In addition to what Jim said above, this might not always work. For example, there are more apple triples (e.g. arm64-apple-ios)

- added util function for querying whether a language is supported by the type system - populate the breakpoint filters table based on the supported language(s)

oontvoo · 2024-05-24T13:21:39Z

I would be nice if we can detect if we support Swift dynamically. Internally in LLDB, we can ask for a TypeSystem by language using:
llvm::Expected<lldb::TypeSystemSP>
TypeSystemMap::GetTypeSystemForLanguage(lldb::LanguageType language,
                                        Module *module, bool can_create);
llvm::Expected<lldb::TypeSystemSP>
TypeSystemMap::GetTypeSystemForLanguage(lldb::LanguageType language,
                                        Target *target, bool can_create);
These functions will return respectively:
TypeSystem::CreateInstance(language, module);
TypeSystem::CreateInstance(language, target);
We should be able to add a new static method to the TypeSystem.h/.cpp that can answer if a language is supported with something like:
static bool TypeSystem::SupportsLanguage(lldb::LanguageType language);
Each TypeSystem plugin can register a new callback that can answer if they support a language. Then we can add a static function on SBDebugger that would expose this function with something like:
static bool SBDebugger::SupportsLanguage(lldb::LanguageType language);
I will make comments in the DAP.cpp where this would be used.

Done!

JDevlieghere · 2024-05-24T15:20:19Z

lldb/source/Symbol/TypeSystem.cpp

+  LanguageSet plugins =
+      PluginManager::GetAllTypeSystemSupportedLanguagesForTypes();


s/plugins/languages/

JDevlieghere · 2024-05-24T15:24:17Z

lldb/tools/lldb-dap/DAP.h

@@ -331,6 +333,7 @@ struct DAP {
  // "Content-Length:" field followed by the length, followed by the raw
  // JSON bytes.
  void SendJSON(const std::string &json_str);
+  bool bp_initted;


This should be next to exception_breakpoints, but I wouldn't bother with the boolean and instead make exception_breakpoints a std::optional<...> and initialize it in PopulateExceptionBreakpoints so that the two are inherently tied together.

- use std::optional - rename plugins->languages

JDevlieghere

LGTM!

lldb/tools/lldb-dap/DAP.cpp

oontvoo · 2024-05-28T15:04:00Z

@clayborg Hi, do you have any further comments/feedback on this? Thanks!
(If not, I plan to merge this in the next few days)

gulfemsavrun · 2024-05-29T20:43:18Z

We started seeing lldb test failures in TestDAP*.

======================================================================
FAIL: test_commands (TestDAP_launch.TestDAP_launch.test_commands)
    Tests the "initCommands", "preRunCommands", "stopCommands",
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/b/s/w/ir/x/w/llvm-llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py", line 150, in wrapper
    return func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/b/s/w/ir/x/w/llvm-llvm-project/lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py", line 324, in test_commands
    self.continue_to_breakpoints(breakpoint_ids)
  File "/b/s/w/ir/x/w/llvm-llvm-project/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py", line 245, in continue_to_breakpoints
    self.verify_breakpoint_hit(breakpoint_ids)
  File "/b/s/w/ir/x/w/llvm-llvm-project/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py", line 99, in verify_breakpoint_hit
    self.assertTrue(False, "breakpoint not hit")
AssertionError: False is not true : breakpoint not hit

https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket/8746585790559468897/+/u/lldb/test/stdout

This reverts commit cfb209b.

Reverts #87550 because it broke `TestDAP*` lldb tests. https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64-rbe/b8746585790559468897/overview

Re-apply #87550 with fixes. Details: Some tests in fuchsia failed because of the newly added assertion. This was because `GetExceptionBreakpoint()` could be called before `g_dap.debugger` was initted. The fix here is to just lazily populate the list in GetExceptionBreakpoint() rather than assuming it's already been initted. (There is some nuisance here because we can't simply just populate it in DAP::DAP(), which is a global ctor and is called before `SBDebugger::Initialize()` is called. )

felipepiovezan · 2024-06-07T15:47:35Z

@oontvoo I think this broke the bots again: https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/as-lldb-cmake/5341/console

felipepiovezan · 2024-06-07T15:49:06Z

Would you mind reverting it until you have a chance to look at the failures?

oontvoo requested a review from JDevlieghere as a code owner April 3, 2024 20:16

llvmbot added the lldb label Apr 3, 2024

oontvoo requested review from labath and cmtice and removed request for cmtice April 3, 2024 20:17

jimingham reviewed Apr 3, 2024

View reviewed changes

clayborg reviewed Apr 4, 2024

View reviewed changes

bulbazord reviewed Apr 5, 2024

View reviewed changes

oontvoo requested review from daniel-grumberg, cyndyishida, aaupov, maksfb, rafaelauler, ayermolo, dcci, lanza, bcardosolopes, tbaederr and Endilll as code owners May 24, 2024 13:08

oontvoo force-pushed the bp_filter branch from 98023c0 to 7a0af7b Compare May 24, 2024 13:15

oontvoo removed request for daniel-grumberg, cyndyishida, aaupov, maksfb, rafaelauler, ayermolo, dcci and lanza May 24, 2024 13:16

oontvoo removed request for bcardosolopes, tbaederr and Endilll May 24, 2024 13:16

oontvoo closed this May 24, 2024

oontvoo force-pushed the bp_filter branch from 7a0af7b to f35aac6 Compare May 24, 2024 13:17

[lldb]Clean up breakpoint filters

e86d5f9

- added util function for querying whether a language is supported by the type system - populate the breakpoint filters table based on the supported language(s)

oontvoo reopened this May 24, 2024

oontvoo closed this May 24, 2024

oontvoo reopened this May 24, 2024

JDevlieghere reviewed May 24, 2024

View reviewed changes

addressed review comments:

e004a90

- use std::optional - rename plugins->languages

JDevlieghere approved these changes May 24, 2024

View reviewed changes

lldb/tools/lldb-dap/DAP.cpp Show resolved Hide resolved

oontvoo and others added 2 commits May 28, 2024 11:02

Merge branch 'llvm:main' into bp_filter

7aa99ee

addressed review comment

49fdb82

oontvoo merged commit cfb209b into llvm:main May 29, 2024
5 checks passed

gulfemsavrun added a commit that referenced this pull request May 29, 2024

Revert "[lldb][lldb-dap] Cleanup breakpoint filters. (#87550)"

9f72683

This reverts commit cfb209b.

gulfemsavrun mentioned this pull request May 29, 2024

Revert "[lldb][lldb-dap] Cleanup breakpoint filters." #93739

Merged

oontvoo mentioned this pull request Jun 6, 2024

Reapply PR/87550 #94625

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lldb][lldb-dap] Cleanup breakpoint filters. #87550

[lldb][lldb-dap] Cleanup breakpoint filters. #87550

oontvoo commented Apr 3, 2024

llvmbot commented Apr 3, 2024

jimingham Apr 3, 2024

clayborg left a comment

clayborg Apr 4, 2024

oontvoo Apr 5, 2024

jimingham Apr 5, 2024

oontvoo Apr 5, 2024

oontvoo Apr 5, 2024

jimingham Apr 8, 2024

walter-erquinigo Apr 18, 2024

adrian-prantl commented Apr 5, 2024

bulbazord Apr 5, 2024

oontvoo commented May 24, 2024

JDevlieghere May 24, 2024

JDevlieghere May 24, 2024

JDevlieghere left a comment

oontvoo commented May 28, 2024

gulfemsavrun commented May 29, 2024 •

edited

Loading

felipepiovezan commented Jun 7, 2024

felipepiovezan commented Jun 7, 2024

		for (const auto &exc_bp : g_dap.exception_breakpoints) {
		// Skipping objc breakpoint filters if not working on macos.

		LanguageSet plugins =
		PluginManager::GetAllTypeSystemSupportedLanguagesForTypes();

[lldb][lldb-dap] Cleanup breakpoint filters. #87550

[lldb][lldb-dap] Cleanup breakpoint filters. #87550

Conversation

oontvoo commented Apr 3, 2024

llvmbot commented Apr 3, 2024

Choose a reason for hiding this comment

clayborg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrian-prantl commented Apr 5, 2024

Choose a reason for hiding this comment

oontvoo commented May 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JDevlieghere left a comment

Choose a reason for hiding this comment

oontvoo commented May 28, 2024

gulfemsavrun commented May 29, 2024 • edited Loading

felipepiovezan commented Jun 7, 2024

felipepiovezan commented Jun 7, 2024

gulfemsavrun commented May 29, 2024 •

edited

Loading