[Driver][AVR] Emit proper warnings for different options

[benshi001]

1. If '-S'/'-c' is specified, do not generate link warnings;
2. If '-S'/'-c' is not specified, '-mmcu' is specified and there is valid GCC installation, do not generate link warnings;
3. If '-S'/'-c' is not specified, and '-mmcu' is not specified, genereate link warnings;
4. If '-S'/'-c' is not specified, and there is no valid GCC installation, genereate link warnings.

[benshi001]

1. If '-S'/'-c' is specified, do not generate link warnings;
   • User should be clear that he does not want link, so there is no link warning.

2. If '-S'/'-c' is not specified, '-mmcu' is specified and there is valid GCC installation, do not generate link warnings;
   • This is the normal case, link should be succesful.

3. If '-S'/'-c' is not specified, and '-mmcu' is not specified, genereate link warnings;
   • Since no MCU is specified, we should warn the user that no device library is linked.

4. If '-S'/'-c' is not specified, and there is no valid avr-GCC installation, genereate link warnings.
   • Since there is no avr-ld installed, we should warn the user that there is no link happens. That may changes if we choose to use lld in the future.

[MaskRay]

clang/test/Driver/avr-toolchain.c

39

-target is legacy spelling. Better use --target= for new RUN lines.

40

negative patterns easily go stale without being noticed.
Check whether you can just use CHECK5-NOT: warning: instead of specifying the full diagnostic.

[MaskRay]

clang/lib/Driver/ToolChains/AVR.cpp

375

This is insufficient. -fsyntax-only, -E, etc should not need the diagnostic as well.
I know there may be test coverage gap but it is excessive to test all of -S -c -E ... that there is no diagnostic.

[MaskRay]

I think it is excessive to add so many RUN lines. I do not understand much about AVR -mcpu. That said, I created D122553 for what I think should be done for the -c/-S/-fsyntax-only condition. More tests would just be excessive.

You may adjust the patch to do the rest cleanups/fixes.

[benshi001]

In D122524#3410542, @MaskRay wrote:

I think it is excessive to add so many RUN lines. I do not understand much about AVR -mcpu. That said, I created D122553 for what I think should be done for the -c/-S/-fsyntax-only condition. More tests would just be excessive.

You may adjust the patch to do the rest cleanups/fixes.

I have made some changes based on your https://reviews.llvm.org/D122553, it seems you have reverted. I suggest you recommit, since I have fixed the failures of lacking avr-gcc.

[MaskRay]

In D122524#3411872, @benshi001 wrote:

In D122524#3410542, @MaskRay wrote:

I think it is excessive to add so many RUN lines. I do not understand much about AVR -mcpu. That said, I created D122553 for what I think should be done for the -c/-S/-fsyntax-only condition. More tests would just be excessive.

You may adjust the patch to do the rest cleanups/fixes.

I have made some changes based on your https://reviews.llvm.org/D122553, it seems you have reverted. I suggest you recommit, since I have fixed the failures of lacking avr-gcc.

I did not specify --sysroot so the new tests failed on systems without avr-gcc. Relanded.

[benshi001]

In D122524#3411879, @MaskRay wrote:

In D122524#3411872, @benshi001 wrote:

In D122524#3410542, @MaskRay wrote:

I think it is excessive to add so many RUN lines. I do not understand much about AVR -mcpu. That said, I created D122553 for what I think should be done for the -c/-S/-fsyntax-only condition. More tests would just be excessive.

You may adjust the patch to do the rest cleanups/fixes.

I have made some changes based on your https://reviews.llvm.org/D122553, it seems you have reverted. I suggest you recommit, since I have fixed the failures of lacking avr-gcc.

I did not specify --sysroot so the new tests failed on systems without avr-gcc. Relanded.

I have made some changes based on your code:

1. We should always warn if there is no mmcu specified, since its info (instruction set version) is used by the llvm backend.
2. We need no warn no avr-gcc&avr-libc if -S/-c is specified, since the compiling stage need neither avr-gcc nor avr-libc.

Thanks for your way to avoid checking so many -c/-S/-fsyntax-only/... conditions.

