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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/Target.cpp:2426
+    return R_ABS;
+  case R_AVR_NONE:
+    return R_NONE;
----------------
Why do you need this ?


================
Comment at: ELF/Target.cpp:2448
+  case R_AVR_CALL:
+    error(getErrorLocation(Loc) + "unimplemented relocation type: R_AVR_CALL");
+    break;
----------------
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.


================
Comment at: test/ELF/avr-call.s:5
+# RUN: ld.lld %t1.o -o %t.exe
+# RUN: llvm-objdump -d %t.exe | FileCheck %s
+
----------------
So here you're dumping disassembly what looks right direction, but you do not check output anywhere.

FileCheck will search for "CHECK" label by default to perform validating,
Please review how "CHECK:" is used in other our testcases with llvm-objdump -d.


================
Comment at: test/ELF/avr-call.s:7
+
+# REQUIRES: avr
+
----------------
That is my personal opinion, and other reviewers probably have different one, but personally I prefer to put requirements for test on a first line to make clear what needed for run.


================
Comment at: test/ELF/avr-call.s:9
+
+loc:
+  nop
----------------
Does not seem loc is used anywhere ?


================
Comment at: test/ELF/avr-call.s:15
+setup:
+  call bar        # R_AVR_CALL
+  call foo        # R_AVR_CALL
----------------
So you have R_AVR_CALL relocation in code, but your implementation does not implement it and errors out. I think you want to implement it to write proper testcase.


Repository:
  rL LLVM

https://reviews.llvm.org/D32991