[lldb] Issue a warning when Target XML should have been used but we do not have libxml2 #170663
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…rget XML In https://discourse.llvm.org/t/does-lldb-qemu-support-dumping-x64-control-registers-such-as-cr3/89031 a user was not seeing certain registers when connected to QEMU. Turns out their LLDB build did not have libxml2 enabled. While logging is not the first thing most users will think of, it is something an expert can ask for to confirm whether they have XML support enabled. So in this PR I've shuffled the logic GetGDBServerRegisterInfo to better report problems in the log. The key one is when lldb does not have libxml2 but the server did say it supports qxfer:features. In this case we would have used it if we could, and the debug session will likely be degraded because we are not able to.
If you connect lldb without libxml2 enabled, to a debug stub, that server may offer Target XML but lldb will not use it. This often causes incorrect or missing registers. So much so that I think users should be made aware of this so they can find an lldb with libxml2 enabled. This warning will only be printed when: * The debug server offered us Target XML but lldb does not have libxml2, and - * qRegisterInfo was not supported by the debug server. This means that (lldb without libxml2) -> (lldb-server or debugserver) will not warn as we'll fall back to qRegisterInfo. All that's potentially missing is advanced register formatting information, which most people won't notice is missing anyway. If they do, the logs contain information about this. If you connect (lldb without libxml2) > (gdbserver, or a stub inspired by it) you will see this warning. As they do not support qRegisterInfo. ``` $ ./bin/lldb /tmp/test.o -o "gdb-remote 1234" (lldb) target create "/tmp/test.o" Current executable set to '/tmp/test.o' (aarch64). (lldb) gdb-remote 1234 warning: the debug server supports Target Description XML but LLDB does not have XML enabled. Using "qRegisterInfo" was also not possible. Register information may be incorrect or missing. ``` When qRegisterInfo is not supported, there are 4 possible situations. 1. The debug server offered Target XML and we do not have libxml2 We should warn the user so they can find an lldb with libxml2. 2. The debug server offered Target XML but it was not valid XML Ideally we'd warn here too, but the error handling needs more work to implement this, and there is logging for it if you know where to look. 3. The debug server did not offer Target XML and we have libxml2 4. The debug server did not offer Target XML and we do have libxml2 There's no XML to parse, so no reason to warn. The user is not getting a worse debug experience because we lack libxml2. Their only course of action here would be to replace the debug stub. Given that this occurs a lot with stubs embedded in kernels or debug hardware, they may not have a choice either.
Member
|
@llvm/pr-subscribers-lldb Author: David Spickett (DavidSpickett) ChangesIf you connect lldb without libxml2 enabled, to a debug stub, So much so that I think users should be made aware of this so This warning will only be printed when:
This means that (lldb without libxml2) -> (lldb-server or debugserver) If you connect (lldb without libxml2) > (gdbserver, or a stub inspired by it) When qRegisterInfo is not supported, there are 4 possible situations.
We should warn the user so they can find an lldb with libxml2.
Ideally we'd warn here too, but the error handling needs more
There's no XML to parse, so no reason to warn. The user is not getting Their only course of action here would be to replace the debug stub. Full diff: https://github.com/llvm/llvm-project/pull/170663.diff 2 Files Affected:
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index 3c4d9a1f1ad37..b71ae53fc4031 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -441,8 +441,16 @@ void ProcessGDBRemote::BuildDynamicRegisterInfo(bool force) {
if (!arch_to_use.IsValid())
arch_to_use = target_arch;
- if (GetGDBServerRegisterInfo(arch_to_use))
+ llvm::Error register_info_err = GetGDBServerRegisterInfo(arch_to_use);
+ if (!register_info_err) {
+ // We got the registers from target XML.
return;
+ }
+
+ Log *log = GetLog(GDBRLog::Process);
+ LLDB_LOG_ERROR(log, std::move(register_info_err),
+ "Failed to read register information from target XML: {0}");
+ LLDB_LOG(log, "Now trying to use qRegisterInfo instead.");
char packet[128];
std::vector<DynamicRegisterInfo::Register> registers;
@@ -541,7 +549,25 @@ void ProcessGDBRemote::BuildDynamicRegisterInfo(bool force) {
assert(reg_info.byte_size != 0);
registers.push_back(reg_info);
} else {
- break; // ensure exit before reg_num is incremented
+ // Only warn if we were offered Target XML and could not use it, and
+ // the qRegisterInfo fallback failed. This is something a user could
+ // take action on by getting an lldb with libxml2.
+ //
+ // It's possible we weren't offered Target XML and qRegisterInfo failed,
+ // but there's no much a user can do about that. It may be the intended
+ // way the debug stub works, so we do not warn for that case.
+ if (response_type == StringExtractorGDBRemote::eUnsupported &&
+ m_gdb_comm.GetQXferFeaturesReadSupported() &&
+ !XMLDocument::XMLEnabled()) {
+ Debugger::ReportWarning(
+ "the debug server supports Target Description XML but LLDB does "
+ "not "
+ "have XML parsing enabled. Using \"qRegisterInfo\" was also not "
+ "possible. "
+ "Register information may be incorrect or missing.",
+ GetTarget().GetDebugger().GetID());
+ }
+ break;
}
} else {
break;
@@ -5137,14 +5163,19 @@ void ProcessGDBRemote::AddRemoteRegisters(
// query the target of gdb-remote for extended target information returns
// true on success (got register definitions), false on failure (did not).
-bool ProcessGDBRemote::GetGDBServerRegisterInfo(ArchSpec &arch_to_use) {
- // Make sure LLDB has an XML parser it can use first
- if (!XMLDocument::XMLEnabled())
- return false;
-
- // check that we have extended feature read support
+llvm::Error ProcessGDBRemote::GetGDBServerRegisterInfo(ArchSpec &arch_to_use) {
+ // If the remote does not offer XML, does not matter if we would have been
+ // able to parse it.
if (!m_gdb_comm.GetQXferFeaturesReadSupported())
- return false;
+ return llvm::createStringError(
+ llvm::inconvertibleErrorCode(),
+ "the debug server does not support \"qXfer:features:read\"");
+
+ if (!XMLDocument::XMLEnabled())
+ return llvm::createStringError(
+ llvm::inconvertibleErrorCode(),
+ "the debug server supports \"qXfer:features:read\", but LLDB does not "
+ "have XML parsing enabled (check LLLDB_ENABLE_LIBXML2)");
// These hold register type information for the whole of target.xml.
// target.xml may include further documents that
@@ -5161,7 +5192,11 @@ bool ProcessGDBRemote::GetGDBServerRegisterInfo(ArchSpec &arch_to_use) {
!registers.empty())
AddRemoteRegisters(registers, arch_to_use);
- return m_register_info_sp->GetNumRegisters() > 0;
+ return m_register_info_sp->GetNumRegisters() > 0
+ ? llvm::ErrorSuccess()
+ : llvm::createStringError(
+ llvm::inconvertibleErrorCode(),
+ "the debug server did not describe any registers");
}
llvm::Expected<LoadedModuleInfoList> ProcessGDBRemote::GetLoadedModuleList() {
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
index eb33b52b57441..b7e8777c9e12e 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -416,7 +416,7 @@ class ProcessGDBRemote : public Process,
void AddRemoteRegisters(std::vector<DynamicRegisterInfo::Register> ®isters,
const ArchSpec &arch_to_use);
// Query remote GDBServer for register information
- bool GetGDBServerRegisterInfo(ArchSpec &arch);
+ llvm::Error GetGDBServerRegisterInfo(ArchSpec &arch);
lldb::ModuleSP LoadModuleAtAddress(const FileSpec &file,
lldb::addr_t link_map,
|
Collaborator
Author
|
This is stacked on #170478. The most recent 2 commits are the new code. Between war and peace in the commit message and some more comments in the code, I hope my reasoning is clear now and would be in future. In that future we may be more verbose for other failure modes but for now I am being conservative and just highlighting this one. |
Collaborator
Author
|
I also want to add some instructions on https://lldb.llvm.org/use/troubleshooting.html about diagnoising this and why the warning may happen. I'll do that if this is accepted. This will create a warning -> Google -> our docs path I hope. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If you connect lldb without libxml2 enabled, to a debug stub,
that server may offer Target XML but lldb will not use it. This
often causes incorrect or missing registers.
So much so that I think users should be made aware of this so
they can find an lldb with libxml2 enabled.
This warning will only be printed when:
This means that (lldb without libxml2) -> (lldb-server or debugserver)
will not warn as we'll fall back to qRegisterInfo. All that's potentially
missing is advanced register formatting information, which most people
won't notice is missing anyway. If they do, the logs contain information
about this.
If you connect (lldb without libxml2) > (gdbserver, or a stub inspired by it)
you will see this warning. As they do not support qRegisterInfo.
When qRegisterInfo is not supported, there are 4 possible situations.
We should warn the user so they can find an lldb with libxml2.
Ideally we'd warn here too, but the error handling needs more
work to implement this, and there is logging for it if you know
where to look.
There's no XML to parse, so no reason to warn. The user is not getting
a worse debug experience because we lack libxml2.
Their only course of action here would be to replace the debug stub.
Given that this occurs a lot with stubs embedded in kernels or debug
hardware, they may not have a choice either.