[MaskRay]

clang/lib/Driver/ToolChains/AVR.cpp

476–477

The two if can be combined.

[benshi001]

clang/lib/Driver/ToolChains/AVR.cpp

476–477

I will do it when committing.

[benshi001]

clang/lib/Driver/ToolChains/AVR.cpp

476–477

We should not merge the above two if, due to this check.

So I will keep my orginal form when committing.

[MaskRay]

clang/lib/Driver/ToolChains/AVR.cpp

476–477

OK. If we decide that empty -mpu will report a warning, reporting an additional warn_drv_avr_stdlib_not_linked may or may not be necessary, but I'm happy to defer that decision to you.

clang/lib/Driver/ToolChains/AVR.cpp

 AVRToolChain::AVRToolChain(const Driver &D, const llvm::Triple &Triple,
                            const ArgList &Args)
     : Generic_ELF(D, Triple, Args), LinkStdlib(false) {
   GCCInstallation.init(Triple, Args);
 
   // Only add default libraries if the user hasn't explicitly opted out.
   if (!Args.hasArg(options::OPT_nostdlib) &&
       !Args.hasArg(options::OPT_nodefaultlibs) &&
+      !Args.hasArg(options::OPT_S) &&
       !Args.hasArg(options::OPT_c /* does not apply when not linking */)) {
     std::string CPU = getCPUName(D, Args, Triple);
 
     if (CPU.empty()) {
       // We cannot link any standard libraries without an MCU specified.
       D.Diag(diag::warn_drv_avr_mcu_not_specified);
     } else {
       Optional<StringRef> FamilyName = GetMCUFamilyName(CPU);
void AVR::Linker::ConstructJob(Compilation &C, const JobAction &JA,
   CmdArgs.push_back(Output.getFilename());
 
   // Enable garbage collection of unused sections.
   CmdArgs.push_back("--gc-sections");
 
   // Add library search paths before we specify libraries.
   Args.AddAllArgs(CmdArgs, options::OPT_L);
   getToolChain().AddFilePathLibArgs(Args, CmdArgs);
 
   if (SectionAddressData.hasValue()) {
     std::string DataSectionArg = std::string("-Tdata=0x") +
                                  llvm::utohexstr(SectionAddressData.getValue());
     CmdArgs.push_back(Args.MakeArgString(DataSectionArg));
   } else {
     // We do not have an entry for this CPU in the address mapping table yet.
     D.Diag(diag::warn_drv_avr_linker_section_addresses_not_implemented) << CPU;
   }
 

clang/test/Driver/avr-toolchain.c

 // RUN: %clang %s -### -target avr -fuse-init-array -fuse-cxa-atexit 2>&1 | FileCheck -check-prefix=CHECK4 %s
 // CHECK4: clang{{.*}} "-cc1" "-triple" "avr"
 // CHECK4-NOT: "-fno-use-init-array"
 // CHECK4-NOT: "-fno-use-cxa-atexit"
 
 // RUN: %clang %s -### -target avr --sysroot %S/Inputs/basic_avr_tree 2>&1 -nostdinc | FileCheck --check-prefix=NOSTDINC %s
 // RUN: %clang %s -### -target avr --sysroot %S/Inputs/basic_avr_tree 2>&1 -nostdlibinc | FileCheck --check-prefix=NOSTDINC %s
 // NOSTDINC-NOT: "-internal-isystem" {{".*avr/include"}}
+
+// RUN: %clang %s -### -target avr --sysroot %S/Inputs/basic_avr_tree 2>&1 | FileCheck --check-prefix=LINK %s
+// LINK: warning: standard library not linked and so no interrupt vector table or compiler runtime routines will be linked
+
+// RUN: %clang %s -### -target avr --sysroot %S/Inputs/basic_avr_tree -S 2>&1 | FileCheck --check-prefix=NOLINK %s
+// RUN: %clang %s -### -target avr --sysroot %S/Inputs/basic_avr_tree -c 2>&1 | FileCheck --check-prefix=NOLINK %s
+// NOLINK-NOT: warning: standard library not linked and so no interrupt vector table or compiler runtime routines will be linked