Skip to content

Conversation

walter-erquinigo
Copy link
Member

CommandObjectBreakpoint has a harcoded list of languages for which exception breakpoints can be enabled. I'm making this a bit more generic so that my Mojo plugin can get this feature.
Basically, I'm adding a new overridable method Language::SupportsExceptionBreakpoints that can be used by language plugins to determine whether they support this feature or not. This method is used in addition to the hardcoded list and, as an example, I'm using it for the ObjC language support.

Another route is simply to avoid doing the check that it's being done right now and simply try to the create the exception breakpoint for whatever language that is not in the hardcoded list. I'm happy to do that if the reviewers think it's a good idea.

As a note, the other possible place for adding this SupportsExceptionBreakpoints method is in LanguageRuntime. However, accessing it requires having a process, which is not always the case when invoking the breakpoint set -E command. The process might not be alive yet, so Language is a good second place for this.

And as a final note, I don't want to make this SupportsExceptionBreakpoints complicated. I'm keeping it as simple as possible because it can easily evolve as it's not part of the public API.

CommandObjectBreakpoint has a harcoded list of languages for which exception breakpoints can be enabled. I'm making this a bit more generic so that my Mojo plugin can get this feature.
Basically, I'm adding a new overridable method `Language::SupportsExceptionBreakpoints` that can be used by language plugins to determine whether they support this feature or not. This method is used in addition to the hardcoded list and, as an example, I'm using it for the ObjC language support.

Another route is simply to avoid doing the check that it's being done right now and simply try to the create the exception breakpoint for whatever language that is not in the hardcoded list. I'm happy to do that if the reviewers think it's a good idea.

As a note, the other possible place for adding this `SupportsExceptionBreakpoints` method is in `LanguageRuntime`. However, accessing it requires having a process, which is not always the case when invoking the `breakpoint set -E` command. The process might not be alive yet, so `Language` is a good second place for this.

And as a final note, I don't want to make this `SupportsExceptionBreakpoints` complicated. I'm keeping it as simple as possible because it can easily evolve as it's not part of the public API.
@llvmbot
Copy link
Member

llvmbot commented Jul 4, 2024

@llvm/pr-subscribers-lldb

Author: Walter Erquinigo (walter-erquinigo)

Changes

CommandObjectBreakpoint has a harcoded list of languages for which exception breakpoints can be enabled. I'm making this a bit more generic so that my Mojo plugin can get this feature.
Basically, I'm adding a new overridable method Language::SupportsExceptionBreakpoints that can be used by language plugins to determine whether they support this feature or not. This method is used in addition to the hardcoded list and, as an example, I'm using it for the ObjC language support.

Another route is simply to avoid doing the check that it's being done right now and simply try to the create the exception breakpoint for whatever language that is not in the hardcoded list. I'm happy to do that if the reviewers think it's a good idea.

As a note, the other possible place for adding this SupportsExceptionBreakpoints method is in LanguageRuntime. However, accessing it requires having a process, which is not always the case when invoking the breakpoint set -E command. The process might not be alive yet, so Language is a good second place for this.

And as a final note, I don't want to make this SupportsExceptionBreakpoints complicated. I'm keeping it as simple as possible because it can easily evolve as it's not part of the public API.


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

3 Files Affected:

  • (modified) lldb/include/lldb/Target/Language.h (+5-1)
  • (modified) lldb/source/Commands/CommandObjectBreakpoint.cpp (+6-3)
  • (modified) lldb/source/Plugins/Language/ObjC/ObjCLanguage.h (+2)
