[AVR][clang] Reject non assembly source files for the avr1 family

[benshi001]

Summary

Devices belongs to the avr1 family do not has SRAM, so
only assembly source files should be accepted. This behaviour
conforms to GCC.

[benshi001]

This is GCC's handling on AVR

https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/config/avr/avr.c;h=62973927fdc30d502e5d225f83cdde958bf2dad0;hb=refs/heads/master#l10424

around line 10433 / function static void avr_file_start (void),

gcc rises an error architecture %qs supported for assembler only

[benshi001]

clang/lib/Driver/ToolChains/AVR.cpp

425

We need not care about '-x c++' or '-x c', which are excluded by above checks.

[benshi001]

clang/lib/Driver/ToolChains/AVR.cpp

423

In this for loop, the key point is which one is found first.

It is OK that -x assembler-with-cpp comes first, otherwise an error should be reported.

[aykevl]

I'm not sure this is the correct location for these checks. You're essentially checking whether the compilation looks like a C/C++ compilation or an assembly compilation based on the flags and the file name. However, the Clang driver already does something like this: it converts the command line arguments and files into a list of jobs to perform. This is done in Driver::BuildActions, Driver::BuildJobs, Clang::ConstructJob, and other places.
I think a better place to do this check is in Clang::ConstructJob. There is already something similar here:

https://github.com/llvm/llvm-project/blob/10ed1eca241f893085b8db40138e588e72aaee3a/clang/lib/Driver/ToolChains/Clang.cpp#L4396-L4398

// C++ is not supported for IAMCU.
if (IsIAMCU && types::isCXX(Input.getType()))
  D.Diag(diag::err_drv_clang_unsupported) << "C++ for IAMCU";

I think something like this will work, in Clang::ConstructJob:

bool IsAVR = ...
...

