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

Pavel Labath via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Wed Sep 16 05:57:03 PDT 2020

labath accepted this revision.
labath added inline comments.
This revision is now accepted and ready to land.

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
DavidSpickett wrote:
> labath wrote:
> > DavidSpickett wrote:
> > > 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.
> > > 
> > Right, but this won't check that the second message is not displayed, will it? I thought we'd change this to an exact string match instead of a regex. It won't be consistent with the surrounding code, but it could be argued that the surrounding code should also be changed to use exact matches in order to ensure the error messages are really reasonable...
> > 
> > I'm not convinced of the value of the other test, but I won't stop you from adding it. :)
> Right, I misunderstood. (also not sure my first version did what I thought it did either)
> Changed to match the full error message, that should cover it. If you did carry on you'd get some other message that won't match, from lldb-server or otherwise.
Awesome. In fact, now that we have the full error message, we can see that's still kinda redundant. But that's for another patch...

  rG LLVM Github Monorepo



More information about the lldb-commits mailing list