[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

419

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

[benshi001]

clang/lib/Driver/ToolChains/AVR.cpp

417

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 ↗

(On Diff #402379)

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

[benshi001]

clang/lib/Driver/ToolChains/Clang.cpp

5149 ↗

(On Diff #402379)

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 err_drv_nested_config_file: Error<
   "option '--config' is not allowed inside configuration file">;
 def err_drv_arg_requires_bitcode_input: Error<
   "option '%0' requires input to be LLVM bitcode">;
 
 def err_target_unsupported_arch
   : Error<"the target architecture '%0' is not supported by the target '%1'">;
 def err_cpu_unsupported_isa
   : Error<"CPU '%0' does not support '%1' execution mode">;
+def err_cpu_unsupported_language
+  : Error<"CPU '%0' does not support '%1' language mode">;
 def err_arch_unsupported_isa
   : Error<"architecture '%0' does not support '%1' execution mode">;
 
 def err_drv_I_dash_not_supported : Error<
   "'%0' not supported, please use -iquote instead">;
 def err_drv_unknown_argument : Error<"unknown argument: '%0'">;
 def err_drv_unknown_argument_with_suggestion : Error<
   "unknown argument '%0'; did you mean '%1'?">;

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_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");
+
+  // Search for the main file name.
+  const char *MainFileName = nullptr;
+  for (unsigned I = 0; I < CC1Args.size(); I++)
+    if (StringRef(CC1Args[I]) == "-main-file-name")
+      MainFileName = CC1Args[I + 1];
+  // Report an error if the main input file is not assembly and the avr mcu
+  // belongs to the AVR1 family.
+  if (MainFileName) {
+    llvm::Optional<StringRef> FamilyName = GetMCUFamilyName(AVRMcu);
+    if (FamilyName.hasValue() && *FamilyName == "avr1") {
+      types::ID MainFileId = types::lookupTypeForExtension(
+          llvm::sys::path::extension(MainFileName).drop_front());
+      if (MainFileId != types::TY_Asm)
+        getDriver().Diag(diag::err_cpu_unsupported_language)
+            << AVRMcu << getTypeName(MainFileId);
+    }
+  }
 }
 
 Tool *AVRToolChain::buildLinker() const {
   return new tools::AVR::Linker(getTriple(), *this, LinkStdlib);
 }
 
 void AVR::Linker::ConstructJob(Compilation &C, const JobAction &JA,
                                const InputInfo &Output,
                                const InputInfoList &Inputs, const ArgList &Args,
                                const char *LinkingOutput) const {
   const Driver &D = getToolChain().getDriver();
 
   // Compute information about the target AVR.
   std::string CPU = getCPUName(D, Args, getToolChain().getTriple());
   llvm::Optional<StringRef> FamilyName = GetMCUFamilyName(CPU);
   llvm::Optional<unsigned> SectionAddressData = GetMCUSectionAddressData(CPU);
 
   std::string Linker = getToolChain().GetProgramPath(getShortName());
   ArgStringList CmdArgs;
   AddLinkerInputs(getToolChain(), Inputs, Args, CmdArgs, JA);
 
   CmdArgs.push_back("-o");
   CmdArgs.push_back(Output.getFilename());
 

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: CPU 'attiny11' does not support 'c' language mode
 
 // 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"
 
 // 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"