This is an archive of the discontinued LLVM Phabricator instance.

[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 Timeline

kuilpd created this revision.Mar 27 2023, 7:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2023, 7:44 AM
kuilpd requested review of this revision.Mar 27 2023, 7:44 AM
asl added a comment.Mar 27 2023, 10:48 AM

@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!

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.

lldb/include/lldb/Utility/DataExtractor.h
845–846

I know this is not your code but maybe we should deconditionalize this assertion?

lldb/source/Expression/IRMemoryMap.cpp
98–113

I think with the change above we should probably make this assertion more useful.

99–100

switch on the expression directly instead of creating a local?

101–109

Instead of making 8 the default, maybe we can assert on default or something? This can prevent future bugs where perhaps address_byte_size is not 8 for some reason and we do the wrong thing.

159–164

Same here, let's not assume 8 if we hit default.

171–174

Is this true for all machines where the address byte size is 2? Seems like 512 and 4096 should be based on something other than the address byte size. Platform/ABI perhaps?

lldb/source/Expression/LLVMUserExpression.cpp
336–339

Same here, this seems dependent on Platform/ABI.

lldb/source/Plugins/ABI/MSP430/ABISysV_msp430.h
14–17

You can remove these comments, I don't think we track that kind of thing anymore... I think?

52–55

nit: you can merge these

kuilpd marked 8 inline comments as done.Mar 28 2023, 9:54 AM
kuilpd added inline comments.
lldb/source/Expression/IRMemoryMap.cpp
98–113

Do we need this assert if I there is already one in the default case above?

default:
  lldbassert(false && "Invalid address size.");
  return LLDB_INVALID_ADDRESS;
kuilpd updated this revision to Diff 509080.Mar 28 2023, 11:27 AM

Added asserts in default cases, added checks for architecture instead of address size, added a unit test.

kuilpd updated this revision to Diff 509089.Mar 28 2023, 11:47 AM

Wrong previous diff.

Added asserts in default cases, added checks for architecture instead of address size, added a unit test.

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

Two things come to mind:

  • Core files (though it is embedded so is that even a thing?)
  • Mocking an msp430 remote, as we do for example in Target XML tests.
kuilpd added a comment.Apr 4 2023, 8:42 AM

Two things come to mind:

  • Core files (though it is embedded so is that even a thing?)

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?

  • Mocking an msp430 remote, as we do for example in Target XML tests.

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.
I can make a test that checks if fallback information even exists and registers have correct data and size, for example.
What else would you recommend to test for?

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?

It's more "world" than "core" but yeah why not. Probably won't need all 64k though, see below...

I can make a test that checks if fallback information even exists and registers have correct data and size, for example.

Yes that would be great. Plus it documents our expectations (or lack of) from this third party debug stub.

What else would you recommend to test for?

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:

  • Stepping
  • Breakpoints, hardware and software. Also, do the breakpoints report an address after the stop or before.

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.

lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp
126

If this is bare metal why do we have changes to Linux code here? Or is this the default platform used when connecting to the msp430 debug stub.

That said, I'm not sure we A: support Hexagon or B: it runs linux either :)

bulbazord added inline comments.Apr 5 2023, 10:04 AM
lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp
126

Hexagon is definitely supported AFAIK but I'm not sure it runs Linux. It does looks like we support an SysV ABI class for Hexagon though, so perhaps that's somewhat indicative that it runs something with that ABI...

kuilpd updated this revision to Diff 511497.Apr 6 2023, 12:05 PM

Added a test for registers, breakpoints, stepping, backtracing.

Two things come to mind:

  • Core files (though it is embedded so is that even a thing?)

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.
Plus, mspdebug doesn't actually support core files, there is no use for it now even if LLDB can generate it.

  • Mocking an msp430 remote, as we do for example in Target XML tests.

Did this, I put the same packets mspdebug sends into the mockup.

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. Plus, mspdebug doesn't actually support core files, there is no use for it now even if LLDB can generate it.

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.

lldb/test/API/functionalities/gdb_remote_client/TestMSP430MSPDebug.py
7

Please add a comment along the lines of: This test ensures that lldb correctly handles packets sent by <the msp430 debug stub>.

Ideally with a link to that stub or its source or the vendor's website, whatever's available.

If you can say how you captured the packets that would be good too. I assume just connecting and turning on packet logging but worth mentioning in case you had to do something unique.

24

Can you generate these with a helper function? Seems like the first bit changes and the rest are blanks you could add each time.

Also could you yield each time? Been a long time since I did serious Python but I think that could remove the counter.

30

I don't know that we have recent enough Pythons to use match, when was it added?

I don't have the minimum versions for lldb to hand, I would guess it is 3.7.

34

For these can you write a helper to generate the bulk of it? And ideally describe which bit is the important part. I guess some of this is the stack.

97

Could you also check that the disassembler works?

I mistakenly thought that msp430 didn't have an llvm backend but it seems that it does so it should "just work".

lldb/test/API/functionalities/gdb_remote_client/msp430.yaml
2

I forget if/how you add comments to a YAML file but however it's done, please add the C source that was used to generate the program file as a comment at the top.

kuilpd updated this revision to Diff 512891.Apr 12 2023, 10:26 AM
kuilpd marked 6 inline comments as done.

Added comments, changed memory strings formatting, added disassembler check.

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)

lldb/test/API/functionalities/gdb_remote_client/TestMSP430MSPDebug.py
104

Write some registers too? Again, not likely to find anything, just ensures we have the right offsets and don't take the wrong 16 bits out of the value.

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

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.

kuilpd updated this revision to Diff 513245.Apr 13 2023, 8:17 AM
kuilpd edited the summary of this revision. (Show Details)

Rebased and updated register info initialization.

DavidSpickett accepted this revision.Apr 17 2023, 9:05 AM

I'm sure there's something missing but LGTM.

@bulbazord ?

This revision is now accepted and ready to land.Apr 17 2023, 9:05 AM
bulbazord accepted this revision.Apr 17 2023, 10:52 AM

I'm sure there's something missing but LGTM.

@bulbazord ?

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.

This revision was automatically updated to reflect the committed changes.
asl added a comment.Apr 17 2023, 11:32 AM

@kuilpd

Please ensure that the patch is rebased into top of main and builds w/o errors / warnings.

kuilpd updated this revision to Diff 514435.Apr 17 2023, 3:13 PM

Fixed Triple.h include.