[Lldb-commits] [lldb] 71cf97e - Reland "[lldb] Don't send invalid region addresses to lldb server"
David Spickett via lldb-commits
lldb-commits at lists.llvm.org
Mon Oct 5 03:50:39 PDT 2020
Author: David Spickett
Date: 2020-10-05T11:50:29+01:00
New Revision: 71cf97e95b8c888367284d1d12925f79b38034eb
URL: https://github.com/llvm/llvm-project/commit/71cf97e95b8c888367284d1d12925f79b38034eb
DIFF: https://github.com/llvm/llvm-project/commit/71cf97e95b8c888367284d1d12925f79b38034eb.diff
LOG: Reland "[lldb] Don't send invalid region addresses to lldb server"
This reverts commit c65627a1fe3be7521fc232d633bb6df577f55269.
The test immediately after the new invalid symbol test was
failing on Windows. This was because when we called
VirtualQueryEx to get the region info for 0x0,
even if it succeeded we would call GetLastError.
Which must have picked up the last error that was set while
trying to lookup "not_an_address". Which happened to be 2.
("The system cannot find the file specified.")
To fix this only call GetLastError when we know VirtualQueryEx
has failed. (when it returns 0, which we were also checking for anyway)
Also convert memory region to an early return style
to make the logic clearer.
Reviewed By: labath, stella.stamenova
Differential Revision: https://reviews.llvm.org/D88229
Added:
Modified:
lldb/source/Commands/CommandObjectMemory.cpp
lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.cpp
lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
Removed:
################################################################################
diff --git a/lldb/source/Commands/CommandObjectMemory.cpp b/lldb/source/Commands/CommandObjectMemory.cpp
index 474c37710149..d4c2808dc159 100644
--- a/lldb/source/Commands/CommandObjectMemory.cpp
+++ b/lldb/source/Commands/CommandObjectMemory.cpp
@@ -1687,63 +1687,66 @@ class CommandObjectMemoryRegion : public CommandObjectParsed {
protected:
bool DoExecute(Args &command, CommandReturnObject &result) override {
ProcessSP process_sp = m_exe_ctx.GetProcessSP();
- if (process_sp) {
- Status error;
- lldb::addr_t load_addr = m_prev_end_addr;
+ if (!process_sp) {
m_prev_end_addr = LLDB_INVALID_ADDRESS;
+ result.AppendError("invalid process");
+ result.SetStatus(eReturnStatusFailed);
+ return false;
+ }
+
+ Status error;
+ lldb::addr_t load_addr = m_prev_end_addr;
+ m_prev_end_addr = LLDB_INVALID_ADDRESS;
- const size_t argc = command.GetArgumentCount();
- if (argc > 1 || (argc == 0 && load_addr == LLDB_INVALID_ADDRESS)) {
- result.AppendErrorWithFormat("'%s' takes one argument:\nUsage: %s\n",
- m_cmd_name.c_str(), m_cmd_syntax.c_str());
+ const size_t argc = command.GetArgumentCount();
+ if (argc > 1 || (argc == 0 && load_addr == LLDB_INVALID_ADDRESS)) {
+ result.AppendErrorWithFormat("'%s' takes one argument:\nUsage: %s\n",
+ m_cmd_name.c_str(), m_cmd_syntax.c_str());
+ result.SetStatus(eReturnStatusFailed);
+ return false;
+ }
+
+ if (argc == 1) {
+ auto load_addr_str = command[0].ref();
+ load_addr = OptionArgParser::ToAddress(&m_exe_ctx, load_addr_str,
+ LLDB_INVALID_ADDRESS, &error);
+ if (error.Fail() || load_addr == LLDB_INVALID_ADDRESS) {
+ result.AppendErrorWithFormat("invalid address argument \"%s\": %s\n",
+ command[0].c_str(), error.AsCString());
result.SetStatus(eReturnStatusFailed);
- } else {
- if (command.GetArgumentCount() == 1) {
- auto load_addr_str = command[0].ref();
- load_addr = OptionArgParser::ToAddress(&m_exe_ctx, load_addr_str,
- LLDB_INVALID_ADDRESS, &error);
- if (error.Fail() || load_addr == LLDB_INVALID_ADDRESS) {
- result.AppendErrorWithFormat(
- "invalid address argument \"%s\": %s\n", command[0].c_str(),
- error.AsCString());
- result.SetStatus(eReturnStatusFailed);
- }
- }
+ return false;
+ }
+ }
- lldb_private::MemoryRegionInfo range_info;
- error = process_sp->GetMemoryRegionInfo(load_addr, range_info);
- if (error.Success()) {
- lldb_private::Address addr;
- ConstString name = range_info.GetName();
- ConstString section_name;
- if (process_sp->GetTarget().ResolveLoadAddress(load_addr, addr)) {
- SectionSP section_sp(addr.GetSection());
- if (section_sp) {
- // Got the top most section, not the deepest section
- while (section_sp->GetParent())
- section_sp = section_sp->GetParent();
- section_name = section_sp->GetName();
- }
- }
- result.AppendMessageWithFormatv(
- "[{0:x16}-{1:x16}) {2:r}{3:w}{4:x}{5}{6}{7}{8}\n",
- range_info.GetRange().GetRangeBase(),
- range_info.GetRange().GetRangeEnd(), range_info.GetReadable(),
- range_info.GetWritable(), range_info.GetExecutable(),
- name ? " " : "", name, section_name ? " " : "", section_name);
- m_prev_end_addr = range_info.GetRange().GetRangeEnd();
- result.SetStatus(eReturnStatusSuccessFinishResult);
- } else {
- result.SetStatus(eReturnStatusFailed);
- result.AppendErrorWithFormat("%s\n", error.AsCString());
+ lldb_private::MemoryRegionInfo range_info;
+ error = process_sp->GetMemoryRegionInfo(load_addr, range_info);
+ if (error.Success()) {
+ lldb_private::Address addr;
+ ConstString name = range_info.GetName();
+ ConstString section_name;
+ if (process_sp->GetTarget().ResolveLoadAddress(load_addr, addr)) {
+ SectionSP section_sp(addr.GetSection());
+ if (section_sp) {
+ // Got the top most section, not the deepest section
+ while (section_sp->GetParent())
+ section_sp = section_sp->GetParent();
+ section_name = section_sp->GetName();
}
}
- } else {
- m_prev_end_addr = LLDB_INVALID_ADDRESS;
- result.AppendError("invalid process");
- result.SetStatus(eReturnStatusFailed);
+ result.AppendMessageWithFormatv(
+ "[{0:x16}-{1:x16}) {2:r}{3:w}{4:x}{5}{6}{7}{8}\n",
+ range_info.GetRange().GetRangeBase(),
+ range_info.GetRange().GetRangeEnd(), range_info.GetReadable(),
+ range_info.GetWritable(), range_info.GetExecutable(), name ? " " : "",
+ name, section_name ? " " : "", section_name);
+ m_prev_end_addr = range_info.GetRange().GetRangeEnd();
+ result.SetStatus(eReturnStatusSuccessFinishResult);
+ return true;
}
- return result.Succeeded();
+
+ result.SetStatus(eReturnStatusFailed);
+ result.AppendErrorWithFormat("%s\n", error.AsCString());
+ return false;
}
const char *GetRepeatCommand(Args ¤t_command_args,
diff --git a/lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.cpp b/lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.cpp
index 07a81cdf69cc..91b3311dc857 100644
--- a/lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.cpp
+++ b/lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.cpp
@@ -405,7 +405,8 @@ Status ProcessDebugger::GetMemoryRegionInfo(lldb::addr_t vm_addr,
MEMORY_BASIC_INFORMATION mem_info = {};
SIZE_T result = ::VirtualQueryEx(handle, addr, &mem_info, sizeof(mem_info));
if (result == 0) {
- if (::GetLastError() == ERROR_INVALID_PARAMETER) {
+ DWORD last_error = ::GetLastError();
+ if (last_error == ERROR_INVALID_PARAMETER) {
// ERROR_INVALID_PARAMETER is returned if VirtualQueryEx is called with
// an address past the highest accessible address. We should return a
// range from the vm_addr to LLDB_INVALID_ADDRESS
@@ -417,7 +418,7 @@ Status ProcessDebugger::GetMemoryRegionInfo(lldb::addr_t vm_addr,
info.SetMapped(MemoryRegionInfo::eNo);
return error;
} else {
- error.SetError(::GetLastError(), eErrorTypeWin32);
+ error.SetError(last_error, eErrorTypeWin32);
LLDB_LOG(log,
"VirtualQueryEx returned error {0} while getting memory "
"region info for address {1:x}",
@@ -460,7 +461,6 @@ Status ProcessDebugger::GetMemoryRegionInfo(lldb::addr_t vm_addr,
info.SetMapped(MemoryRegionInfo::eNo);
}
- error.SetError(::GetLastError(), eErrorTypeWin32);
LLDB_LOGV(log,
"Memory region info for address {0}: readable={1}, "
"executable={2}, writable={3}",
diff --git a/lldb/test/API/functionalities/memory-region/TestMemoryRegion.py b/lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
index 283cc945ed09..61e64d44e794 100644
--- a/lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
+++ b/lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
@@ -41,6 +41,12 @@ def test(self):
self.assertFalse(result.Succeeded())
self.assertRegexpMatches(result.GetError(), "Usage: memory region ADDR")
+ # Test that when the address fails to parse, we show an error and do not continue
+ interp.HandleCommand("memory region not_an_address", result)
+ self.assertFalse(result.Succeeded())
+ self.assertEqual(result.GetError(),
+ "error: invalid address argument \"not_an_address\": address expression \"not_an_address\" evaluation failed\n")
+
# Now let's print the memory region starting at 0 which should always work.
interp.HandleCommand("memory region 0x0", result)
self.assertTrue(result.Succeeded())
More information about the lldb-commits
mailing list