if (IsAVR) {
    D.Diag(diag::err_drv_clang_unsupported) << "C/C++ for AVR";

There is a different ConstructJob for assembly, so this works.

clang/lib/Driver/ToolChains/AVR.h

22

I think I would have called this CPU not AVRMcu, but AVRMcu is fine.

clang/test/Driver/avr-mmcu.S

4 ↗

(On Diff #400375)

Is there a reason why you used {{.*}} instead of the actual value?

[benshi001]

Thanks for your suggestion ! I have changed to check avr-1 in Clang::ConstructJob, which is much more clear than my previous solution !

Really appreicate. A bit pity is that I have to make big changes to the constructor AVRToolChain::AVRToolChain, otherwise the expected error *** only supports assembly programming can not rise if -c is specified in clang options. Because in previous code, the reading of FamilyName is embraced by !Args.hasArg(options::OPT_c), something like

if (!Args.hasArg(options::OPT_c)) {
    Optional<StringRef> FamilyName = GetMCUFamilyName(CPU);
}

so I have to re-arrange the code logic.

! In D117423#3251110, @aykevl wrote:

I'm not sure this is the correct location for these checks. You're essentially checking whether the compilation looks like a C/C++ compilation or an assembly compilation based on the flags and the file name. However, the Clang driver already does something like this: it converts the command line arguments and files into a list of jobs to perform. This is done in Driver::BuildActions, Driver::BuildJobs, Clang::ConstructJob, and other places.
I think a better place to do this check is in Clang::ConstructJob. There is already something similar here:

https://github.com/llvm/llvm-project/blob/10ed1eca241f893085b8db40138e588e72aaee3a/clang/lib/Driver/ToolChains/Clang.cpp#L4396-L4398

// C++ is not supported for IAMCU.
if (IsIAMCU && types::isCXX(Input.getType()))
  D.Diag(diag::err_drv_clang_unsupported) << "C++ for IAMCU";

I think something like this will work, in Clang::ConstructJob:

bool IsAVR = ...
...

if (IsAVR) {
    D.Diag(diag::err_drv_clang_unsupported) << "C/C++ for AVR";

There is a different ConstructJob for assembly, so this works.

[benshi001]

The previous logic of the constructor AVRToolChain::AVRToolChain

1. if -c or -nostdlib is specified, no error will rise (even if no MCU is specified).

2. if no MCU is specified, two warnings will rise

warning: no target microcontroller specified on command line
warning: standard library not linked and so no interrupt vector table

3. if the specified avr device has no proper avr family name, two warnings will rise

warning: support for linking stdlibs for microcontroller '%0' is not implemented
warning: standard library not linked and so no interrupt vector table

4. if no available avr-gcc is found, two warnings will rise

warning: no avr-gcc installation can be found on the system,
warning: standard library not linked and so no interrupt vector table

5. if no available avr-libc is found, two warnings will rise

warning: no avr-libc installation can be found on the system
warning: standard library not linked and so no interrupt vector table

And after my re-arrangement of the constructor, the above points 2/3/4/5 do not change. But only point 1 changes to
if -c or -nostdlib is specified and a valid avr mcu is specified, no error will rise.
if -c or -nostdlib is specified but no valid avr mcu is specified, a warning will rise (no target microcontroller specified on command line)

[benshi001]

clang/lib/Driver/ToolChains/AVR.h

22

It is changed to FamilyName, since family is checked other than the device name.

[MaskRay]

clang/lib/Driver/ToolChains/Clang.cpp

5149

Try moving the check from the generic file to the toolchain-specific file.

[benshi001]

clang/lib/Driver/ToolChains/Clang.cpp

5149

I have created another patch to do this check in avr toolchain specific files.

https://reviews.llvm.org/D118095

which is much more complex than current solution.

But I do see there are other target specific checks in the common Clang::ConstructJob, such as

https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L5153

https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L5019

https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L5257

[benshi001]

Re-implement in https://reviews.llvm.org/D118095

clang/include/clang/Basic/DiagnosticDriverKinds.td

 def warn_drv_avr_linker_section_addresses_not_implemented: Warning<
   "support for passing the data section address to the linker for "
   "microcontroller '%0' is not implemented">,
   InGroup<AVRRtlibLinkingQuirks>;
 def warn_drv_avr_stdlib_not_linked: Warning<
   "standard library not linked and so no interrupt vector table or "
   "compiler runtime routines will be linked">,
   InGroup<AVRRtlibLinkingQuirks>;
+def err_drv_avr_assembly_only : Error<
+  "device '%0' only supports assembly programming">;
 def err_drv_cuda_bad_gpu_arch : Error<"unsupported CUDA gpu architecture: %0">;
 def err_drv_no_cuda_installation : Error<
   "cannot find CUDA installation; provide its path via '--cuda-path', or pass "
   "'-nocudainc' to build without CUDA includes">;
 def err_drv_no_cuda_libdevice : Error<
   "cannot find libdevice for %0; provide path to different CUDA installation "
   "via '--cuda-path', or pass '-nocudalib' to build without linking with "
   "libdevice">;

clang/lib/Driver/ToolChains/AVR.h

 #include "clang/Driver/Tool.h"
 #include "clang/Driver/ToolChain.h"
 
 namespace clang {
 namespace driver {
 namespace toolchains {
 
 class LLVM_LIBRARY_VISIBILITY AVRToolChain : public Generic_ELF {
+  StringRef FamilyName;
+
 public:
   AVRToolChain(const Driver &D, const llvm::Triple &Triple,
                const llvm::opt::ArgList &Args);
   void
   AddClangSystemIncludeArgs(const llvm::opt::ArgList &DriverArgs,
                             llvm::opt::ArgStringList &CC1Args) const override;
 
   void
   addClangTargetOptions(const llvm::opt::ArgList &DriverArgs,
                         llvm::opt::ArgStringList &CC1Args,
                         Action::OffloadKind DeviceOffloadKind) const override;
 
+  StringRef getFamilyName() const { return FamilyName; }
+
 protected:
   Tool *buildLinker() const override;
 
 private:
   /// Whether libgcc, libct, and friends should be linked.
   ///
   /// This is not done if the user does not specify a
   /// microcontroller on the command line.

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), 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_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);
-      Optional<std::string> AVRLibcRoot = findAVRLibcInstallation();
+    D.Diag(diag::warn_drv_avr_stdlib_not_linked);
+    return;
+  }
 
-      if (!FamilyName.hasValue()) {
-        // 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 (!GCCInstallation.isValid()) {
-        // No avr-gcc found and so no runtime linked.
-        D.Diag(diag::warn_drv_avr_gcc_not_found);
-      } else if (!AVRLibcRoot.hasValue()) {
-        // No avr-libc found and so no runtime linked.
-        D.Diag(diag::warn_drv_avr_libc_not_found);
-      } else { // We have enough information to link stdlibs
-        std::string GCCRoot(GCCInstallation.getInstallPath());
-        std::string GCCParentPath(GCCInstallation.getParentLibPath());
-        std::string LibcRoot = AVRLibcRoot.getValue();
-        std::string SubPath = GetMCUSubPath(CPU);
+  Optional<StringRef> FamilyName = GetMCUFamilyName(CPU);
+    // 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;
+    D.Diag(diag::warn_drv_avr_stdlib_not_linked);
+    return;
+  } else {
+    this->FamilyName = *FamilyName;
+  }
+
+  // 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 /* does not apply when not linking */) ||
+      Args.hasArg(options::OPT_c /* does not apply when not linking */))
+    return;
+
+  if (!GCCInstallation.isValid()) {
+    D.Diag(diag::warn_drv_avr_stdlib_not_linked);
+    return;
+  }
+
+  Optional<std::string> AVRLibcRoot = findAVRLibcInstallation();
+  if (!AVRLibcRoot.hasValue()) {
+    D.Diag(diag::warn_drv_avr_stdlib_not_linked);
+    return;
+  }
+
+  // We have enough information to link stdlibs
 
-        getProgramPaths().push_back(GCCParentPath + "/../bin");
-        getFilePaths().push_back(LibcRoot + std::string("/lib/") + SubPath);
-        getFilePaths().push_back(GCCRoot + std::string("/") + SubPath);
 
-        LinkStdlib = true;
-      }
-    }
-
-    if (!LinkStdlib)
-      D.Diag(diag::warn_drv_avr_stdlib_not_linked);
-  }
-}
 
 void AVRToolChain::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
                                              ArgStringList &CC1Args) const {
   if (DriverArgs.hasArg(options::OPT_nostdinc) ||
       DriverArgs.hasArg(options::OPT_nostdlibinc))
     return;
 
   // Omit if there is no avr-libc installed.
void AVR::Linker::ConstructJob(Compilation &C, const JobAction &JA,
 
   // 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;
   }
 
   // If the family name is known, we can link with the device-specific libgcc.
   // Without it, libgcc will simply not be linked. This matches avr-gcc

clang/lib/Driver/ToolChains/Clang.cpp

 //===-- Clang.cpp - Clang+LLVM ToolChain Implementations --------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Clang.h"
 #include "AMDGPU.h"
 #include "Arch/AArch64.h"
 #include "Arch/ARM.h"
 #include "Arch/M68k.h"
 #include "Arch/Mips.h"
 #include "Arch/PPC.h"
 #include "Arch/RISCV.h"
 #include "Arch/Sparc.h"
 #include "Arch/SystemZ.h"
 #include "Arch/VE.h"
 #include "Arch/X86.h"
+#include "AVR.h"
 #include "CommonArgs.h"
 #include "Hexagon.h"
 #include "MSP430.h"
 #include "PS4CPU.h"
 #include "clang/Basic/CLWarnings.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/LangOptions.h"
#endif
   if (Arg *A = Args.getLastArg(options::OPT_mdouble_EQ)) {
     if (TC.getArch() == llvm::Triple::avr)
       A->render(Args, CmdArgs);
     else
       D.Diag(diag::err_drv_unsupported_opt_for_target)
           << A->getAsString(Args) << TripleStr;
   }
 
+  // Devices in the avr-1 family only support assembly programming.
+  if (TC.getArch() == llvm::Triple::avr) {
+    auto const &AVRTC = static_cast<const toolchains::AVRToolChain &>(TC);
+    if (AVRTC.getFamilyName().equals("avr1"))
+      D.Diag(diag::err_drv_avr_assembly_only)
+          << getCPUName(D, Args, TC.getTriple(), false);
+  }
+
   if (Arg *A = Args.getLastArg(options::OPT_LongDouble_Group)) {
     if (TC.getTriple().isX86())
       A->render(Args, CmdArgs);
     else if (TC.getTriple().isPPC() &&
              (A->getOption().getID() != options::OPT_mlong_double_80))
       A->render(Args, CmdArgs);
     else
       D.Diag(diag::err_drv_unsupported_opt_for_target)

clang/test/Driver/avr-mmcu.c

 // A test for the propagation of the -mmcu option to -cc1 and -cc1as
 
-// RUN: %clang -### -target avr -no-canonical-prefixes -mmcu=attiny11 -save-temps %s 2>&1 | FileCheck -check-prefix=CHECK0 %s
-// CHECK0: clang{{.*}} "-cc1" {{.*}} "-target-cpu" "attiny11"
-// CHECK0: clang{{.*}} "-cc1as" {{.*}} "-target-cpu" "attiny11"
+// RUN: %clang -### -target avr -no-canonical-prefixes -mmcu=attiny11 %s 2>&1 | FileCheck -check-prefix=CHECK0 %s
+// CHECK0: error: device 'attiny11' only supports assembly programming
 
 // RUN: %clang -### -target avr -no-canonical-prefixes -mmcu=at90s2313 -save-temps %s 2>&1 | FileCheck -check-prefix=CHECK1 %s
 // CHECK1: clang{{.*}} "-cc1" {{.*}} "-target-cpu" "at90s2313"
 // CHECK1: clang{{.*}} "-cc1as" {{.*}} "-target-cpu" "at90s2313"
+// CHECK1-NOT: error: device 'at90s2313' only supports assembly programming
 
 // RUN: %clang -### -target avr -no-canonical-prefixes -mmcu=at90s8515 -save-temps %s 2>&1 | FileCheck -check-prefix=CHECK2 %s
 // CHECK2: clang{{.*}} "-cc1" {{.*}} "-target-cpu" "at90s8515"
 // CHECK2: clang{{.*}} "-cc1as" {{.*}} "-target-cpu" "at90s8515"
 
 // RUN: %clang -### -target avr -no-canonical-prefixes -mmcu=attiny13a -save-temps %s 2>&1 | FileCheck -check-prefix=CHECK3 %s
 // CHECK3: clang{{.*}} "-cc1" {{.*}} "-target-cpu" "attiny13a"
 // CHECK3: clang{{.*}} "-cc1as" {{.*}} "-target-cpu" "attiny13a"