Support MSP430

[mskvortsov]

Summary

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

[grimar]

Overall looks good to me. Comments are inlined.

ELF/Arch/MSP430.cpp

7

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

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

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

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]

ELF/Arch/MSP430.cpp

63

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

[mskvortsov]

Feedback addressed.

[grimar]

LGTM.

ELF/Arch/MSP430.cpp

19

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

[mskvortsov]

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

[grimar]

In D56535#1352593, @mskvortsov wrote:

@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

[mskvortsov]

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

[grimar]

In D56535#1352620, @mskvortsov wrote:

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

r350819. Thanks!

[mskvortsov]

In D56535#1352643, @grimar wrote:

In D56535#1352620, @mskvortsov wrote:

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.

[grimar]

In D56535#1352780, @mskvortsov wrote:

In D56535#1352643, @grimar wrote:

In D56535#1352620, @mskvortsov wrote:

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]

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]

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 ↗

(On Diff #181038)

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

lld/trunk/ELF/CMakeLists.txt

18 ↗

(On Diff #181038)

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

[ruiu]

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 ↗

(On Diff #181038)

Does this instruction actually triggers a CPU interrupt?

[ruiu]

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

[grimar]

In D56535#1352963, @ruiu wrote:

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.

[mskvortsov]

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 ↗

(On Diff #181038)

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

[mskvortsov]

In D56535#1353028, @ruiu wrote:

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.

[grimar]

In D56535#1353028, @ruiu wrote:

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.

ELF/Arch/MSP430.cpp

+//===- MSP430.cpp ---------------------------------------------------------===//
+//
+//                             The LLVM Linker
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "InputFiles.h"
+#include "Symbols.h"
+#include "Target.h"
+#include "lld/Common/ErrorHandler.h"
+#include "llvm/Object/ELF.h"
+#include "llvm/Support/Endian.h"
+
+using namespace llvm;
+using namespace llvm::object;
+using namespace llvm::support::endian;
+using namespace llvm::ELF;
+using namespace lld;
+using namespace lld::elf;
+
+namespace {
+class MSP430 final : public TargetInfo {
+public:
+  MSP430();
+  RelExpr getRelExpr(RelType Type, const Symbol &S,
+                     const uint8_t *Loc) const override;
+  void relocateOne(uint8_t *Loc, RelType Type, uint64_t Val) const override;
+};
+} // namespace
+
+MSP430::MSP430() {
+  // mov.b #0, r3
+  TrapInstr = {0x43, 0x43};
+}
+
+RelExpr MSP430::getRelExpr(RelType Type, const Symbol &S,
+                           const uint8_t *Loc) const {
+  switch (Type) {
+  case R_MSP430_10_PCREL:
+  case R_MSP430_16_PCREL:
+  case R_MSP430_16_PCREL_BYTE:
+  case R_MSP430_2X_PCREL:
+  case R_MSP430_RL_PCREL:
+  case R_MSP430_SYM_DIFF:
+    return R_PC;
+  default:
+    return R_ABS;
+  }
+}
+
+void MSP430::relocateOne(uint8_t *Loc, RelType Type, uint64_t Val) const {
+  switch (Type) {
+  case R_MSP430_8:
+    checkIntUInt(Loc, Val, 8, Type);
+    *Loc = Val;
+    break;
+  case R_MSP430_16:
+  case R_MSP430_16_PCREL:
+    if (Val & 1)
+      error(getErrorLocation(Loc) + "invalid value for relocation " +
+            toString(Type) + ": 0x" + utohexstr(Val) + " is odd");
+    LLVM_FALLTHROUGH;
+  case R_MSP430_16_BYTE:
+  case R_MSP430_16_PCREL_BYTE:
+    checkIntUInt(Loc, Val, 16, Type);
+    write16le(Loc, Val);
+    break;
+  case R_MSP430_32:
+    checkIntUInt(Loc, Val, 32, Type);
+    write32le(Loc, Val);
+    break;
+  case R_MSP430_10_PCREL: {
+    if (Val & 1)
+      error(getErrorLocation(Loc) + "invalid value for relocation " +
+            toString(Type) + ": 0x" + utohexstr(Val) + " is odd");
+    int16_t Offset = ((int16_t)Val >> 1) - 1;
+    checkInt(Loc, Offset, 10, Type);
+    write16le(Loc, (read16le(Loc) & 0xFC00) | (Offset & 0x3FF));
+    break;
+  }
+  default:
+    error(getErrorLocation(Loc) + "unrecognized reloc " + toString(Type));
+  }
+}
+
+TargetInfo *elf::getMSP430TargetInfo() {
+  static MSP430 Target;
+  return &Target;
+}

ELF/CMakeLists.txt

add_lld_library(lldELF
   AArch64ErrataFix.cpp
   Arch/AArch64.cpp
   Arch/AMDGPU.cpp
   Arch/ARM.cpp
   Arch/AVR.cpp
   Arch/Hexagon.cpp
   Arch/Mips.cpp
   Arch/MipsArchTree.cpp
+  Arch/MSP430.cpp
   Arch/PPC.cpp
   Arch/PPC64.cpp
   Arch/RISCV.cpp
   Arch/SPARCV9.cpp
   Arch/X86.cpp
   Arch/X86_64.cpp
   CallGraphSort.cpp
   DWARF.cpp

ELF/InputFiles.cpp

case Triple::thumb:
     return EM_ARM;
   case Triple::avr:
     return EM_AVR;
   case Triple::mips:
   case Triple::mipsel:
   case Triple::mips64:
   case Triple::mips64el:
     return EM_MIPS;
+  case Triple::msp430:
+    return EM_MSP430;
   case Triple::ppc:
     return EM_PPC;
   case Triple::ppc64:
   case Triple::ppc64le:
     return EM_PPC64;
   case Triple::x86:
     return T.isOSIAMCU() ? EM_IAMCU : EM_386;
   case Triple::x86_64:

ELF/Target.h

protected:
   uint64_t DefaultImageBase = 0x10000;
 };
 
 TargetInfo *getAArch64TargetInfo();
 TargetInfo *getAMDGPUTargetInfo();
 TargetInfo *getARMTargetInfo();
 TargetInfo *getAVRTargetInfo();
 TargetInfo *getHexagonTargetInfo();
+TargetInfo *getMSP430TargetInfo();
 TargetInfo *getPPC64TargetInfo();
 TargetInfo *getPPCTargetInfo();
 TargetInfo *getRISCVTargetInfo();
 TargetInfo *getSPARCV9TargetInfo();
 TargetInfo *getX32TargetInfo();
 TargetInfo *getX86TargetInfo();
 TargetInfo *getX86_64TargetInfo();
 template <class ELFT> TargetInfo *getMipsTargetInfo();

ELF/Target.cpp

case ELF32BEKind:
       return getMipsTargetInfo<ELF32BE>();
     case ELF64LEKind:
       return getMipsTargetInfo<ELF64LE>();
     case ELF64BEKind:
       return getMipsTargetInfo<ELF64BE>();
     default:
       llvm_unreachable("unsupported MIPS target");
     }
+  case EM_MSP430:
+    return getMSP430TargetInfo();
   case EM_PPC:
     return getPPCTargetInfo();
   case EM_PPC64:
     return getPPC64TargetInfo();
   case EM_RISCV:
     return getRISCVTargetInfo();
   case EM_SPARCV9:
     return getSPARCV9TargetInfo();

test/ELF/Inputs/msp430.s

+  .text
+  .global _start
+_start:
+  nop

test/ELF/msp430.s

+; REQUIRES: msp430
+; RUN: llvm-mc -filetype=obj -triple=msp430-elf %s -o %t
+; RUN: llvm-mc -filetype=obj -triple=msp430-elf %S/Inputs/msp430.s -o %t2
+; RUN: ld.lld --Tdata=0x2000 --Ttext=0x8000 --defsym=_byte=0x21 %t2 %t -o %t3
+; RUN: llvm-objdump -d %t3 | FileCheck %s
+; RUN: llvm-objdump -s -j .data %t3 | FileCheck -check-prefix=CHECK-DATA %s
+
+  .text
+  .global foo
+foo:
+; R_MSP430_10_PCREL
+  jmp _start
+
+; CHECK:      Disassembly of section .text:
+; CHECK-NEXT: _start:
+; CHECK-NEXT: 8000: {{.*}} nop
+; CHECK:      foo:
+; CHECK-NEXT: 8004: {{.*}} jmp $-4
+
+; R_MSP430_16_BYTE
+  call #_start
+
+; CHECK:      call #32768
+
+; R_MSP430_16_PCREL_BYTE
+  mov #-1, _start
+
+; CHECK:      800a: {{.*}} mov #-1, -12
+
+  .data
+; R_MSP430_8
+  .byte _byte
+; R_MSP430_16
+  .word _start
+; R_MSP430_32
+  .long _start
+
+; CHECK-DATA: 2000 21008000 800000

test/lit.cfg.py

llvm_config.feature_config(
     [('--build-mode', {'DEBUG': 'debug'}),
      ('--assertion-mode', {'ON': 'asserts'}),
      ('--targets-built', {'AArch64': 'aarch64',
                           'AMDGPU': 'amdgpu',
                           'ARM': 'arm',
                           'AVR': 'avr',
                           'Hexagon': 'hexagon',
                           'Mips': 'mips',
+                          'MSP430': 'msp430',
                           'PowerPC': 'ppc',
                           'RISCV': 'riscv',
                           'Sparc': 'sparc',
                           'WebAssembly': 'wasm',
                           'X86': 'x86'})
      ])
 
 # Set a fake constant version so that we get consistent output.