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

1701

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

420

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

1701

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

 //===-- CommandObjectMemory.cpp -------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
if (process_sp) {
       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());
         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();

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

 //===-- ProcessDebugger.cpp -----------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
if (::GetLastError() == ERROR_INVALID_PARAMETER) {
       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);
       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")