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

[benshi001]

There is an old solution in https://reviews.llvm.org/D117423. The concern over there is that AVR specific checks should be implemented in AVR toolchain specific files. And this patch obeys that.

[MaskRay]

I don't know the AVR specific thing but having this toolchain-specific diagnostic in the toolchain-specific file seems the right direction.

clang/lib/Driver/ToolChains/AVR.cpp

418

https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop

[benshi001]

clang/lib/Driver/ToolChains/AVR.cpp

393

Currently command clang xx.c --target=avr -mmcu=attiny40 leads to wrong assembly generated, so we temperarily disable the avrtiny family. Until https://github.com/llvm/llvm-project/issues/53459 is fixed.

[MaskRay]

I do not know whether the complexity is justified. Rejecting some -mmcu= for C source files looks quite dubious. Does it really help users?

Bear in mind that all these complexity translates to clang executable size increase for other users (most people don't use AVR).

[benshi001]

In D118095#3282039, @MaskRay wrote:

I do not know whether the complexity is justified. Rejecting some -mmcu= for C source files looks quite dubious. Does it really help users?

Bear in mind that all these complexity translates to clang executable size increase for other users (most people don't use AVR).

Actually I would like to make clang+llvm+lld fully compatible with gcc+binutils (functionality, ABI, quality, ...), and finally replace avr-gcc as Arduino's default toolchain. (https://www.arduino.cc/en/software, Arduino is a popular MCU platform for hardware prototype development & verification)

Even many avr-libc (https://www.nongnu.org/avr-libc/) developers are trying build it with clang.

If you think it is complex, we can remove the extreme check of -x assembler-with-cpp, which is rarely in a real world.

[benshi001]

The other implementation (https://reviews.llvm.org/D117423) is abundoned, since we should not put AVR specified checks in the common code in Clang.cpp.

[aykevl]

@MaskRay it was my suggestion to move this from the toolchain specific file to the generic file, because it makes the implementation much simpler. See my comment D117423#3251110 for details.

In D118095#3282039, @MaskRay wrote:

Rejecting some -mmcu= for C source files looks quite dubious. Does it really help users?

For context: the avr1 family isn't supported by avr-gcc either. It's a old and rather limited subset of the AVR instruction set. I assume it's going to be rather difficult (and not worth the trouble) to write a C compiler for it, as it doesn't even have a fully functional stack. From Wikipedia:

The AVR1 subset was not popular and no new models have been introduced since 2000. It omits all RAM except for the 32 registers mapped at address 0–31 and the I/O ports at addresses 32–95. The stack is replaced by a 3-level hardware stack, and the PUSH and POP instructions are deleted. All 16-bit operations are deleted, as are IJMP, ICALL, and all load and store addressing modes except indirect via Z.

So I think the idea is to disallow this family so that users won't accidentally try to use a C compiler for these chips. However, writing assembly might still make sense.

[benshi001]

In D118095#3408037, @aykevl wrote:

@MaskRay it was my suggestion to move this from the toolchain specific file to the generic file, because it makes the implementation much simpler. See my comment D117423#3251110 for details.

In D118095#3282039, @MaskRay wrote:

Rejecting some -mmcu= for C source files looks quite dubious. Does it really help users?

For context: the avr1 family isn't supported by avr-gcc either. It's a old and rather limited subset of the AVR instruction set. I assume it's going to be rather difficult (and not worth the trouble) to write a C compiler for it, as it doesn't even have a fully functional stack. From Wikipedia:

The AVR1 subset was not popular and no new models have been introduced since 2000. It omits all RAM except for the 32 registers mapped at address 0–31 and the I/O ports at addresses 32–95. The stack is replaced by a 3-level hardware stack, and the PUSH and POP instructions are deleted. All 16-bit operations are deleted, as are IJMP, ICALL, and all load and store addressing modes except indirect via Z.

So I think the idea is to disallow this family so that users won't accidentally try to use a C compiler for these chips. However, writing assembly might still make sense.

My opinion is: we indeed need to be compatible with avr-gcc (accept assembly but reject c progarms), but we do it in AVR specific files and let common code clean.

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 {
+  std::string AVRMcu;
+
 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

clang/lib/Driver/ToolChains/AVR.cpp

 //===--- AVR.cpp - AVR 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 "AVR.h"
 #include "CommonArgs.h"
 #include "clang/Driver/Compilation.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Driver/InputInfo.h"
 #include "clang/Driver/Options.h"
+#include "clang/Driver/Types.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/MC/SubtargetFeature.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
 
 using namespace clang::driver;
 using namespace clang::driver::toolchains;
 using namespace clang::driver::tools;
 using namespace clang;
 using namespace llvm::opt;
 
 namespace {

 } // 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);
 
+  // Save the CPU name for further checks.
+  AVRMcu = getCPUName(D, Args, Triple);
+
   // 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 */)) {
-    std::string CPU = getCPUName(D, Args, Triple);
 
-    if (CPU.empty()) {
+    if (AVRMcu.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<StringRef> FamilyName = GetMCUFamilyName(AVRMcu);
       Optional<std::string> AVRLibcRoot = findAVRLibcInstallation();
 
       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;
+            << AVRMcu;
       } 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);
+        std::string SubPath = GetMCUSubPath(AVRMcu);
 
         getProgramPaths().push_back(GCCParentPath + "/../bin");
         getFilePaths().push_back(LibcRoot + std::string("/lib/") + SubPath);
         getFilePaths().push_back(GCCRoot + std::string("/") + SubPath);
 
         LinkStdlib = true;
       }
     }

 void AVRToolChain::addClangTargetOptions(
     const llvm::opt::ArgList &DriverArgs, llvm::opt::ArgStringList &CC1Args,
     Action::OffloadKind DeviceOffloadKind) const {
   // By default, use `.ctors` (not `.init_array`), as required by libgcc, which
   // runs constructors/destructors on AVR.
   if (!DriverArgs.hasFlag(options::OPT_fuse_init_array,
                           options::OPT_fno_use_init_array, false))
     CC1Args.push_back("-fno-use-init-array");
+
+  // Reject non assembly source files for devices in the avr1 family.
+  // FIXME: Enable avrtiny when the following bug is fixed:
+  // https://github.com/llvm/llvm-project/issues/53459
+  llvm::Optional<StringRef> FamilyName = GetMCUFamilyName(AVRMcu);
+  if (FamilyName.hasValue() &&
+      (FamilyName->equals("avr1") || FamilyName->equals("avrtiny"))) {
+    // Only '-x assembler-with-cpp' or '-x assembler' is allowed for avr1.
+    auto Langs = DriverArgs.getAllArgValues(options::OPT_x);
+    for (auto Lang : Langs)
+      if (Lang != "assembler-with-cpp" && Lang != "assembler")
+        getDriver().Diag(diag::err_drv_avr_assembly_only) << AVRMcu;
+
+    // Get the main file name.
+    const char *MainFileName = nullptr;
+    for (unsigned I = 0, J = CC1Args.size(); I < J; I++)
+      if (StringRef(CC1Args[I]).equals("-main-file-name")) {
+        MainFileName = CC1Args[I + 1];
+        break;
+      }
+    if (!MainFileName)
+      return;
+
+    // The main input file should be assembly only, it is OK that the main
+    // input file has either a '.S' or a '.s' suffix.
+    types::ID MainFileId = types::lookupTypeForExtension(
+        llvm::sys::path::extension(MainFileName).drop_front());
+    if (MainFileId == types::TY_Asm || MainFileId == types::TY_PP_Asm)
+      return;
+
+    // The main input file should be assembly only, it should be led by either
+    // '-x assembler-with-cpp' or '-x assembler', if it has neither a '.S' nor
+    // a '.s' suffix.
+    for (unsigned I = 0, J = DriverArgs.size(); I < J; I++) {
+      StringRef Arg(DriverArgs.getArgString(I));
+      if (Arg.equals("assembler-with-cpp") || Arg.equals("assembler"))
+        return;
+      if (Arg.contains(MainFileName))
+        getDriver().Diag(diag::err_drv_avr_assembly_only) << AVRMcu;
+    }
+  }
 }
 
 Tool *AVRToolChain::buildLinker() const {
   return new tools::AVR::Linker(getTriple(), *this, LinkStdlib);
 }
 
 void AVR::Linker::ConstructJob(Compilation &C, const JobAction &JA,
                                const InputInfo &Output,

clang/test/Driver/avr-mmcu.S

+// A test for the supported language mode by different avr families.
+
+// RUN: %clang -### -target avr -no-canonical-prefixes -mmcu=attiny11 %s -c 2>&1 | FileCheck -check-prefix=CHECKA %s
+// CHECKA-NOT: error: device 'attiny11' only supports assembly programming
+
+// RUN: %clang -### -target avr -no-canonical-prefixes -mmcu=attiny11 -x c++ %s -c 2>&1 | FileCheck -check-prefix=CHECKB %s
+// CHECKB: error: device 'attiny11' only supports assembly programming
+
+// RUN: %clang -### -target avr -no-canonical-prefixes -mmcu=at90s8515 %s -c 2>&1 | FileCheck -check-prefix=CHECKC %s
+// CHECKC-NOT: error: device 'at90s8515' only supports assembly programming
+
+// RUN: %clang -### -target avr -no-canonical-prefixes -mmcu=at90s8515 %s -x c++ -c 2>&1 | FileCheck -check-prefix=CHECKD %s
+// CHECKD-NOT: error: device 'at90s8515' only supports assembly programming

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 -c 2>&1 | FileCheck -check-prefix=CHECKA %s
+// CHECKA: error: device 'attiny11' only supports assembly programming
+
+// RUN: %clang -### -target avr -no-canonical-prefixes -mmcu=attiny11 -x assembler-with-cpp %s -c 2>&1 | FileCheck -check-prefix=CHECKB %s
+// CHECKB-NOT: 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"