-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[lldb][Expression] Emit a 'Note' diagnostic that indicates the language used for expression evaluation #161688
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
[lldb][Expression] Emit a 'Note' diagnostic that indicates the language used for expression evaluation #161688
Conversation
Have yet to write tests for it. |
@llvm/pr-subscribers-debuginfo @llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) ChangesSince it's a 'Note' diagnostic it would only show up when expression evaluation actually failed. This helps with expression evaluation failure reports in mixed language environments where it's not quite clear what language the expression ran as. It may also reduce confusion around why the expression evaluator ran an expression in a language it wasn't asked to run (a softer alternative to what I attempted in #156648). Here are some example outputs:
I didn't put the diagnostic on the same line as the inline diagnostic for now because of implementation convenience, but if reviewers deem that a blocker I can take a stab at that again. Also, other language plugins (namely Swift), won't immediately benefit from this and will have to emit their own diagnistc. I played around with having a virtual API on rdar://160297649 Full diff: https://github.com/llvm/llvm-project/pull/161688.diff 5 Files Affected:
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
index 924953cc43fa2..77a8afa511a6a 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -74,6 +74,7 @@
#include "lldb/Core/Debugger.h"
#include "lldb/Core/Disassembler.h"
#include "lldb/Core/Module.h"
+#include "lldb/Expression/DiagnosticManager.h"
#include "lldb/Expression/IRExecutionUnit.h"
#include "lldb/Expression/IRInterpreter.h"
#include "lldb/Host/File.h"
@@ -527,7 +528,8 @@ static void SetupTargetOpts(CompilerInstance &compiler,
static void SetupLangOpts(CompilerInstance &compiler,
ExecutionContextScope &exe_scope,
- const Expression &expr) {
+ const Expression &expr,
+ DiagnosticManager &diagnostic_manager) {
Log *log = GetLog(LLDBLog::Expressions);
// If the expression is being evaluated in the context of an existing stack
@@ -547,6 +549,8 @@ static void SetupLangOpts(CompilerInstance &compiler,
: lldb::eLanguageTypeUnknown),
lldb_private::Language::GetNameForLanguageType(language));
+ lldb::LanguageType language_for_note = language;
+
LangOptions &lang_opts = compiler.getLangOpts();
switch (language) {
@@ -560,12 +564,14 @@ static void SetupLangOpts(CompilerInstance &compiler,
// family language, because the expression parser uses features of C++ to
// capture values.
lang_opts.CPlusPlus = true;
+ language_for_note = lldb::eLanguageTypeC_plus_plus;
break;
case lldb::eLanguageTypeObjC:
lang_opts.ObjC = true;
// FIXME: the following language option is a temporary workaround,
// to "ask for ObjC, get ObjC++" (see comment above).
lang_opts.CPlusPlus = true;
+ language_for_note = lldb::eLanguageTypeObjC_plus_plus;
// Clang now sets as default C++14 as the default standard (with
// GNU extensions), so we do the same here to avoid mismatches that
@@ -610,6 +616,21 @@ static void SetupLangOpts(CompilerInstance &compiler,
break;
}
+ if (language_for_note != language)
+ diagnostic_manager.AddDiagnostic(
+ llvm::formatv(
+ "Requested expression evaluation as '{0}' but fell back to '{1}'.",
+ lldb_private::Language::GetNameForLanguageType(language),
+ lldb_private::Language::GetNameForLanguageType(language_for_note))
+ .str(),
+ lldb::Severity::eSeverityInfo, DiagnosticOrigin::eDiagnosticOriginLLDB);
+ else
+ diagnostic_manager.AddDiagnostic(
+ llvm::formatv("Requested expression evaluation as '{0}'",
+ lldb_private::Language::GetNameForLanguageType(language))
+ .str(),
+ lldb::Severity::eSeverityInfo, DiagnosticOrigin::eDiagnosticOriginLLDB);
+
lang_opts.Bool = true;
lang_opts.WChar = true;
lang_opts.Blocks = true;
@@ -687,8 +708,8 @@ static void SetupImportStdModuleLangOpts(CompilerInstance &compiler,
ClangExpressionParser::ClangExpressionParser(
ExecutionContextScope *exe_scope, Expression &expr,
- bool generate_debug_info, std::vector<std::string> include_directories,
- std::string filename)
+ bool generate_debug_info, DiagnosticManager &diagnostic_manager,
+ std::vector<std::string> include_directories, std::string filename)
: ExpressionParser(exe_scope, expr, generate_debug_info), m_compiler(),
m_pp_callbacks(nullptr),
m_include_directories(std::move(include_directories)),
@@ -754,7 +775,7 @@ ClangExpressionParser::ClangExpressionParser(
}
// 4. Set language options.
- SetupLangOpts(*m_compiler, *exe_scope, expr);
+ SetupLangOpts(*m_compiler, *exe_scope, expr, diagnostic_manager);
auto *clang_expr = dyn_cast<ClangUserExpression>(&m_expr);
if (clang_expr && clang_expr->DidImportCxxModules()) {
LLDB_LOG(log, "Adding lang options for importing C++ modules");
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.h b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.h
index 93e0b007dbcc8..734ad51c9646e 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.h
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.h
@@ -65,6 +65,7 @@ class ClangExpressionParser : public ExpressionParser {
/// diagnostics (i.e. errors, warnings or notes from Clang).
ClangExpressionParser(ExecutionContextScope *exe_scope, Expression &expr,
bool generate_debug_info,
+ DiagnosticManager &diagnostic_manager,
std::vector<std::string> include_directories = {},
std::string filename = "<clang expression>");
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangFunctionCaller.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangFunctionCaller.cpp
index e4a094f3aa512..d2db319afb7a0 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangFunctionCaller.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangFunctionCaller.cpp
@@ -189,8 +189,8 @@ ClangFunctionCaller::CompileFunction(lldb::ThreadSP thread_to_use_sp,
lldb::ProcessSP jit_process_sp(m_jit_process_wp.lock());
if (jit_process_sp) {
const bool generate_debug_info = true;
- auto *clang_parser = new ClangExpressionParser(jit_process_sp.get(), *this,
- generate_debug_info);
+ auto *clang_parser = new ClangExpressionParser(
+ jit_process_sp.get(), *this, generate_debug_info, diagnostic_manager);
num_errors = clang_parser->Parse(diagnostic_manager);
m_parser.reset(clang_parser);
} else {
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
index 6b743e29e21f6..e8d5ec3c7fd96 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
@@ -574,7 +574,7 @@ bool ClangUserExpression::TryParse(
m_parser = std::make_unique<ClangExpressionParser>(
exe_ctx.GetBestExecutionContextScope(), *this, generate_debug_info,
- m_include_directories, m_filename);
+ diagnostic_manager, m_include_directories, m_filename);
unsigned num_errors = m_parser->Parse(diagnostic_manager);
@@ -818,7 +818,7 @@ bool ClangUserExpression::Complete(ExecutionContext &exe_ctx,
}
ClangExpressionParser parser(exe_ctx.GetBestExecutionContextScope(), *this,
- false);
+ false, diagnostic_manager);
// We have to find the source code location where the user text is inside
// the transformed expression code. When creating the transformed text, we
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp
index 1f44200c4cff8..e6983066a12fa 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangUtilityFunction.cpp
@@ -120,7 +120,7 @@ bool ClangUtilityFunction::Install(DiagnosticManager &diagnostic_manager,
const bool generate_debug_info = true;
ClangExpressionParser parser(exe_ctx.GetBestExecutionContextScope(), *this,
- generate_debug_info);
+ generate_debug_info, diagnostic_manager);
unsigned num_errors = parser.Parse(diagnostic_manager);
|
This going to be quite useful for diagnosing perplexing errors!
|
Not that I'm aware of. Could try creating one
Yea we could elaborate. I didn't want to make the |
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.
Very nice. I looked at the example before reading the PR description and I wondered (1) if this would show up even if the expression succeeded and (2) if this would show up if the languages are the same. The answer to both is no which I believe makes sense.
5e3b5e8
to
224af5e
Compare
…1803) The intention for this API is to be used when presenting language names to the user, e.g., in expression evaluation diagnostics or LLDB errors. Most uses of `GetNameForLanguageType` can be probably replaced with `GetDisplayNameForLanguageType`, but that's out of scope of this PR. This uses `llvm::dwarf::LanguageDescription` under the hood, so we would lose the version numbers in the names. If we deem those to be important, we could switch to an implementation that hardcodes a list of user-friendly names with version numbers included. The intention is to use it from #161688 Depends on #161804
…pe API (#161803) The intention for this API is to be used when presenting language names to the user, e.g., in expression evaluation diagnostics or LLDB errors. Most uses of `GetNameForLanguageType` can be probably replaced with `GetDisplayNameForLanguageType`, but that's out of scope of this PR. This uses `llvm::dwarf::LanguageDescription` under the hood, so we would lose the version numbers in the names. If we deem those to be important, we could switch to an implementation that hardcodes a list of user-friendly names with version numbers included. The intention is to use it from llvm/llvm-project#161688 Depends on llvm/llvm-project#161804
cc2ca49
to
d3d00cf
Compare
d3d00cf
to
d5457cd
Compare
d5457cd
to
d62e766
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
…162048) Currently `llvm::dwarf::LanguageDescription` returns a stringified `DW_LNAME`. It would be useful to have an API that returns the language name for a particular `DW_LNAME_`/version pair. LLDB's use case is that it wants to emit diagnostics with human readable descriptions of the language we got from debug-info (see #161688). We could maintain a side-table in LLDB but thought this might be generally useful and should live next to the existing `LanguageDescription` API.
…r version (#162048) Currently `llvm::dwarf::LanguageDescription` returns a stringified `DW_LNAME`. It would be useful to have an API that returns the language name for a particular `DW_LNAME_`/version pair. LLDB's use case is that it wants to emit diagnostics with human readable descriptions of the language we got from debug-info (see llvm/llvm-project#161688). We could maintain a side-table in LLDB but thought this might be generally useful and should live next to the existing `LanguageDescription` API.
d62e766
to
6ae18a7
Compare
…e used for expression evaluation Since it's a 'Note' diagnostic it would only show up when expression evaluation actually failed. This helps with expression evaluation failure reports in mixed language environments where it's not quite clear what language the expression ran as. It may also reduce confusion around why the expression evaluator ran an expression in a language it wasn't asked to run (a softer alternative to what I attempted in llvm#156648). Here are some example outputs: ``` (lldb) expr -l c -- blah ˄ ╰─ error: use of undeclared identifier 'blah' note: Requested expression evaluation as 'c' but fell back to 'c++'. (lldb) expr -l c++ -- blah ˄ ╰─ error: use of undeclared identifier 'blah' note: Requested expression evaluation as c++ (lldb) expr -l objc -- blah ˄ ╰─ error: use of undeclared identifier 'blah' note: Requested expression evaluation as 'objective-c' but fell back to 'objective-c++'. (lldb) expr -l rust -- blah ˄ ╰─ error: use of undeclared identifier 'blah' note: Requested expression evaluation as rust ``` I didn't put the diagnostic on the same line as the inline diagnostic for now because of implementation convenience, but if reviewers deem that a blocker I can take a stab at that again. Also, other language plugins (namely Swift), won't immediately benefit from this and will have to emit their own diagnistc. I played around with having a virtual API on `UserExpression` or `ExpressionParser` that will be called consistently, but by the time we're about to parse the expression we are already several frames deep into the plugin. Before (and at the beginning of) the generic `UserExpression::Parse` call we don't have enough information to notify which language we're going to parse in (at least for the C++ plugin). rdar://160297649
6ae18a7
to
4fd3ee0
Compare
expr blah | ||
|
||
# CHECK-NO-TARGET: (lldb) expr | ||
# CHECK-NO-TARGET: note: Falling back to default language. Ran expression as 'Objective C++'. |
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.
Should there be another test that says expr 1
with no arguments that checks that the note isn't printed when expression is successful?
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.
There were such tests (but I specified the language explicitly). Added another case where i don't specify the language
…162048) Currently `llvm::dwarf::LanguageDescription` returns a stringified `DW_LNAME`. It would be useful to have an API that returns the language name for a particular `DW_LNAME_`/version pair. LLDB's use case is that it wants to emit diagnostics with human readable descriptions of the language we got from debug-info (see #161688). We could maintain a side-table in LLDB but thought this might be generally useful and should live next to the existing `LanguageDescription` API.
…lvm#162048) Currently `llvm::dwarf::LanguageDescription` returns a stringified `DW_LNAME`. It would be useful to have an API that returns the language name for a particular `DW_LNAME_`/version pair. LLDB's use case is that it wants to emit diagnostics with human readable descriptions of the language we got from debug-info (see llvm#161688). We could maintain a side-table in LLDB but thought this might be generally useful and should live next to the existing `LanguageDescription` API.
…ge used for expression evaluation (llvm#161688) Depends on: * llvm#162050 Since it's a 'Note' diagnostic it would only show up when expression evaluation actually failed. This helps with expression evaluation failure reports in mixed language environments where it's not quite clear what language the expression ran as. It may also reduce confusion around why the expression evaluator ran an expression in a language it wasn't asked to run (a softer alternative to what I attempted in llvm#156648). Here are some example outputs: ``` # Without target (lldb) expr blah note: Falling back to default language. Ran expression as 'Objective C++'. # Stopped in target (lldb) expr blah note: Ran expression as 'C++14'. (lldb) expr -l objc -- blah note: Expression evaluation in pure Objective-C not supported. Ran expression as 'Objective C++'. (lldb) expr -l c -- blah note: Expression evaluation in pure C not supported. Ran expression as 'ISO C++'. (lldb) expr -l c++14 -- blah note: Ran expression as 'C++14' (lldb) expr -l c++20 -- blah note: Ran expression as 'C++20' (lldb) expr -l objective-c++ -- blah note: Ran expression as 'Objective C++' (lldb) expr -l D -- blah note: Expression evaluation in D not supported. Falling back to default language. Ran expression as 'Objective C++'. ``` I didn't put the diagnostic on the same line as the inline diagnostic for now because of implementation convenience, but if reviewers deem that a blocker I can take a stab at that again. Also, other language plugins (namely Swift), won't immediately benefit from this and will have to emit their own diagnistc. I played around with having a virtual API on `UserExpression` or `ExpressionParser` that will be called consistently, but by the time we're about to parse the expression we are already several frames deep into the plugin. Before (and at the beginning of) the generic `UserExpression::Parse` call we don't have enough information to notify which language we're going to parse in (at least for the C++ plugin). rdar://160297649 rdar://159669244 (cherry picked from commit e3620fe)
The buildbot lldb-x86_64-win is broken. |
Will take a look. Seems like the default language versions are different on windows. Just have to adjust the test cases a bit |
…lvm#162048) Currently `llvm::dwarf::LanguageDescription` returns a stringified `DW_LNAME`. It would be useful to have an API that returns the language name for a particular `DW_LNAME_`/version pair. LLDB's use case is that it wants to emit diagnostics with human readable descriptions of the language we got from debug-info (see llvm#161688). We could maintain a side-table in LLDB but thought this might be generally useful and should live next to the existing `LanguageDescription` API. (cherry picked from commit 030d8e6)
…m#161803) The intention for this API is to be used when presenting language names to the user, e.g., in expression evaluation diagnostics or LLDB errors. Most uses of `GetNameForLanguageType` can be probably replaced with `GetDisplayNameForLanguageType`, but that's out of scope of this PR. This uses `llvm::dwarf::LanguageDescription` under the hood, so we would lose the version numbers in the names. If we deem those to be important, we could switch to an implementation that hardcodes a list of user-friendly names with version numbers included. The intention is to use it from llvm#161688 Depends on llvm#161804 (cherry picked from commit 2c37244)
|
Seems to have fixed the API test failure. But shell test still fails. @slydiman any chance you can run the shell test command locally and post the full output:
The test log is not enough for me to go on. We can skip it on Windows for now if you need to unblock the bots urgently. |
Depends on:
Since it's a 'Note' diagnostic it would only show up when expression evaluation actually failed. This helps with expression evaluation failure reports in mixed language environments where it's not quite clear what language the expression ran as. It may also reduce confusion around why the expression evaluator ran an expression in a language it wasn't asked to run (a softer alternative to what I attempted in #156648).
Here are some example outputs:
I didn't put the diagnostic on the same line as the inline diagnostic for now because of implementation convenience, but if reviewers deem that a blocker I can take a stab at that again.
Also, other language plugins (namely Swift), won't immediately benefit from this and will have to emit their own diagnistc. I played around with having a virtual API on
UserExpression
orExpressionParser
that will be called consistently, but by the time we're about to parse the expression we are already several frames deep into the plugin. Before (and at the beginning of) the genericUserExpression::Parse
call we don't have enough information to notify which language we're going to parse in (at least for the C++ plugin).rdar://160297649
rdar://159669244