[Lldb-commits] [PATCH] D87694: [lldb] Don't send invalid region addresses to lldb server

[David Spickett via Phabricator via lldb-commits]

DavidSpickett marked an inline comment as done.
DavidSpickett added inline comments.


================
Comment at: lldb/source/Commands/CommandObjectMemory.cpp:1710
             result.SetStatus(eReturnStatusFailed);
           }
         }
----------------
labath wrote:
> How about adding `return false` here to avoid indenting the code below?
I was trying to match the rest, but yes there is no sense making it more complicated.


================
Comment at: lldb/test/API/functionalities/memory-region/TestMemoryRegion.py:44-52
+        # Test that when the address fails to parse, we do not carry on
+        # and ask lldb-server for an invalid address
+        interp.HandleCommand("memory region not_an_address", result)
+        self.assertFalse(result.Succeeded())
+        self.assertRegexpMatches(result.GetError(),
+                "error: invalid address argument \"not_an_address\"")
+        # This would be found if we carried on despite the error
----------------
labath wrote:
> If you're goal is to ensure that lldb-server is really not queried, then this would require a different kind of a test.
> 
> But if (as I suspect), you just want to ensure the error message makes sense, then it would be better to just match the full text of the error message, and drop all references to lldb-server.
It's both, that you get an error that makes sense but also that we don't then carry on regardless. I suppose there's no guarantee that that second error is caused by sending a qMemoryRegion packet so it isn't very exact.

I'll add a second test that checks that the specific packets are not sent after the first error.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87694/new/

https://reviews.llvm.org/D87694