[PATCH] D56535: Support MSP430

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

Overall looks good to me. Comments are inlined.



================
Comment at: ELF/Arch/MSP430.cpp:7
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
----------------
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)


================
Comment at: ELF/Arch/MSP430.cpp:63
+    if (Val & 1)
+      error(getErrorLocation(Loc) + "invalid value for relocation " +
+            toString(Type) + ": 0x" + utohexstr(Val) + " is odd");
----------------
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.


================
Comment at: ELF/Arch/MSP430.cpp:79
+            toString(Type) + ": 0x" + utohexstr(Val) + " is odd");
+    int16_t Offset = ((int16_t)Val >> 1) - 1;
+    checkInt(Loc, Offset, 10, Type);
----------------
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.


================
Comment at: test/ELF/msp430.s:7
+; RUN: llvm-objdump -s -j .data %t3 | FileCheck -check-prefix=CHECK-DATA %s
+
+  .text
----------------
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.


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56535/new/

https://reviews.llvm.org/D56535