diff --git a/lldb/include/lldb/Target/Language.h b/lldb/include/lldb/Target/Language.h
index ff7c60bf68bfc..9c2c765ce497f 100644
--- a/lldb/include/lldb/Target/Language.h
+++ b/lldb/include/lldb/Target/Language.h
@@ -245,7 +245,7 @@ class Language : public PluginInterface {
   // a match.  But we wouldn't want this to match AnotherA::my_function.  The
   // user is specifying a truncated path, not a truncated set of characters.
   // This function does a language-aware comparison for those purposes.
-  virtual bool DemangledNameContainsPath(llvm::StringRef path, 
+  virtual bool DemangledNameContainsPath(llvm::StringRef path,
                                          ConstString demangled) const;
 
   // if a language has a custom format for printing variable declarations that
@@ -363,6 +363,10 @@ class Language : public PluginInterface {
     return false;
   }
 
+  /// Returns true if this Language supports exception breakpoints via a
+  /// corresponding LanguageRuntime plugin.
+  virtual bool SupportsExceptionBreakpoints() const { return false; }
+
 protected:
   // Classes that inherit from Language can see and modify these
 
diff --git a/lldb/source/Commands/CommandObjectBreakpoint.cpp b/lldb/source/Commands/CommandObjectBreakpoint.cpp
index cd4c7790f447e..a5fe9273fac76 100644
--- a/lldb/source/Commands/CommandObjectBreakpoint.cpp
+++ b/lldb/source/Commands/CommandObjectBreakpoint.cpp
@@ -308,9 +308,6 @@ class CommandObjectBreakpointSet : public CommandObjectParsed {
         case eLanguageTypeC_plus_plus_14:
           m_exception_language = eLanguageTypeC_plus_plus;
           break;
-        case eLanguageTypeObjC:
-          m_exception_language = eLanguageTypeObjC;
-          break;
         case eLanguageTypeObjC_plus_plus:
           error_context =
               "Set exception breakpoints separately for c++ and objective-c";
@@ -319,6 +316,12 @@ class CommandObjectBreakpointSet : public CommandObjectParsed {
           error_context = "Unknown language type for exception breakpoint";
           break;
         default:
+          if (Language *languagePlugin = Language::FindPlugin(language)) {
+            if (languagePlugin->SupportsExceptionBreakpoints()) {
+              m_exception_language = language;
+              break;
+            }
+          }
           error_context = "Unsupported language type for exception breakpoint";
         }
         if (!error_context.empty())
diff --git a/lldb/source/Plugins/Language/ObjC/ObjCLanguage.h b/lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
index a50f4b036108d..a61d0f128370d 100644
--- a/lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
+++ b/lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
@@ -194,6 +194,8 @@ class ObjCLanguage : public Language {
 
   llvm::StringRef GetInstanceVariableName() override { return "self"; }
 
+  bool SupportsExceptionBreakpoints() const override { return true; }
+
   // PluginInterface protocol
   llvm::StringRef GetPluginName() override { return GetPluginNameStatic(); }
 };

@DavidSpickett
Copy link
Collaborator

This method is used in addition to the hardcoded list and, as an example, I'm using it for the ObjC language support.

And does this list include languages that are not plugin provided?

I was wondering why only Objective C changed, but if the rest are not plugins then this makes sense.

@@ -245,7 +245,7 @@ class Language : public PluginInterface {
// a match. But we wouldn't want this to match AnotherA::my_function. The
// user is specifying a truncated path, not a truncated set of characters.
// This function does a language-aware comparison for those purposes.
virtual bool DemangledNameContainsPath(llvm::StringRef path,
virtual bool DemangledNameContainsPath(llvm::StringRef path,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unrelated change.

Copy link
Collaborator

@DavidSpickett DavidSpickett left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -319,6 +316,12 @@ class CommandObjectBreakpointSet : public CommandObjectParsed {
error_context = "Unknown language type for exception breakpoint";
break;
default:
if (Language *languagePlugin = Language::FindPlugin(language)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could drop the braces from the first if I think, but this is already in so many sets of braces I'd keep them for clarity's sake.

Copy link
Member Author

Choose a reason for hiding this comment

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

Within switch statements I tend to be very paranoid with braces

@walter-erquinigo
Copy link
Member Author

And does this list include languages that are not plugin provided?

The hardcoded list in the file being changes includes C++ and Objc in its various variants.

I was wondering why only Objective C changed, but if the rest are not plugins then this makes sense.

That was the only simple example I could use to show how this change does work.

Thanks for the review!

@walter-erquinigo walter-erquinigo merged commit 94a067a into llvm:main Jul 4, 2024
walter-erquinigo pushed a commit that referenced this pull request Jul 5, 2024
As a minor follow up for #97675, I'm renaming `SupportsExceptionBreakpoints` to `SupportsExceptionBreakpointsOnThrow` and adding a `SupportsExceptionBreakpointsOnCatch` to have a bit of more granularity.
@walter-erquinigo walter-erquinigo deleted the walter/exception2 branch July 6, 2024 01:42
kbluck pushed a commit to kbluck/llvm-project that referenced this pull request Jul 6, 2024
…lvm#97675)

CommandObjectBreakpoint has a harcoded list of languages for which
exception breakpoints can be enabled. I'm making this a bit more generic
so that my Mojo plugin can get this feature.
Basically, I'm adding a new overridable method
`Language::SupportsExceptionBreakpoints` that can be used by language
plugins to determine whether they support this feature or not. This
method is used in addition to the hardcoded list and, as an example, I'm
using it for the ObjC language support.

Another route is simply to avoid doing the check that it's being done
right now and simply try to the create the exception breakpoint for
whatever language that is not in the hardcoded list. I'm happy to do
that if the reviewers think it's a good idea.

As a note, the other possible place for adding this
`SupportsExceptionBreakpoints` method is in `LanguageRuntime`. However,
accessing it requires having a process, which is not always the case
when invoking the `breakpoint set -E` command. The process might not be
alive yet, so `Language` is a good second place for this.

And as a final note, I don't want to make this
`SupportsExceptionBreakpoints` complicated. I'm keeping it as simple as
possible because it can easily evolve as it's not part of the public
API.
kbluck pushed a commit to kbluck/llvm-project that referenced this pull request Jul 6, 2024
As a minor follow up for llvm#97675, I'm renaming `SupportsExceptionBreakpoints` to `SupportsExceptionBreakpointsOnThrow` and adding a `SupportsExceptionBreakpointsOnCatch` to have a bit of more granularity.
@jimingham
Copy link
Collaborator

CommandObjectBreakpoint has a harcoded list of languages for which exception breakpoints can be enabled. I'm making this a bit more generic so that my Mojo plugin can get this feature. Basically, I'm adding a new overridable method Language::SupportsExceptionBreakpoints that can be used by language plugins to determine whether they support this feature or not. This method is used in addition to the hardcoded list and, as an example, I'm using it for the ObjC language support.

Another route is simply to avoid doing the check that it's being done right now and simply try to the create the exception breakpoint for whatever language that is not in the hardcoded list. I'm happy to do that if the reviewers think it's a good idea.

As a note, the other possible place for adding this SupportsExceptionBreakpoints method is in LanguageRuntime. However, accessing it requires having a process, which is not always the case when invoking the breakpoint set -E command. The process might not be alive yet, so Language is a good second place for this.

Exception Breakpoints are created by the static method LanguageRuntime::CreateExceptionBreakpoint. So they don't require a process. Exception breakpoint resolvers can be "resolver discovering resolvers". This used to be necessary when there were two ObjC runtimes possible on MacOS X, and you couldn't necessarily tell which one was going to be used till the program till it ran. So they are able to adjust themselves when their Target launches a process.

It's a little awkward to have a static method in LanguageRuntime that makes the Exception breakpoint conditioned by a Language method 'SupportsExceptionBreakpoint".

And as a final note, I don't want to make this SupportsExceptionBreakpoints complicated. I'm keeping it as simple as possible because it can easily evolve as it's not part of the public API.

@walter-erquinigo
Copy link
Member Author

Exception Breakpoints are created by the static method LanguageRuntime::CreateExceptionBreakpoint. So they don't require a process.

Indeed, but let me elaborate. What I need is a method that doesn't require a process that could tell me if a certain language can support exception breakpoints, as that's what the checks in the breakpoint set -E <lang> command try to do.

Then I had two main choices w.r.t LanguageRuntime:

  • Add the method as non-static LanguageRuntime, but LanguageRuntime instances do require a process.
  • Add a static method in LanguageRuntime, like CreateExceptionBreakpoint, but it turns out that CreateExceptionBreakpoint ends up relying on Language::GetDefaultExceptionResolverDescription and Language::GetExceptionResolverDescription. So this made me believe that it'd be better to place all the static exception feature bits in Language and not LanguageRuntime. LanguageRuntime seems to be a place more suited for dealing with runtime behaviors and not static pieces of information.

It's a little awkward to have a static method in LanguageRuntime that makes the Exception breakpoint conditioned by a Language method 'SupportsExceptionBreakpoint".

I somewhat agree with it, but probably I'd end up writing a static method LanguageRuntime::SupportsExceptionBreakpoint that invokes a non-static Language::SupportsExceptionBreakpoint. And as I mentioned, there's already precedent for other exception related stuff in Language.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants