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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
