[PATCH] D54334: [AVR] Automatically link CRT and libgcc from the system avr-gcc

Wed Dec 19 01:42:50 PST 2018

dylanmckay marked 3 inline comments as done.
dylanmckay added a comment.

> I'm not certain if it will be possible to devise test cases for the two diagnostics I pointed out or not

I don't think it will be. The compile warning logic works by directly querying the physical filesystem using the `llvm::sys::fs` APIs <http://llvm.org/doxygen/namespacellvm_1_1sys_1_1fs.html>, which don't appear to support mocking or virtual filesystems. The only way for the pragmatically trigger the warnings is to dangerously manipulate the filesystem, potentially destroying the user's avr-gcc installation.

> I don't know enough about AVR to sign off on whether the patch logic is correct or not.

I've added some review comments to the AVR logic referencing the GCC manual and how this patch matches it. I've also added the only two LLVM reviewers I know have AVR experience so hopefully they will be able to comment on the accuracy of the AVR logic.

================
Comment at: lib/Driver/ToolChains/AVR.cpp:35
+  return llvm::StringSwitch<llvm::Optional<StringRef>>(MCU)
+      .Case("atmega328", Optional<StringRef>("avr5"))
+      .Case("atmega328p", Optional<StringRef>("avr5"))
----------------
Here is the avr-gcc list of devices and their architectures

`atmega328` and `atmega328p` are a part of the `avr5` family.

> avr5
>
>    “Enhanced” devices with 16 KiB up to 64 KiB of program memory.
>    mcu = ata5702m322, ata5782, ata5790, ata5790n, ata5791, ata5795, ata5831, ata6613c, ata6614q, ata8210, ata8510, atmega16, atmega16a, atmega16hva, atmega16hva2, atmega16hvb, atmega16hvbrevb, atmega16m1, atmega16u4, atmega161, atmega162, atmega163, atmega164a, atmega164p, atmega164pa, atmega165, atmega165a, atmega165p, atmega165pa, atmega168, atmega168a, atmega168p, atmega168pa, atmega168pb, atmega169, atmega169a, atmega169p, atmega169pa, atmega32, atmega32a, atmega32c1, atmega32hvb, atmega32hvbrevb, atmega32m1, atmega32u4, atmega32u6, atmega323, atmega324a, atmega324p, atmega324pa, atmega325, atmega325a, atmega325p, atmega325pa, atmega3250, atmega3250a, atmega3250p, atmega3250pa, atmega328, atmega328p, atmega328pb, atmega329, atmega329a, atmega329p, atmega329pa, atmega3290, atmega3290a, atmega3290p, atmega3290pa, atmega406, atmega64, atmega64a, atmega64c1, atmega64hve, atmega64hve2, atmega64m1, atmega64rfr2, atmega640, atmega644, atmega644a, atmega644p, atmega644pa, atmega644rfr2, atmega645, atmega645a, atmega645p, atmega6450, atmega6450a, atmega6450p, atmega649, atmega649a, atmega649p, atmega6490, atmega6490a, atmega6490p, at90can32, at90can64, at90pwm161, at90pwm216, at90pwm316, at90scr100, at90usb646, at90usb647, at94k, m3000.

https://gcc.gnu.org/onlinedocs/gcc/AVR-Options.html

================
Comment at: lib/Driver/ToolChains/AVR.cpp:139
+    // Add the link library specific to the MCU.
+    CmdArgs.push_back(Args.MakeArgString(std::string("-l") + CPU));
+
----------------
Here is the avr-gcc manual documenting the filename of this particular libary[1].

> Options:
>
> -nodevicelib
>
>     Don’t link against AVR-LibC’s device specific library lib<mcu>.a. 

This patch follows the same conventions as avr-gcc and thus can link the same libraries as avr-gcc.

[1] - https://gcc.gnu.org/onlinedocs/gcc/AVR-Options.html

================
Comment at: lib/Driver/ToolChains/AVR.cpp:143
+    // This is almost always required because otherwise avr-ld
+    // will assume 'avr2' and warn about the program being larger
+    // than the bare minimum supports.
----------------
This is an existing piece of avr-gcc logic also implemented by the LLVM AVR backend

> -mmcu=mcu
>
>    Specify Atmel AVR instruction set architectures (ISA) or MCU type. 
> The default for this option is ‘avr2’. 

https://gcc.gnu.org/onlinedocs/gcc/AVR-Options.html

Repository:
  rC Clang

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

https://reviews.llvm.org/D54334