Reland "[lldb] Don't send invalid region addresses to lldb server"

[DavidSpickett]

Summary

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.

[DavidSpickett]

This is probably pretty trivial but my first dive into win32 stuff so wanted to double check my logic before relanding.

Originally reviewed here: https://reviews.llvm.org/D87694
The failure on one of the buildbots: http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/18988/steps/test/logs/stdio

References:
https://docs.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-virtualqueryex
https://docs.microsoft.com/en-us/windows/win32/api/errhandlingapi/nf-errhandlingapi-getlasterror

[stella.stamenova]

lldb/source/Commands/CommandObjectMemory.cpp

1716–1717

Actually, I would change the logic here a little bit to make it easier to read. Right now it is:

if (argc > 1 || ... ) {
} else {
  if (GetArgumentCount() == 1) {
  }
  ...
}

It should be:

if (argc > 1 || ... ) {
} else if (argc == 1) { //since argc already has the value of GetArgumentCount()
}

if (result.Succeeded()) {
 ...
}

This will make the function more readable, fixing the bug that you found, preserving most of its logic and keeping the single return.

lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.cpp

421

This GetLastError call is not guaranteed to return the same error as above since the error could be cleared by a call to GetLastError. You should instead do:

if (result == 0) {
   DWORD lastError = ::GetLastError();
   if (lastError == ERROR_INVALID_PARAMETER) {
   }
   else {
   }
}

[DavidSpickett]

• Convert memory region to early return style to make the logic clearer
• Only call ::GetLastError once when VirtualQueryEx fails

[DavidSpickett]

lldb/source/Commands/CommandObjectMemory.cpp

1716–1717

You're the second person to bring this up, so I've converted the logic to return early. (I know that's more than you asked but I think it helps overall)

[DavidSpickett]

• clang-format

[stella.stamenova]

LGTM

[labath]

Thanks for tracking this down.

lldb/source/Commands/CommandObjectMemory.cpp

CommandObjectMemoryRegion(CommandInterpreter &interpreter)
                                 eCommandProcessMustBeLaunched),
         m_prev_end_addr(LLDB_INVALID_ADDRESS) {}
 
   ~CommandObjectMemoryRegion() override = default;
 
 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;
-      m_prev_end_addr = LLDB_INVALID_ADDRESS;
+    if (!process_sp) {
+      m_prev_end_addr = LLDB_INVALID_ADDRESS;
+      result.AppendError("invalid process");
+      result.SetStatus(eReturnStatusFailed);
+      return false;
+    }
+
 
-      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);
-      } 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;
+    }
+
+    if (argc == 1) {
+        result.AppendErrorWithFormat("invalid address argument \"%s\": %s\n",
+                                     command[0].c_str(), error.AsCString());
+        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());
-        }
-      }
-    } else {
-      m_prev_end_addr = LLDB_INVALID_ADDRESS;
-      result.AppendError("invalid process");
-      result.SetStatus(eReturnStatusFailed);
-    }
-    return result.Succeeded();
+          range_info.GetWritable(), range_info.GetExecutable(), name ? " " : "",
+          name, section_name ? " " : "", section_name);
+      return true;
+
+    result.AppendErrorWithFormat("%s\n", error.AsCString());
+    return false;
   }
 
   const char *GetRepeatCommand(Args &current_command_args,
                                uint32_t index) override {
     // If we repeat this command, repeat it without any arguments so we can
     // show the next memory range
     return m_cmd_name.c_str();
   }

lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.cpp

Status ProcessDebugger::GetMemoryRegionInfo(lldb::addr_t vm_addr,
   }
 
   LLDB_LOG(log, "getting info for address {0:x}", vm_addr);
 
   void *addr = reinterpret_cast<void *>(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
       info.GetRange().SetRangeBase(vm_addr);
       info.GetRange().SetRangeEnd(LLDB_INVALID_ADDRESS);
       info.SetReadable(MemoryRegionInfo::eNo);
       info.SetExecutable(MemoryRegionInfo::eNo);
       info.SetWritable(MemoryRegionInfo::eNo);
       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}",
                error, vm_addr);
       return error;
     }
   }
 
if (mem_info.State != MEM_FREE) {
     SYSTEM_INFO data;
     ::GetSystemInfo(&data);
     DWORD page_offset = vm_addr % data.dwPageSize;
     info.GetRange().SetRangeBase(vm_addr);
     info.GetRange().SetByteSize(mem_info.RegionSize - page_offset);
     info.SetMapped(MemoryRegionInfo::eNo);
   }
 
-  error.SetError(::GetLastError(), eErrorTypeWin32);
   LLDB_LOGV(log,
             "Memory region info for address {0}: readable={1}, "
             "executable={2}, writable={3}",
             vm_addr, info.GetReadable(), info.GetExecutable(),
             info.GetWritable());
   return error;
 }
 

lldb/test/API/functionalities/memory-region/TestMemoryRegion.py

def test(self):
         interp = self.dbg.GetCommandInterpreter()
         result = lldb.SBCommandReturnObject()
 
         # Test that the first 'memory region' command prints the usage.
         interp.HandleCommand("memory region", result)
         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())
         self.assertRegexpMatches(result.GetOutput(), "\\[0x0+-")
 
         # Keep printing memory regions until we printed all of them.
         while True:
             interp.HandleCommand("memory region", result)
             if not result.Succeeded():
                 break
 
         # Now that we reached the end, 'memory region' should again print the usage.
         interp.HandleCommand("memory region", result)
         self.assertFalse(result.Succeeded())
         self.assertRegexpMatches(result.GetError(), "Usage: memory region ADDR")