[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

378

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

451

The two if can be combined.

[benshi001]

clang/lib/Driver/ToolChains/AVR.cpp

451

I will do it when committing.

[benshi001]

clang/lib/Driver/ToolChains/AVR.cpp

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

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

 } // end anonymous namespace
 
 /// AVR Toolchain
 AVRToolChain::AVRToolChain(const Driver &D, const llvm::Triple &Triple,
                            const ArgList &Args)
     : Generic_ELF(D, Triple, Args) {
   GCCInstallation.init(Triple, Args);
 
+  std::string CPU = getCPUName(D, Args, Triple);
+  if (CPU.empty())
+    D.Diag(diag::warn_drv_avr_mcu_not_specified);
+
   // Only add default libraries if the user hasn't explicitly opted out.
   if (!Args.hasArg(options::OPT_nostdlib) &&
-      !Args.hasArg(options::OPT_nodefaultlibs)) {
-    if (GCCInstallation.isValid()) {
-      GCCInstallPath = GCCInstallation.getInstallPath();
-      std::string GCCParentPath(GCCInstallation.getParentLibPath());
-      getProgramPaths().push_back(GCCParentPath + "/../bin");
-    } else {
-      D.Diag(diag::warn_drv_avr_gcc_not_found);
-    }
+      !Args.hasArg(options::OPT_nodefaultlibs) &&
+      GCCInstallation.isValid()) {
   }
 }
 
 void AVRToolChain::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
                                              ArgStringList &CC1Args) const {
   if (DriverArgs.hasArg(options::OPT_nostdinc) ||
       DriverArgs.hasArg(options::OPT_nostdlibinc))
     return;
void AVR::Linker::ConstructJob(Compilation &C, const JobAction &JA,
 
   // Add library search paths before we specify libraries.
   Args.AddAllArgs(CmdArgs, options::OPT_L);
   getToolChain().AddFilePathLibArgs(Args, CmdArgs);
 
   // Only add default libraries if the user hasn't explicitly opted out.
   bool LinkStdlib = false;
   if (!Args.hasArg(options::OPT_nostdlib) &&
       !Args.hasArg(options::OPT_nodefaultlibs)) {
-    if (CPU.empty()) {
-      // We cannot link any standard libraries without an MCU specified.
-      D.Diag(diag::warn_drv_avr_mcu_not_specified);
-    } else {
+    if (!CPU.empty()) {
       Optional<StringRef> FamilyName = GetMCUFamilyName(CPU);
       Optional<std::string> AVRLibcRoot = TC.findAVRLibcInstallation();
 
       if (!FamilyName) {
         // We do not have an entry for this CPU in the family
         // mapping table yet.
         D.Diag(diag::warn_drv_avr_family_linking_stdlibs_not_implemented)
             << CPU;
+      } else if (TC.getGCCInstallPath().empty()) {
+        // We can not link since there is no avr-ld.
+        D.Diag(diag::warn_drv_avr_gcc_not_found);
       } else if (!AVRLibcRoot) {
         // No avr-libc found and so no runtime linked.
         D.Diag(diag::warn_drv_avr_libc_not_found);
-      } else if (!TC.getGCCInstallPath().empty()) {
+      } else {
         std::string SubPath = GetMCUSubPath(CPU);
         CmdArgs.push_back(
             Args.MakeArgString(Twine("-L") + *AVRLibcRoot + "/lib/" + SubPath));
         CmdArgs.push_back(
             Args.MakeArgString("-L" + TC.getGCCInstallPath() + "/" + SubPath));
         LinkStdlib = true;
       }
     }
 
     if (!LinkStdlib)
       D.Diag(diag::warn_drv_avr_stdlib_not_linked);
   }
 
   if (SectionAddressData.hasValue()) {
     std::string DataSectionArg = std::string("-Tdata=0x") +
                                  llvm::utohexstr(SectionAddressData.getValue());
     CmdArgs.push_back(Args.MakeArgString(DataSectionArg));
   } else {

clang/test/Driver/avr-toolchain.c

 // 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 -### --target=avr --sysroot=%S/Inputs/basic_avr_tree %s 2>&1 | FileCheck --check-prefix=WARN_STDLIB %s
-// RUN: %clang -### --target=avr --sysroot=%S/Inputs/basic_avr_tree -mmcu=atmega328 %s 2>&1 | FileCheck --check-prefix=NOWARN_STDLIB %s
-// RUN: %clang -### --target=avr --sysroot=%S/Inputs/basic_avr_tree -c %s 2>&1 | FileCheck --check-prefix=NOWARN_STDLIB %s
-
-// WARN_STDLIB: warning: no target microcontroller specified on command line, cannot link standard libraries, please pass -mmcu=<mcu name>
-// WARN_STDLIB: warning: standard library not linked and so no interrupt vector table or compiler runtime routines will be linked
-// NOWARN_STDLIB-NOT: warning:
+// RUN: %clang -### --target=avr --sysroot=%S/Inputs/basic_avr_tree -mmcu=atmega328 %s 2>&1 | FileCheck --check-prefix=NOWARN %s
+// RUN: %clang -### --target=avr --sysroot=%S/Inputs/basic_avr_tree -mmcu=atmega328 -S %s 2>&1 | FileCheck --check-prefix=NOWARN %s
+// RUN: %clang -### --target=avr --sysroot=%S/Inputs/ -mmcu=atmega328 -S %s 2>&1 | FileCheck --check-prefix=NOWARN %s
+// NOWARN-NOT: warning:
+
+// RUN: %clang -### --target=avr --sysroot=%S/Inputs/basic_avr_tree -S %s 2>&1 | FileCheck --check-prefixes=NOMCU,LINKA %s
+// RUN: %clang -### --target=avr --sysroot=%S/Inputs/ -S %s 2>&1 | FileCheck --check-prefixes=NOMCU,LINKA %s
+// RUN: %clang -### --target=avr --sysroot=%S/Inputs/basic_avr_tree %s 2>&1 | FileCheck --check-prefixes=NOMCU,LINKB %s
+// NOMCU: warning: no target microcontroller specified on command line, cannot link standard libraries, please pass -mmcu=<mcu name>
+// LINKB: warning: standard library not linked and so no interrupt vector table or compiler runtime routines will be linked
+// LINKB: warning: support for passing the data section address to the linker for microcontroller '' is not implemented
+// NOMCU-NOT: warning: {{.*}} avr-gcc
+// NOMCU-NOT: warning: {{.*}} avr-libc
+// LINKA-NOT: warning: {{.*}} interrupt vector
+// LINKA-NOT: warning: {{.*}} data section address
+
+// RUN: %clang -### --target=avr --sysroot=%S/Inputs/ -mmcu=atmega328 %s 2>&1 | FileCheck --check-prefixes=NOGCC %s
+// NOGCC: warning: no avr-gcc installation can be found on the system, cannot link standard libraries
+// NOGCC: warning: standard library not linked and so no interrupt vector table or compiler runtime routines will be linked
+// NOGCC-NOT: warning: {{.*}} microcontroller
+// NOGCC-NOT: warning: {{.*}} avr-libc
+// NOGCC-NOT: warning: {{.*}} data section address