This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by DavidSpickett on Sep 24 2020, 6:59 AM.

Details

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.

Diff Detail

Event Timeline

DavidSpickett created this revision.Sep 24 2020, 6:59 AM
Herald added a project: Restricted Project. · View Herald Transcript
DavidSpickett requested review of this revision.Sep 24 2020, 6:59 AM
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 {
   }
}
  • Convert memory region to early return style to make the logic clearer
  • Only call ::GetLastError once when VirtualQueryEx fails
DavidSpickett edited the summary of this revision. (Show Details)Sep 25 2020, 3:42 AM
DavidSpickett added inline comments.Sep 25 2020, 3:44 AM
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 marked an inline comment as done.Sep 25 2020, 3:44 AM
  • clang-format
This revision is now accepted and ready to land.Sep 25 2020, 9:31 AM
labath accepted this revision.Sep 29 2020, 4:33 AM

Thanks for tracking this down.

This revision was landed with ongoing or failed builds.Oct 5 2020, 3:50 AM
This revision was automatically updated to reflect the committed changes.