[PATCH] D56535: Support MSP430
George Rimar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jan 10 03:18:30 PST 2019
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
More information about the llvm-commits
mailing list