[PATCH] D32991: [ELF] Initial migration of AVR target

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Did you try to run a program generated with this patch on an actual AVR (or a simulator)? Did it work?



================
Comment at: ELF/Target.cpp:251
+
+class AVRTargetInfo final : public TargetInfo {
+public:
----------------
Move this above `MipsTargetInfo`.


================
Comment at: ELF/Target.cpp:253
+public:
+  AVRTargetInfo();
+  RelExpr getRelExpr(uint32_t Type, const SymbolBody &S,
----------------
Since this constructor is the same as the default constructor, remove this line.


================
Comment at: ELF/Target.cpp:293-294
     return make<X86_64TargetInfo<ELF64LE>>();
+  case EM_AVR:
+    return make<AVRTargetInfo>();
   }
----------------
Move this after `EM_ARM` as these enums are sorted in alphabetical order.


================
Comment at: ELF/Target.cpp:2421
+
+RelExpr AVRTargetInfo::getRelExpr(uint32_t Type, const SymbolBody &S,
+                                  const uint8_t *Loc) const {
----------------
Move these new functions above Mips.


================
Comment at: ELF/Target.cpp:2424-2425
+  switch (Type) {
+  default:
+    return R_ABS;
+  case R_AVR_NONE:
----------------
I think what you want to do for now is to return `R_ABS` for `R_AVR_32`, and report an error for any other relocation type just like other `getRelExpr`s do.


================
Comment at: ELF/Target.cpp:2448
+  case R_AVR_CALL:
+    error(getErrorLocation(Loc) + "unimplemented relocation type: R_AVR_CALL");
+    break;
----------------
grimar wrote:
> We do not do like that. If some relocation is not implemented, the error below will catch it anyways, so you do not need to have logic for R_AVR_CALL.
Yes, please remove unless you actually implement `R_AVR_CALL`.


================
Comment at: test/ELF/avr-call.s:1
+# Check R_AVR_CALL relocation handling.
+
----------------
For consistency with other architectures, you want to name this file `basic-avr.s`.


Repository:
  rL LLVM

https://reviews.llvm.org/D32991