Add MSP430 to the list of available targets, implement MSP430 ABI, add support for debugging targets with 16-bit address size.
The update is intended for use with MSPDebug, a GDB server implementation for MSP430.
Paths
| Differential D146965
[lldb] Add support for MSP430 in LLDB. ClosedPublic Authored by kuilpd on Mar 27 2023, 7:44 AM.
Details Summary Add MSP430 to the list of available targets, implement MSP430 ABI, add support for debugging targets with 16-bit address size. The update is intended for use with MSPDebug, a GDB server implementation for MSP430.
Diff Detail Event TimelineComment Actions @JDevlieghere @bulbazord I don't think I know lldb codebase well enough to cover the complete review here. But from MSP430-standpoint things seem to be correct. Will you be able to review or suggest some other reviewers? Thanks! Comment Actions I'm not too familiar with MSP430 but the general idea looks fine. Others may have comments about areas I'm less familiar with. One concern I have is that there are no tests. I can understand that it may be difficult to get automated tests running testing the debugging of an architecture like MSP430 but there are things we can test to sanity test MSP430 support... At the very least, adding a unit test for the new ArchSpec support would be good -- llvm-project/lldb/unittests/Utility/ArchSpecTest.cpp.
Comment Actions Added asserts in default cases, added checks for architecture instead of address size, added a unit test. Comment Actions Wrong previous diff. Added asserts in default cases, added checks for architecture instead of address size, added a unit test. Comment Actions
Two things come to mind:
Comment Actions
It is completely bare metal, but the address space is only 16-bit, so would the entire 64 KB of memory count as a core dump?
mspdebug (the tool that implements gdb server for MSP430) doesn't have a lot of features, doesn't support memory region info or even target.xml with register information, so it all comes down to fallback information in LLDB. Comment Actions
It's more "world" than "core" but yeah why not. Probably won't need all 64k though, see below...
Yes that would be great. Plus it documents our expectations (or lack of) from this third party debug stub.
Backtrace is the obvious thing, assuming you have implemented any of that yet. What you could do is dump the stack from a real device and use the previously mentioned fake GDB remote to replay it to lldb. Other things:
If the feature mostly relies on the existing msp430 stub being correct, there's not much point testing it. For anything where lldb has to make some msp430 specific decision, see if you can mock up a gdb remote to test it.
Comment Actions
Looked into core dumping in LLDB a bit, I think it pretty much requires that there is an OS, and there needs to be an explicit support for the architecture in question.
Did this, I put the same packets mspdebug sends into the mockup. Comment Actions
That makes sense. I was more thinking about carving useful memory out of a raw dump of the RAM in any case, which is exactly what you've done in the test you added. If you wanted to have a corefile like experience I guess you could do that RAM dump, load it back into a simulator and connect to that. But regardless, out of scope here.
kuilpd marked 6 inline comments as done. Comment ActionsAdded comments, changed memory strings formatting, added disassembler check. Comment Actions Thanks for the test changes, looks good. There is some documentation that lists what targets we support, MSP430 should be added there. lldb/docs/index.rst is one of those. Otherwise, can anyone else think of major debug features that should be (at least) smoke tested? (of course you could do this in follow up patches if it needs more work)
Comment Actions Also please update the commit message to make it clear that this is for using lldb with mspdebug, not with lldb-server. To help us and also to help anyone who might link to this commit to share the good news :) Comment Actions I just landed https://github.com/llvm/llvm-project/commit/57c8fee1b97eb7e70513b935b765f8381a941b18 which will give you build errors. All you have to do is add an extra nullptr to your register infos. This revision is now accepted and ready to land.Apr 17 2023, 9:05 AM Comment Actions
I'm satisfied with this since there are tests. I'm not sure what might be missing but I'm sure we'll find it in time. Closed by commit rG82c02b733c77: [lldb] Add support for MSP430 in LLDB. (authored by asl). · Explain WhyApr 17 2023, 11:05 AM This revision was automatically updated to reflect the committed changes. asl added a reverting change: rG845612062389: Revert "[lldb] Add support for MSP430 in LLDB.".Apr 17 2023, 11:30 AM Comment Actions Please ensure that the patch is rebased into top of main and builds w/o errors / warnings.
Revision Contents
Diff 514435 lldb/include/lldb/Utility/ArchSpec.h
lldb/include/lldb/Utility/DataExtractor.h
lldb/source/Expression/IRMemoryMap.cpp
lldb/source/Expression/LLVMUserExpression.cpp
lldb/source/Host/common/NativeProcessProtocol.cpp
lldb/source/Plugins/ABI/CMakeLists.txt
lldb/source/Plugins/ABI/MSP430/ABISysV_msp430.h
lldb/source/Plugins/ABI/MSP430/ABISysV_msp430.cpp
lldb/source/Plugins/ABI/MSP430/CMakeLists.txt
lldb/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp
lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterFallback.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
lldb/source/Target/Platform.cpp
lldb/source/Utility/ArchSpec.cpp
lldb/test/API/functionalities/gdb_remote_client/TestMSP430MSPDebug.py
lldb/test/API/functionalities/gdb_remote_client/msp430.yaml
lldb/unittests/Utility/ArchSpecTest.cpp
|
I know this is not your code but maybe we should deconditionalize this assertion?