Page MenuHomePhabricator

Support MSP430
ClosedPublic

Authored by mskvortsov on Thu, Jan 10, 2:39 AM.

Details

Summary

This change adds a basic support for linking static MSP430 ELF code.
Implemented relocation types are intended to correspond to the BFD.

Diff Detail

Repository
rL LLVM

Event Timeline

mskvortsov created this revision.Thu, Jan 10, 2:39 AM
mskvortsov retitled this revision from Support MSP430. This change adds a basic support for linking static MSP430 ELF code. Implemented relocation types are intended to correspond to the BFD. to Support MSP430.Thu, Jan 10, 2:41 AM
mskvortsov edited the summary of this revision. (Show Details)
asl added a subscriber: asl.Thu, Jan 10, 2:50 AM

Overall looks good to me. Comments are inlined.

ELF/Arch/MSP430.cpp
7 ↗(On Diff #181014)

Please add a description of the platform. See AVR.cpp for a sample.
(Note: we do not have it for X86_64.cpp which is the most popular one I guess, but for less popular platforms I think this is a must)

63 ↗(On Diff #181014)

I would remove this.

How much is real to have this error?
I would do just the following for start:

case R_MSP430_16:
case R_MSP430_16_PCREL:
case R_MSP430_16_BYTE:
case R_MSP430_16_PCREL_BYTE:
  checkIntUInt(Loc, Val, 16, Type);
  write16le(Loc, Val);

Note that this is an msp430 that AFAIK has many documentation issues. I would try to with the simple as simple code as possible for now.

79 ↗(On Diff #181014)

The same here. I would add a check that Val fits into int16_t ideally, but for now, I think you can omit this error check and skip any checks at all (We do not check other relocations we have for oddness I think anyway it seems). So I would simplify things as much as possible for initial msp400 support.

test/ELF/msp430.s
7 ↗(On Diff #181014)

Please explain the purpose of the test here (just add a short description of what it do like "This tests the simple msp400 relocations like X, Y" etc).

Also seems you can go with a single llvm-objdump invocation, please do in that way.

grimar added inline comments.Thu, Jan 10, 3:20 AM
ELF/Arch/MSP430.cpp
63 ↗(On Diff #181014)

:] *I would try to go with the simple code as possible for now.

Feedback addressed.

grimar accepted this revision.Thu, Jan 10, 4:34 AM

LGTM.

ELF/Arch/MSP430.cpp
18 ↗(On Diff #181028)

I would remove this sentence. It too easy to forget to update it.

This revision is now accepted and ready to land.Thu, Jan 10, 4:34 AM

@grimar Thank you for the review. Could you please commit this change?

@grimar Thank you for the review. Could you please commit this change?

Seems something went wrong. It does not compile for me. Could you re-upload?

161>D:\Work\llvm\tools\lld\ELF\Arch\MSP430.cpp(81): error C2065: 'Offset': undeclared identifier
161>D:\Work\llvm\tools\lld\ELF\Arch\MSP430.cpp(82): error C2065: 'Offset': undeclared identifier
161>D:\Work\llvm\tools\lld\ELF\Arch\MSP430.cpp(82): error C2660: 'llvm::support::endian::write16le': function does not take 1 argument

Oops, I missed a line, sorry for that. Re-uploaded.

Oops, I missed a line, sorry for that. Re-uploaded.

r350819. Thanks!

Closed by commit rL350819: [LLD][ELF] - Support MSP430. (authored by grimar, committed by ). · Explain WhyThu, Jan 10, 5:48 AM
This revision was automatically updated to reflect the committed changes.

Oops, I missed a line, sorry for that. Re-uploaded.

r350819. Thanks!

It seems like the test/ELF/msp430.s file got lost while committing.

Oops, I missed a line, sorry for that. Re-uploaded.

r350819. Thanks!

It seems like the test/ELF/msp430.s file got lost while committing.

Not sure why that happened but committed in r350833, thanks!

ruiu added a subscriber: ruiu.Thu, Jan 10, 9:12 AM

Could you please wait for a while for me to review? Adding a new target is a big deal, and it should be done more carefully. Sending a patch and submitting in a few hours is unacceptable. George, when you give LGTM please note about that too.

ruiu added a comment.Thu, Jan 10, 9:18 AM

This patch doesn't have any test -- the test file doesn't contain any RUN: line, so nothing is tested so far.

lld/trunk/ELF/Arch/MSP430.cpp
45

TrapInstr must be 4 bytes long. Otherwise it would create a garbage on every two bytes.

lld/trunk/ELF/CMakeLists.txt
18

Sort asciibetically. "MSP" should precede "Mips" in the dictionary order.

ruiu added a comment.Thu, Jan 10, 9:29 AM

After reading this patch, I decided to *not* roll back this patch as the patch seems acceptable with a few changes (I'm going to make the changes), but please keep in mind that that handling is exceptional; when someone added a new target in such a way like this, we'd normally have to revert it no matter what the patch is, as it is a violation of the usual review process. And I believe you didn't really have to rush to submit; this is a good patch, so just relax and wait for a little for others to review it as well.

lld/trunk/ELF/Arch/MSP430.cpp
45

Does this instruction actually triggers a CPU interrupt?

ruiu added a comment.Thu, Jan 10, 10:16 AM

How mature is this implementation of MSP support? I'd like to add a note to the release notes for the next major release.

And I believe you didn't really have to rush to submit; this is a good patch, so just relax and wait for a little for others to review it as well.

Agree. Sorry for rushing.

After reading this patch, I decided to *not* roll back this patch as the patch seems acceptable with a few changes (I'm going to make the changes), but please keep in mind that that handling is exceptional; when someone added a new target in such a way like this, we'd normally have to revert it no matter what the patch is, as it is a violation of the usual review process. And I believe you didn't really have to rush to submit; this is a good patch, so just relax and wait for a little for others to review it as well.

Yeah, sorry, probably I just shouldn't have asked to submit that soon.

lld/trunk/ELF/Arch/MSP430.cpp
45

It does trigger an interrupt on a simulator and presumably it does not on a real hardware.

How mature is this implementation of MSP support? I'd like to add a note to the release notes for the next major release.

I think the implementation could be characterized as "initial".
We have several tests compiled by LLVM and linked by LLD running on the gdb-sim simulator, including some tests shipped with newlib.

How mature is this implementation of MSP support? I'd like to add a note to the release notes for the next major release.

It is not mature at all and it is initial support for hello world AFAIK. So I would not add more than "initial implementation of MSP430 target" line. Michael might know much more than me here.