Page MenuHomePhabricator

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

Authored by DavidSpickett on Sep 15 2020, 7:45 AM.



Previously when <addr> in "memory region <addr>" didn't
parse correctly, we'd print an error then also ask lldb-server
for a region containing LLDB_INVALID_ADDRESS.

(lldb) memory region not_an_address
error: invalid address argument "not_an_address"...
error: Server returned invalid range

Only send the command to lldb-server if the address
parsed correctly.

(lldb) memory region not_an_address
error: invalid address argument "not_an_address"...

Diff Detail

Unit TestsFailed

330 mswindows > LLVM.DebugInfo::debuglineinfo-path.ll
Script: -- : 'RUN: at line 7'; c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\llc.exe -mtriple=x86_64-pc-windows-gnu -O0 -filetype=obj -o C:\ws\w16c2-1\llvm-project\premerge-checks\build\test\DebugInfo\Output\debuglineinfo-path.ll.tmp < C:\ws\w16c2-1\llvm-project\premerge-checks\llvm\test\DebugInfo\debuglineinfo-path.ll
100 mswindows > LLVM.DebugInfo::symbolize-build-id.test
Script: -- : 'RUN: at line 1'; c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\yaml2obj.exe C:\ws\w16c2-1\llvm-project\premerge-checks\llvm\test\DebugInfo\symbolize-build-id.test -o C:\ws\w16c2-1\llvm-project\premerge-checks\build\test\DebugInfo\Output\symbolize-build-id.test.tmp
50 mswindows > LLVM.DebugInfo::symbolize-gnu-debuglink.test
Script: -- : 'RUN: at line 1'; c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\llvm-symbolizer.exe --obj=C:\ws\w16c2-1\llvm-project\premerge-checks\llvm\test\DebugInfo/Inputs/dwarfdump-test.elf-x86-64.debuglink 0x40113f | c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16c2-1\llvm-project\premerge-checks\llvm\test\DebugInfo\symbolize-gnu-debuglink.test
60 mswindows > LLVM.DebugInfo::symbolize.test
Script: -- : 'RUN: at line 1'; echo "C:\ws\w16c2-1\llvm-project\premerge-checks\llvm\test\DebugInfo/Inputs/dwarfdump-test.elf-x86-64 0x40113f" > C:\ws\w16c2-1\llvm-project\premerge-checks\build\test\DebugInfo\Output\symbolize.test.tmp.input
90 mswindows > LLVM.DebugInfo/X86::2011-09-26-GlobalVarContext.ll
Script: -- : 'RUN: at line 1'; c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\llc.exe -mtriple=x86_64-pc-linux-gnu C:\ws\w16c2-1\llvm-project\premerge-checks\llvm\test\DebugInfo\X86\2011-09-26-GlobalVarContext.ll -o C:\ws\w16c2-1\llvm-project\premerge-checks\build\test\DebugInfo\X86\Output\2011-09-26-GlobalVarContext.ll.tmp -filetype=obj
View Full Test Results (10 Failed)

Event Timeline

DavidSpickett created this revision.Sep 15 2020, 7:45 AM
Herald added a project: Restricted Project. · View Herald Transcript
DavidSpickett requested review of this revision.Sep 15 2020, 7:45 AM
labath added a subscriber: labath.Sep 16 2020, 1:25 AM
labath added inline comments.

How about adding return false here to avoid indenting the code below?


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.

  • Return early
  • Only check for expected error in TestMemoryRegion
DavidSpickett marked an inline comment as done.Sep 16 2020, 1:56 AM
DavidSpickett added inline comments.

I was trying to match the rest, but yes there is no sense making it more complicated.


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.

labath added inline comments.Sep 16 2020, 3:17 AM

Yeah, that's the old style of things, but we're currently changing things to use early exits where possible. If I was doing this, I'd early-exitify the whole function, but I did not want to make you do that for such a simple fix.


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. :)

DavidSpickett marked an inline comment as done.
  • Check complete error message instead of using regex
DavidSpickett added inline comments.Sep 16 2020, 4:02 AM

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.

labath accepted this revision.Sep 16 2020, 5:57 AM
labath added inline comments.

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...

This revision is now accepted and ready to land.Sep 16 2020, 5:57 AM
This revision was landed with ongoing or failed builds.Sep 17 2020, 2:26 AM
This revision was automatically updated to reflect the committed changes.