[lldb][Expression] Emit hint to use --c++-ignore-context-qualifiers#177927
[lldb][Expression] Emit hint to use --c++-ignore-context-qualifiers#177927Michael137 merged 4 commits intollvm:mainfrom
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
33001d7 to
e19019a
Compare
|
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) ChangesDepends on:
(only last commit is relevant for this review) This patch emits a Patch is 63.91 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/177927.diff 41 Files Affected:
diff --git a/lldb/include/lldb/API/SBExpressionOptions.h b/lldb/include/lldb/API/SBExpressionOptions.h
index a9e929a4c0bd9..2b922b97f75b9 100644
--- a/lldb/include/lldb/API/SBExpressionOptions.h
+++ b/lldb/include/lldb/API/SBExpressionOptions.h
@@ -107,6 +107,10 @@ class LLDB_API SBExpressionOptions {
// Sets whether we will JIT an expression if it cannot be interpreted
void SetAllowJIT(bool allow);
+ bool GetIgnoreConstContext();
+
+ void SetIgnoreConstContext(bool b = true);
+
protected:
lldb_private::EvaluateExpressionOptions *get() const;
diff --git a/lldb/include/lldb/Expression/UserExpression.h b/lldb/include/lldb/Expression/UserExpression.h
index 2fde73dafa035..977a0adef1cbe 100644
--- a/lldb/include/lldb/Expression/UserExpression.h
+++ b/lldb/include/lldb/Expression/UserExpression.h
@@ -313,6 +313,16 @@ class UserExpression : public Expression {
lldb::ProcessSP &process_sp,
lldb::StackFrameSP &frame_sp);
+ /// Called by expression evaluator when a parse error occurs. Gives this
+ /// UserExpression object a chance to inspect and adjust the error diagnostics
+ /// contained in the specified \c diagnostic_manager.
+ ///
+ /// \param[in,out] diagnostic_manager DiagnosticManager manager holding the
+ /// parse error diagnostics. This function may mutate the diagnostics.
+ ///
+ virtual void
+ FixupParseErrorDiagnostics(DiagnosticManager &diagnostic_manager) const {}
+
/// The address the process is stopped in.
Address m_address;
/// The text of the expression, as typed by the user.
diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h
index 812a638910b3b..77caccdbcc74d 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -481,6 +481,10 @@ class EvaluateExpressionOptions {
void SetIsForUtilityExpr(bool b) { m_running_utility_expression = b; }
+ bool GetIgnoreConstContext() const { return m_ignore_const_contexts; }
+
+ void SetIgnoreConstContext(bool val) { m_ignore_const_contexts = val; }
+
private:
ExecutionPolicy m_execution_policy = default_execution_policy;
SourceLanguage m_language;
@@ -518,6 +522,8 @@ class EvaluateExpressionOptions {
/// used for symbol/function lookup before any other context (except for
/// the module corresponding to the current frame).
SymbolContextList m_preferred_lookup_contexts;
+
+ bool m_ignore_const_contexts = false;
};
// Target
diff --git a/lldb/packages/Python/lldbsuite/test/lldbtest.py b/lldb/packages/Python/lldbsuite/test/lldbtest.py
index 6bb4516948da5..6034eca3b93f2 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -2575,6 +2575,7 @@ def expect_expr(
result_value=None,
result_type=None,
result_children=None,
+ options=None,
):
"""
Evaluates the given expression and verifies the result.
@@ -2584,6 +2585,7 @@ def expect_expr(
:param result_type: The type that the expression result should have. None if the type should not be checked.
:param result_children: The expected children of the expression result
as a list of ValueChecks. None if the children shouldn't be checked.
+ :param options: Expression evaluation options. None if a default set of options should be used.
"""
self.assertTrue(
expr.strip() == expr,
@@ -2591,13 +2593,15 @@ def expect_expr(
)
frame = self.frame()
- options = lldb.SBExpressionOptions()
- # Disable fix-its that tests don't pass by accident.
- options.SetAutoApplyFixIts(False)
+ if not options:
+ options = lldb.SBExpressionOptions()
- # Set the usual default options for normal expressions.
- options.SetIgnoreBreakpoints(True)
+ # Disable fix-its that tests don't pass by accident.
+ options.SetAutoApplyFixIts(False)
+
+ # Set the usual default options for normal expressions.
+ options.SetIgnoreBreakpoints(True)
if self.frame().IsValid():
options.SetLanguage(frame.GuessLanguage())
diff --git a/lldb/source/API/SBExpressionOptions.cpp b/lldb/source/API/SBExpressionOptions.cpp
index 15ed403eaaea1..84f23df47b0cb 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::GetIgnoreConstContext() {
+ LLDB_INSTRUMENT_VA(this);
+
+ return m_opaque_up->GetIgnoreConstContext();
+}
+
+void SBExpressionOptions::SetIgnoreConstContext(bool b) {
+ LLDB_INSTRUMENT_VA(this, b);
+
+ m_opaque_up->SetIgnoreConstContext(b);
+}
+
EvaluateExpressionOptions *SBExpressionOptions::get() const {
return m_opaque_up.get();
}
diff --git a/lldb/source/Commands/CommandObjectExpression.cpp b/lldb/source/Commands/CommandObjectExpression.cpp
index 4919bd3639d3e..14dea55164c6e 100644
--- a/lldb/source/Commands/CommandObjectExpression.cpp
+++ b/lldb/source/Commands/CommandObjectExpression.cpp
@@ -25,6 +25,7 @@
#include "lldb/lldb-enumerations.h"
#include "lldb/lldb-forward.h"
#include "lldb/lldb-private-enumerations.h"
+#include <tuple>
using namespace lldb;
using namespace lldb_private;
@@ -44,6 +45,9 @@ Status CommandObjectExpression::CommandOptions::SetOptionValue(
const int short_option = GetDefinitions()[option_idx].short_option;
switch (short_option) {
+ case 'K':
+ ignore_const_context = true;
+ break;
case 'l':
language = Language::GetLanguageTypeFromString(option_arg);
if (language == eLanguageTypeUnknown) {
@@ -191,6 +195,7 @@ void CommandObjectExpression::CommandOptions::OptionParsingStarting(
top_level = false;
allow_jit = true;
suppress_persistent_result = eLazyBoolCalculate;
+ ignore_const_context = false;
}
llvm::ArrayRef<OptionDefinition>
@@ -213,6 +218,7 @@ CommandObjectExpression::CommandOptions::GetEvaluateExpressionOptions(
options.SetExecutionPolicy(
allow_jit ? EvaluateExpressionOptions::default_execution_policy
: lldb_private::eExecutionPolicyNever);
+ options.SetIgnoreConstContext(ignore_const_context);
bool auto_apply_fixits;
if (this->auto_apply_fixits == eLazyBoolCalculate)
diff --git a/lldb/source/Commands/CommandObjectExpression.h b/lldb/source/Commands/CommandObjectExpression.h
index 6fccf10e5dbc1..6bbe02f584b21 100644
--- a/lldb/source/Commands/CommandObjectExpression.h
+++ b/lldb/source/Commands/CommandObjectExpression.h
@@ -57,6 +57,7 @@ class CommandObjectExpression : public CommandObjectRaw,
LanguageRuntimeDescriptionDisplayVerbosity m_verbosity;
LazyBool auto_apply_fixits;
LazyBool suppress_persistent_result;
+ bool ignore_const_context;
};
CommandObjectExpression(CommandInterpreter &interpreter);
diff --git a/lldb/source/Commands/Options.td b/lldb/source/Commands/Options.td
index d96354a39b8b8..c82a029ae95ce 100644
--- a/lldb/source/Commands/Options.td
+++ b/lldb/source/Commands/Options.td
@@ -778,6 +778,9 @@ let Command = "expression" in {
Desc<"Persist expression result in a variable for subsequent use. "
"Expression results will be labeled with $-prefixed variables, "
"e.g. $0, $1, etc.">;
+ def ignore_const_context : Option<"ignore-const-context", "K">,
+ Groups<[1, 2]>,
+ Desc<"TODO">;
}
let Command = "frame diag" in {
diff --git a/lldb/source/Expression/UserExpression.cpp b/lldb/source/Expression/UserExpression.cpp
index ff7a356dbbb1e..d39bcced48390 100644
--- a/lldb/source/Expression/UserExpression.cpp
+++ b/lldb/source/Expression/UserExpression.cpp
@@ -290,34 +290,29 @@ UserExpression::Evaluate(ExecutionContext &exe_ctx,
// If there is a fixed expression, try to parse it:
if (!parse_success) {
- // Delete the expression that failed to parse before attempting to parse
- // the next expression.
- user_expression_sp.reset();
-
execution_results = lldb::eExpressionParseError;
if (!fixed_expression->empty() && options.GetAutoApplyFixIts()) {
const uint64_t max_fix_retries = options.GetRetriesWithFixIts();
for (uint64_t i = 0; i < max_fix_retries; ++i) {
// Try parsing the fixed expression.
- lldb::UserExpressionSP fixed_expression_sp(
- target->GetUserExpressionForLanguage(
- fixed_expression->c_str(), full_prefix, language, desired_type,
- options, ctx_obj, error));
- if (!fixed_expression_sp)
+ user_expression_sp.reset(target->GetUserExpressionForLanguage(
+ fixed_expression->c_str(), full_prefix, language, desired_type,
+ options, ctx_obj, error));
+ if (!user_expression_sp)
break;
+
DiagnosticManager fixed_diagnostic_manager;
- parse_success = fixed_expression_sp->Parse(
+ parse_success = user_expression_sp->Parse(
fixed_diagnostic_manager, exe_ctx, execution_policy,
keep_expression_in_memory, generate_debug_info);
if (parse_success) {
diagnostic_manager.Clear();
- user_expression_sp = fixed_expression_sp;
break;
}
// The fixed expression also didn't parse. Let's check for any new
// fixits we could try.
- if (!fixed_expression_sp->GetFixedText().empty()) {
- *fixed_expression = fixed_expression_sp->GetFixedText().str();
+ if (!user_expression_sp->GetFixedText().empty()) {
+ *fixed_expression = user_expression_sp->GetFixedText().str();
} else {
// Fixed expression didn't compile without a fixit, don't retry and
// don't tell the user about it.
@@ -328,6 +323,9 @@ UserExpression::Evaluate(ExecutionContext &exe_ctx,
}
if (!parse_success) {
+ if (user_expression_sp)
+ user_expression_sp->FixupParseErrorDiagnostics(diagnostic_manager);
+
if (target->GetEnableNotifyAboutFixIts() && fixed_expression &&
!fixed_expression->empty()) {
std::string fixit =
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
index 24a5dc5362964..52a77fbbac66a 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
@@ -88,10 +88,12 @@ ClangExpressionDeclMap::ClangExpressionDeclMap(
bool keep_result_in_memory,
Materializer::PersistentVariableDelegate *result_delegate,
const lldb::TargetSP &target,
- const std::shared_ptr<ClangASTImporter> &importer, ValueObject *ctx_obj)
+ const std::shared_ptr<ClangASTImporter> &importer, ValueObject *ctx_obj,
+ bool ignore_const_context)
: ClangASTSource(target, importer), m_found_entities(), m_struct_members(),
m_keep_result_in_memory(keep_result_in_memory),
- m_result_delegate(result_delegate), m_ctx_obj(ctx_obj), m_parser_vars(),
+ m_result_delegate(result_delegate), m_ctx_obj(ctx_obj),
+ m_ignore_const_context(ignore_const_context), m_parser_vars(),
m_struct_vars() {
EnableStructVars();
}
@@ -844,6 +846,12 @@ void ClangExpressionDeclMap::LookUpLldbClass(NameSearchContext &context) {
QualType class_qual_type = m_ast_context->getCanonicalTagType(class_decl);
+ // The synthesized __lldb_expr will adopt the qualifiers from this class
+ // type. Make sure we use the qualifiers of the method that we're currently
+ // stopped in.
+ class_qual_type.addFastQualifiers(
+ method_decl->getMethodQualifiers().getFastQualifiers());
+
TypeFromUser class_user_type(
class_qual_type.getAsOpaquePtr(),
function_decl_ctx.GetTypeSystem()->weak_from_this());
@@ -1991,7 +1999,8 @@ void ClangExpressionDeclMap::AddContextClassType(NameSearchContext &context,
std::array<CompilerType, 1> args{void_clang_type.GetPointerType()};
CompilerType method_type = m_clang_ast_context->CreateFunctionType(
- void_clang_type, args, false, 0);
+ void_clang_type, args, false,
+ m_ignore_const_context ? 0 : ut.GetTypeQualifiers());
const bool is_virtual = false;
const bool is_static = false;
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
index dddc5a06c9051..3a5f702f6c674 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
@@ -82,7 +82,8 @@ class ClangExpressionDeclMap : public ClangASTSource {
bool keep_result_in_memory,
Materializer::PersistentVariableDelegate *result_delegate,
const lldb::TargetSP &target,
- const std::shared_ptr<ClangASTImporter> &importer, ValueObject *ctx_obj);
+ const std::shared_ptr<ClangASTImporter> &importer, ValueObject *ctx_obj,
+ bool ignore_const_context);
/// Destructor
~ClangExpressionDeclMap() override;
@@ -306,6 +307,8 @@ class ClangExpressionDeclMap : public ClangASTSource {
///For details see the comment to
///`UserExpression::Evaluate`.
+ bool m_ignore_const_context = false;
+
/// The following values should not live beyond parsing
class ParserVars {
public:
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
index cfe187ffc4114..2c764d250d465 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
@@ -10,6 +10,7 @@
#include "ClangExpressionUtil.h"
+#include "clang/AST/TypeBase.h"
#include "clang/Basic/CharInfo.h"
#include "clang/Basic/FileManager.h"
#include "clang/Basic/SourceManager.h"
@@ -189,6 +190,28 @@ static void AddMacros(const DebugMacros *dm, CompileUnit *comp_unit,
}
}
+/// Return qualifers of the current C++ method.
+static clang::Qualifiers GetFrameCVQualifiers(StackFrame *frame) {
+ if (!frame)
+ return {};
+
+ auto this_sp = frame->FindVariable(ConstString("this"));
+ if (!this_sp)
+ return {};
+
+ // Lambdas that capture 'this' have a member variable called 'this'. The class
+ // context of __lldb_expr for a lambda is the class type of the 'this' capture
+ // (not the anonymous lambda structure). So use the qualifiers of the captured
+ // 'this'.
+ if (auto this_this_sp = this_sp->GetChildMemberWithName("this"))
+ return clang::Qualifiers::fromCVRMask(
+ this_this_sp->GetCompilerType().GetPointeeType().GetTypeQualifiers());
+
+ // Not in a lambda. Return 'this' qualifiers.
+ return clang::Qualifiers::fromCVRMask(
+ this_sp->GetCompilerType().GetPointeeType().GetTypeQualifiers());
+}
+
lldb_private::ClangExpressionSourceCode::ClangExpressionSourceCode(
llvm::StringRef filename, llvm::StringRef name, llvm::StringRef prefix,
llvm::StringRef body, Wrapping wrap, WrapKind wrap_kind)
@@ -340,9 +363,12 @@ void ClangExpressionSourceCode::AddLocalVariableDecls(StreamString &stream,
}
}
-bool ClangExpressionSourceCode::GetText(
- std::string &text, ExecutionContext &exe_ctx, bool add_locals,
- bool force_add_all_locals, llvm::ArrayRef<std::string> modules) const {
+bool ClangExpressionSourceCode::GetText(std::string &text,
+ ExecutionContext &exe_ctx,
+ bool add_locals,
+ bool force_add_all_locals,
+ llvm::ArrayRef<std::string> modules,
+ bool ignore_const_context) const {
const char *target_specific_defines = "typedef signed char BOOL;\n";
std::string module_macros;
llvm::raw_string_ostream module_macros_stream(module_macros);
@@ -464,13 +490,18 @@ bool ClangExpressionSourceCode::GetText(
break;
case WrapKind::CppMemberFunction:
wrap_stream.Printf("%s"
- "void \n"
- "$__lldb_class::%s(void *$__lldb_arg) \n"
- "{ \n"
- " %s; \n"
+ "void \n"
+ "$__lldb_class::%s(void *$__lldb_arg) %s \n"
+ "{ \n"
+ " %s; \n"
"%s"
- "} \n",
+ "} \n",
module_imports.c_str(), m_name.c_str(),
+ ignore_const_context
+ ? ""
+ : GetFrameCVQualifiers(exe_ctx.GetFramePtr())
+ .getAsString()
+ .c_str(),
lldb_local_var_decls.GetData(), tagged_body.c_str());
break;
case WrapKind::ObjCInstanceMethod:
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h
index f721bb2f319e1..02090b1aa9b12 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h
@@ -63,8 +63,8 @@ class ClangExpressionSourceCode : public ExpressionSourceCode {
///
/// \return true iff the source code was successfully generated.
bool GetText(std::string &text, ExecutionContext &exe_ctx, bool add_locals,
- bool force_add_all_locals,
- llvm::ArrayRef<std::string> modules) const;
+ bool force_add_all_locals, llvm::ArrayRef<std::string> modules,
+ bool ignore_const_context) const;
// Given a string returned by GetText, find the beginning and end of the body
// passed to CreateWrapped. Return true if the bounds could be found. This
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
index 2cbbae11bd18a..5695e59ad08dc 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
@@ -27,6 +27,7 @@
#include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
#include "lldb/Core/Debugger.h"
#include "lldb/Core/Module.h"
+#include "lldb/Expression/DiagnosticManager.h"
#include "lldb/Expression/ExpressionSourceCode.h"
#include "lldb/Expression/IRExecutionUnit.h"
#include "lldb/Expression/IRInterpreter.h"
@@ -55,6 +56,7 @@
#include "clang/AST/DeclCXX.h"
#include "clang/AST/DeclObjC.h"
+#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/ScopeExit.h"
#include "llvm/BinaryFormat/Dwarf.h"
@@ -416,7 +418,8 @@ void ClangUserExpression::CreateSourceCode(
m_filename, prefix, m_expr_text, GetWrapKind()));
if (!m_source_code->GetText(m_transformed_text, exe_ctx, !m_ctx_obj,
- for_completion, modules_to_import)) {
+ for_completion, modules_to_import,
+ m_options.GetIgnoreConstContext())) {
diagnostic_manager.PutString(lldb::eSeverityError,
"couldn't construct expression body");
return;
@@ -945,13 +948,49 @@ lldb::ExpressionVariableSP ClangUserExpression::GetResultAfterDematerialization(
return m_result_delegate.GetVariable();
}
+void ClangUserExpression::FixupParseErrorDiagnostics(
+ DiagnosticManager &diagnostic_manager) const {
+ const bool is_fixable_cvr_error = llvm::any_of(
+ diagnostic_manager.Diagnostics(),
+ [](std::unique_ptr<Diagnostic> const &diag) {
+ switch (diag->GetCompilerID()) {
+ ...
[truncated]
|
daacb57 to
0191c44
Compare
| } | ||
|
|
||
| diagnostic_manager.AddDiagnostic( | ||
| "Possibly trying to mutate object in a const context. Try " |
There was a problem hiding this comment.
Do users understand what LLDB means by "context"?
There was a problem hiding this comment.
How about Possibly trying to mutate object in a CV-qualified scope? Since this is the C++ plugin I'd expect people to know (or at least be able to lookup) what CV-qualified means
There was a problem hiding this comment.
I was thinking about something even more direct, such as "trying to mutate object, but this is marked const" maybe?
There was a problem hiding this comment.
We can't explicitly mention const because it could happen on any kind of CV-qualifier mismatch. Don't really like the idea of trying to parse out exactly the qualifier that's mismatch out of the error message
333ba28 to
a8b329e
Compare
|
✅ With the latest revision this PR passed the Python code formatter. |
a8b329e to
a120e7e
Compare
0c1f7bf to
6b457f1
Compare
| diagnostic_manager.AddDiagnostic( | ||
| "Possibly trying to mutate object in a const context. Try " | ||
| "running the expression with: expression --cpp-ignore-context-qualifiers " | ||
| "-- <your expression>", |
There was a problem hiding this comment.
How hard would it be to do %s", expr_text) here?
There was a problem hiding this comment.
Should be very doable. Will do
adrian-prantl
left a comment
There was a problem hiding this comment.
LGTM, one suggestion inside
6b457f1 to
289fe90
Compare
Depends on: * llvm#177921 * llvm#177926 (only last commit is relevant for this review)
289fe90 to
6d8ec2b
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/18/builds/24533 Here is the relevant piece of the build log for the reference |
Depends on:
(only last commit is relevant for this review)
This patch emits a workaround suggestion (in the form of a
note:diagnostic) when an expression fails due to trying to mutate state/call functions with CV-qualifiers that are disallowed by C++ language rules based on the context the expression is evaluated in. The note looks as follows: