[AVR][clang] Pass the address of the data section to the linker for ATmega328

[dylanmckay]

Summary

This patch modifies the Clang AVR toolchain so that it always passes
the '-Tdata=0x800100' to the linker for ATmega328 devices. This matches
AVR-GCC behaviour, and also corresponds to the address of the start of
the data section in data space according to the ATmega328 datasheet.

Without this, clang does not produce a valid ATmega328 binary.

When targeting all non-ATmega328 chips, a warning will be emitted due to
the fact that proper handling for the chips data section address is
not yet implemented.

I've held off adding other microcontrollers for now, mostly because the
AVR toolchain logic is smeared across LLVM core TableGen files, and two Clang
libraries. The 'family detection' logic is also only implemented for
ATmega328 at the moment, for similar reasons.

In the future, I aim to write an RFC to llvm-dev to find a better way
for LLVM to expose target-specific details such as these to compiler
frontends.

[dylanmckay]

Hey @aykevl, could you please confirm that this patch looks okay?

[aykevl]

Looks reasonable to me, although I can't really comment on the contents of this as I'm not very familiar with this code.

I would like to see just a single MCU table that contains all the information (including start addresses). Maybe it can even be generated from ATDF files (http://packs.download.atmel.com/), at least the first time. However, this is a good start (not least because now it warns when producing invalid binaries).

clang/lib/Driver/ToolChains/AVR.cpp

40

I don't think the LLVM coding style says something about this, but coming from Go I'm more used to capitalized abbreviations (MCU, GetMCUSectionAddressData).

However, this is just a superficial thing, feel free to ignore.

[benshi001]

I think this patch is OK to be committed.

And I hope

1. Temporarily using an array for more devices, before a solution of getting info from the tblgen.

2. Distinghuish the arguments for "-L" and "-m". It is correct for atmega328 that avr-ld needs ("-L/usr/lib/avr/lib/avr5" ... "-latmega328p" "-mavr5") But for some special devices, such as attiny24, the following options is needed by avr-ld ("-L/usr/lib/avr/lib/avr25/tiny-stack" ... "-lattiny24" "-mavr25") , due to the avr-lib's file organization.

I am glad to maintain an array before a formal tblgen solution, which includes device family (-m),
lib file sub path (-L), data segment address (-T)

clang/lib/Driver/ToolChains/AVR.cpp

40

I prefer to "Mcu"

[dylanmckay]

clang/lib/Driver/ToolChains/AVR.cpp

40

I will change, thanks for the suggestions. FWIW, Rust prefers the Mcu variant IIRC which is where I adapted this pattern

[dylanmckay]

Regarding TableGen; I would like to send an RFC to llvm-dev to come up with a proper API to expose backend-specific device-specific information and constants to LLVM frontends, as I can imagine that many backends could stand to benefit. I note that there is in general a lot of duplication in individual frontends defining information LLVM already knows about (off the top of my head, inline assembly constraints and registers is one of them but may not easily fit into the paradigm I have in mind).

clang/include/clang/Basic/DiagnosticDriverKinds.td

def warn_drv_avr_gcc_not_found: Warning<
   InGroup<AVRRtlibLinkingQuirks>;
 def warn_drv_avr_libc_not_found: Warning<
   "no avr-libc installation can be found on the system, "
   "cannot link standard libraries">,
   InGroup<AVRRtlibLinkingQuirks>;
 def warn_drv_avr_family_linking_stdlibs_not_implemented: Warning<
   "support for linking stdlibs for microcontroller '%0' is not implemented">,
   InGroup<AVRRtlibLinkingQuirks>;
+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_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.">;

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 "InputInfo.h"
 #include "clang/Driver/Compilation.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Driver/Options.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"
 
 using namespace clang::driver;
 using namespace clang::driver::toolchains;
 using namespace clang::driver::tools;
 using namespace clang;
 using namespace llvm::opt;
 
 namespace {
 
 // TODO: Consider merging this into the AVR device table
 // array in Targets/AVR.cpp.
-llvm::Optional<StringRef> GetMcuFamilyName(StringRef MCU) {
+llvm::Optional<StringRef> GetMCUFamilyName(StringRef MCU) {
   return llvm::StringSwitch<llvm::Optional<StringRef>>(MCU)
       .Case("atmega328", Optional<StringRef>("avr5"))
       .Case("atmega328p", Optional<StringRef>("avr5"))
       .Default(Optional<StringRef>());
 }
 
+llvm::Optional<unsigned> GetMCUSectionAddressData(StringRef MCU) {
+  return llvm::StringSwitch<llvm::Optional<unsigned>>(MCU)
+      .Case("atmega328", Optional<unsigned>(0x800100))
+      .Case("atmega328p", Optional<unsigned>(0x800100))
+      .Default(Optional<unsigned>());
+}
+
 const StringRef PossibleAVRLibcLocations[] = {
     "/usr/avr",
     "/usr/lib/avr",
 };
 
 } // 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(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<StringRef> FamilyName = GetMCUFamilyName(CPU);
       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;
       } else if (!GCCInstallation.isValid()) {

 
 void AVR::Linker::ConstructJob(Compilation &C, const JobAction &JA,
                                const InputInfo &Output,
                                const InputInfoList &Inputs,
                                const ArgList &Args,
                                const char *LinkingOutput) const {
   // Compute information about the target AVR.
   std::string CPU = getCPUName(Args, getToolChain().getTriple());
-  llvm::Optional<StringRef> FamilyName = GetMcuFamilyName(CPU);
+  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());
 
   // 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.
+    getToolChain().getDriver().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
   // behavior.
   if (LinkStdlib) {
     assert(!CPU.empty() && "CPU name must be known in order to link stdlibs");
 
     // Add the object file for the CRT.
     std::string CrtFileName = std::string("-l:crt") + CPU + std::string(".o");