-
Notifications
You must be signed in to change notification settings - Fork 16k
[lldb][Expression] Add API to set/get language-specific expression options #179208
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.
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
base: main
Are you sure you want to change the base?
[lldb][Expression] Add API to set/get language-specific expression options #179208
Conversation
…tions The motivation here is that we don't want to pollute the SBAPI with getters/setters for expression evaluation options that only apply to a single language. The ultimate goal would be to have plugins register additional options to the `expression` command when the plugin is loaded. This patch only provides the minimal `SBExpressionOptions` interface to set an option with an arbitrary name, which the language plugin knows how to interpret. The underlying options dictionary is an `StructuredData::Dictionary` so we can map strings to values of any type. But the SBAPI just exposes setting a boolean value. Future overloads of `SetLanguageOption` can provide setters for more types. The boolean setter/getter will be used for the C++-specific option being introduced in: llvm#177926
|
@adrian-prantl is this what you had in mind in |
|
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) ChangesThe motivation here is that we don't want to pollute the SBAPI with getters/setters for expression evaluation options that only apply to a single language. The ultimate goal would be to have plugins register additional options to the The boolean setter/getter will be used for the C++-specific option being introduced in: #177926 Full diff: https://github.com/llvm/llvm-project/pull/179208.diff 6 Files Affected:
diff --git a/lldb/bindings/interface/SBExpressionOptionsDocstrings.i b/lldb/bindings/interface/SBExpressionOptionsDocstrings.i
index 2bb562778db79..a6de6f034bd7a 100644
--- a/lldb/bindings/interface/SBExpressionOptionsDocstrings.i
+++ b/lldb/bindings/interface/SBExpressionOptionsDocstrings.i
@@ -61,3 +61,9 @@
%feature("docstring", "Sets whether to JIT an expression if it cannot be interpreted."
) lldb::SBExpressionOptions::SetAllowJIT;
+
+%feature("docstring", "Sets language-plugin specific boolean option for expression evaluation."
+) lldb::SBExpressionOptions::SetLanguageOption;
+
+%feature("docstring", "Gets language-plugin specific boolean option for expression evaluation."
+) lldb::SBExpressionOptions::GetLanguageOptionAsBoolean;
diff --git a/lldb/include/lldb/API/SBExpressionOptions.h b/lldb/include/lldb/API/SBExpressionOptions.h
index a9e929a4c0bd9..6f7bd53d6fa70 100644
--- a/lldb/include/lldb/API/SBExpressionOptions.h
+++ b/lldb/include/lldb/API/SBExpressionOptions.h
@@ -11,6 +11,7 @@
#include "lldb/API/SBDefines.h"
#include "lldb/API/SBLanguages.h"
+#include "lldb/API/SBStructuredData.h"
#include <vector>
@@ -107,6 +108,10 @@ class LLDB_API SBExpressionOptions {
// Sets whether we will JIT an expression if it cannot be interpreted
void SetAllowJIT(bool allow);
+ bool GetLanguageOptionAsBoolean(const char *option_name) const;
+
+ void SetLanguageOption(const char *option_name, bool value);
+
protected:
lldb_private::EvaluateExpressionOptions *get() const;
diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h
index 812a638910b3b..2583c14965e61 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -15,6 +15,7 @@
#include <string>
#include <vector>
+#include "lldb/API/SBStructuredData.h"
#include "lldb/Breakpoint/BreakpointList.h"
#include "lldb/Breakpoint/BreakpointName.h"
#include "lldb/Breakpoint/WatchpointList.h"
@@ -307,6 +308,8 @@ class TargetProperties : public Properties {
class EvaluateExpressionOptions {
public:
+ EvaluateExpressionOptions() : m_language_options_sp(std::make_shared<StructuredData::Dictionary>()) {}
+
// MSVC has a bug here that reports C4268: 'const' static/global data
// initialized with compiler generated default constructor fills the object
// with zeros. Confirmed that MSVC is *not* zero-initializing, it's just a
@@ -323,8 +326,6 @@ class EvaluateExpressionOptions {
static constexpr ExecutionPolicy default_execution_policy =
eExecutionPolicyOnlyWhenNeeded;
- EvaluateExpressionOptions() = default;
-
ExecutionPolicy GetExecutionPolicy() const { return m_execution_policy; }
void SetExecutionPolicy(ExecutionPolicy policy = eExecutionPolicyAlways) {
@@ -481,7 +482,40 @@ class EvaluateExpressionOptions {
void SetIsForUtilityExpr(bool b) { m_running_utility_expression = b; }
+ /// Set language-plugin specific option called \c option_name to
+ /// the specified boolean \c value.
+ void SetLanguageOption(llvm::StringRef option_name, bool value) {
+ if (option_name.empty())
+ return;
+
+ GetLanguageOptions().AddBooleanItem(option_name, value);
+ }
+
+ /// Get the language-plugin specific boolean option called \c option_name.
+ ///
+ /// If the option doesn't exist or is not a boolean option, returns false.
+ /// Otherwise returns the boolean value of the option.
+ bool GetLanguageOptionAsBoolean(llvm::StringRef option_name) const {
+ bool result;
+ if (!GetLanguageOptions().GetValueForKeyAsBoolean(option_name, result))
+ return false;
+
+ return result;
+ }
+
private:
+ const StructuredData::Dictionary &GetLanguageOptions() const {
+ assert (m_language_options_sp);
+
+ return *m_language_options_sp;
+ }
+
+ StructuredData::Dictionary &GetLanguageOptions() {
+ assert (m_language_options_sp);
+
+ return *m_language_options_sp;
+ }
+
ExecutionPolicy m_execution_policy = default_execution_policy;
SourceLanguage m_language;
std::string m_prefix;
@@ -514,6 +548,10 @@ class EvaluateExpressionOptions {
mutable std::string m_pound_line_file;
mutable uint32_t m_pound_line_line = 0;
+ /// Dictionary mapping names of language-plugin specific options
+ /// to values.
+ StructuredData::DictionarySP m_language_options_sp = nullptr;
+
/// During expression evaluation, any SymbolContext in this list will be
/// used for symbol/function lookup before any other context (except for
/// the module corresponding to the current frame).
diff --git a/lldb/source/API/SBExpressionOptions.cpp b/lldb/source/API/SBExpressionOptions.cpp
index 15ed403eaaea1..e28c7ebfd0133 100644
--- a/lldb/source/API/SBExpressionOptions.cpp
+++ b/lldb/source/API/SBExpressionOptions.cpp
@@ -256,6 +256,18 @@ void SBExpressionOptions::SetAllowJIT(bool allow) {
: eExecutionPolicyNever);
}
+bool SBExpressionOptions::GetLanguageOptionAsBoolean(const char *option_name) const {
+ LLDB_INSTRUMENT_VA(this, option_name);
+
+ return m_opaque_up->GetLanguageOptionAsBoolean(option_name);
+}
+
+void SBExpressionOptions::SetLanguageOption(const char *option_name, bool value) {
+ LLDB_INSTRUMENT_VA(this, option_name, value);
+
+ m_opaque_up->SetLanguageOption(option_name, value);
+ }
+
EvaluateExpressionOptions *SBExpressionOptions::get() const {
return m_opaque_up.get();
}
diff --git a/lldb/test/API/commands/expression/options/TestExprOptions.py b/lldb/test/API/commands/expression/options/TestExprOptions.py
index 01899f3b97cf4..96bb4742a1a10 100644
--- a/lldb/test/API/commands/expression/options/TestExprOptions.py
+++ b/lldb/test/API/commands/expression/options/TestExprOptions.py
@@ -7,7 +7,6 @@
Test expression command options.
"""
-
import lldb
import lldbsuite.test.lldbutil as lldbutil
from lldbsuite.test.decorators import *
@@ -85,3 +84,27 @@ def test_expr_options_lang(self):
val = frame.EvaluateExpression("id == 0", options)
self.assertTrue(val.IsValid())
self.assertFalse(val.GetError().Success())
+
+ def test_expr_options_language_options(self):
+ """Test SetLanguageOption/GetLanguageOption SBAPIs"""
+
+ options = lldb.SBExpressionOptions()
+ self.assertFalse(options.GetLanguageOptionAsBoolean("foo"))
+ self.assertFalse(options.GetLanguageOptionAsBoolean("bar"))
+
+ options.SetLanguageOption("foo", True)
+ options.SetLanguageOption("bar", True)
+ self.assertTrue(options.GetLanguageOptionAsBoolean("foo"))
+ self.assertTrue(options.GetLanguageOptionAsBoolean("bar"))
+
+ options.SetLanguageOption("foo", False)
+ options.SetLanguageOption("bar", False)
+ self.assertFalse(options.GetLanguageOptionAsBoolean("foo"))
+ self.assertFalse(options.GetLanguageOptionAsBoolean("bar"))
+
+ self.assertFalse(options.GetLanguageOptionAsBoolean(""))
+ options.SetLanguageOption("", True)
+ self.assertFalse(options.GetLanguageOptionAsBoolean(""))
+
+ options.SetLanguageOption(None, True)
+ self.assertFalse(options.GetLanguageOptionAsBoolean(None))
diff --git a/lldb/unittests/Expression/ExpressionTest.cpp b/lldb/unittests/Expression/ExpressionTest.cpp
index ceb567c28ab99..4201516dfc820 100644
--- a/lldb/unittests/Expression/ExpressionTest.cpp
+++ b/lldb/unittests/Expression/ExpressionTest.cpp
@@ -10,6 +10,7 @@
#include "gtest/gtest.h"
#include "TestingSupport/TestUtilities.h"
+#include "lldb/Target/Target.h"
#include "lldb/Expression/Expression.h"
#include "llvm/Testing/Support/Error.h"
@@ -127,3 +128,27 @@ TEST_P(ExpressionTestFixture, FunctionCallLabel) {
INSTANTIATE_TEST_SUITE_P(FunctionCallLabelTest, ExpressionTestFixture,
testing::ValuesIn(g_label_test_cases));
+
+TEST(ExpressionTests, ExpressionOptions_Basic) {
+ EvaluateExpressionOptions options;
+
+ ASSERT_FALSE(options.GetLanguageOptionAsBoolean("foo"));
+ ASSERT_FALSE(options.GetLanguageOptionAsBoolean("bar"));
+
+ options.SetLanguageOption("foo", true);
+ options.SetLanguageOption("bar", true);
+
+ ASSERT_TRUE(options.GetLanguageOptionAsBoolean("foo"));
+ ASSERT_TRUE(options.GetLanguageOptionAsBoolean("bar"));
+
+ options.SetLanguageOption("foo", false);
+ options.SetLanguageOption("bar", false);
+
+ ASSERT_FALSE(options.GetLanguageOptionAsBoolean("foo"));
+ ASSERT_FALSE(options.GetLanguageOptionAsBoolean("bar"));
+
+ // Empty option names not allowed.
+ ASSERT_FALSE(options.GetLanguageOptionAsBoolean(""));
+ options.SetLanguageOption("", true);
+ ASSERT_FALSE(options.GetLanguageOptionAsBoolean(""));
+}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
bulbazord
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
| bool GetLanguageOptionAsBoolean(const char *option_name) const; | ||
|
|
||
| void SetLanguageOption(const char *option_name, bool value); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's weird to a getter specifically for boolean options but have the setter name be "generic" but take a boolean argument. I also think this is going to break in Python if you add overloads for the setter with different argument types like void SetLanguageOption(const char *option_name, int value).
| bool GetBooleanLanguageOption(const char *option_name) const; | |
| void SetBooleanLanguageOption(const char *option_name, bool value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh my assumption was that we can overload functions. Pretty sure SWIG is capable of it? But do we avoid doing overloads?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can overload functions. SWIG will handle it correctly with args or kwargs in python.
I think Ismail's point is one of consistency. The getter has Boolean in the name but the setter doesn't (even though the two methods are supposed to be matching). I think changing the setter name would resolve this.
To go further, does GetLanguageOptionAsBoolean mean you can get a non-boolean option value as a boolean? Maybe something like GetBooleanLanguageOption is clearer? To make it matchy-matchy you'd also want SetBooleanLanguageOption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I'll rename the two as suggested!
| // Sets whether we will JIT an expression if it cannot be interpreted | ||
| void SetAllowJIT(bool allow); | ||
|
|
||
| bool GetLanguageOptionAsBoolean(const char *option_name) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works — or you could have a function that takes an entire dictionary and leave the AsBoolean stuff to SBStructuredData.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could work. I just didnt want to have a setter that takes an SBStructuredData because it made things pretty unergonomic. So i just mirrored the setter/getter. But i dont feel strongly about either direction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we want to expose the dictionary to users. Both because that's unergonomic and because what does the dictionary mean? Can I delete elements from it, add elements to it? I think it's better to hide that.
lldb/include/lldb/Target/Target.h
Outdated
| if (option_name.empty()) | ||
| return; | ||
|
|
||
| GetLanguageOptions().AddBooleanItem(option_name, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this just silently fail if the user has mistyped the name of the item?
That doesn't seem very user-friendly, this should return an SBError or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have anything in-place to validate whether the option is something that the expression parser will understand. But happy to change the SBAPI setter to return a boolean for now, in case we ever want to/are able to validate the option name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ended up making it return a llvm::Expected/SBError
lldb/include/lldb/Target/Target.h
Outdated
| /// | ||
| /// If the option doesn't exist or is not a boolean option, returns false. | ||
| /// Otherwise returns the boolean value of the option. | ||
| bool GetLanguageOptionAsBoolean(llvm::StringRef option_name) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think silently returning false for mistypings is the right API design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea agreed, happy to change it to an llvm::Expected
medismailben
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks!
The motivation here is that we don't want to pollute the SBAPI with getters/setters for expression evaluation options that only apply to a single language. The ultimate goal would be to have plugins register additional options to the
expressioncommand when the plugin is loaded. This patch only provides the minimalSBExpressionOptionsinterface to set an option with an arbitrary name, which the language plugin knows how to interpret. The underlying options dictionary is anStructuredData::Dictionaryso we can map strings to values of any type. But the SBAPI just exposes setting a boolean value. Future overloads ofSetLanguageOptioncan provide setters for more types.The boolean setter/getter will be used for the C++-specific option being introduced in: #177926