[AVR] Fixed broken linking using `avr-gcc`.

[KOLANICH]

Summary

clang provides arguments to avr-gcc in a wrong order. This results in linking errors undefined reference to .... This patch fixes this.

Bug on GitHub: https://github.com/llvm/llvm-project/issues/56409

[KOLANICH]

Fixing tests

[KOLANICH]

Removed unneeded changes introduced by clang-format

[benshi001]

Could you please give more details about the error you got ?

1. what OS did you use ?
2. What avr-gcc version did you use? And what was its installation path ?
3. What were your input files ? And what were the command options when you run clang ?

[benshi001]

clang/lib/Driver/ToolChains/AVR.cpp

488

I think directly using AddLinkerInputs(getToolChain(), Inputs, Args, CmdArgs, JA); twice is fine, we need not define a lamnda function.

[benshi001]

clang/lib/Driver/ToolChains/AVR.cpp

517

We need to add more tests to test this else branch.

[KOLANICH]

1. >what OS did you use ?

Kubuntu 21.10

2. >What avr-gcc version did you use?

7.3.0-atmel3.6.1-arduino7, it is the latest one that Arduino IDE has updated. Its version number matches the one coming from Atmel Microchip website, but it has some patches by Arduino applied.

And what was its installation path ?

Not installed, just unpacked, it is portable.

What were your input files ?

The ones I have created. I'm not ready to share them they are in pretty dirty state now, but the issue is fundamental and should be reproducible on any project structured as a typical Arduino project and using its standard library and runtime.

And what were the command options when you run clang ?

I have modified the well-known set of CMake toolchain files, here is my fork: https://github.com/KOLANICH/Arduino-CMake-Toolchain/tree/clang

The failed command was

bash
/usr/lib/llvm-15/bin/clang++ --target=avr --sysroot=./arduino_ide/1.8.19/hardware/tools/avr -fno-rtti -mmcu=atmega2560 -fpermissive -fno-exceptions -ffunction-sections -fdata-sections -fno-threadsafe-statics  -O3 -DNDEBUG -mmcu=atmega2560 -lm  -fuse-linker-plugin -v --ld-path="/usr/lib/llvm-15/bin/ld.lld" CMakeFiles/stepper_control.dir/src/main.cpp.obj -o stepper_control.elf  lib_arduino_lib_core.a  lib_arduino_lib_Stepper.a  lib_arduino_lib_core.a

(the paths have been edited, the prefix was replaced to not disclose projects location in the fs)

It results in

clang: warning: argument unused during compilation: '--ld-path=/usr/lib/llvm-15/bin/ld.lld' [-Wunused-command-line-argument]

It seems that lld cannot link for AVR target and it is an another big issue.

bash
"./arduino_ide/1.8.19/hardware/tools/avr/lib/gcc/avr/7.3.0/../../../../bin/avr-ld" -lm CMakeFiles/stepper_control.dir/src/main.cpp.obj lib_arduino_lib_core.a lib_arduino_lib_Stepper.a lib_arduino_lib_core.a -o stepper_control.elf --gc-sections -L./arduino_ide/1.8.19/hardware/tools/avr/lib/gcc/avr/7.3.0/../../../../avr/lib/avr6 -L./arduino_ide/1.8.19/hardware/tools/avr/lib/gcc/avr/7.3.0/avr6 -Tdata=0x800200 --start-group -l:crtatmega2560.o -lgcc -lm -lc -latmega2560 --end-group -mavr6

The actual command - it always calls avr-ld and I have no means to override it. If we put our object files to the group in the end, it links succesfully (if -flto is not enabled. Offtop: if it is enabled, the linker fails to link llvm bitcode, I have tried to link all the bitcode + native code into a joint object file, then compile it using llc, then link using avr-ld and got other undefined reference issues, it seems the optimizer eats some symbols, and using -O0 makes it run out of registers). To be honest, if I had means to override it I'd have just written a shell script instead of taking the burden of compiling clang from source.

./arduino_ide/1.8.19/hardware/tools/avr/lib/gcc/avr/7.3.0/../../../../avr/lib/avr6/crtatmega2560.o:(.init9+0x0): undefined reference to `main'

main is defined within lib_arduino_lib_core.a, which is compiled by the CMake scripts for each target, and it uses setup and loop that are defined in CMakeFiles/stepper_control.dir/src/main.cpp.obj, which is compiled from my code. A typical Arduino project is structured like this.

We need to add more tests to test this else branch.

It should have been already tested, we just changed the place where the user's files are placed into the command line.

I think directly using AddLinkerInputs(getToolChain(), Inputs, Args, CmdArgs, JA); twice is fine, we need not define a lamnda function.

I think it is more error-prone than having a lambda. I expect the optimizer to be smart enough to inline it.

[benshi001]

1. If you can not offer the source code, could please attach the obj files and the library files ? I am quite confused that the order affects.

2. I am uncomfortable with the lamnda, I still think copying twice looks more clear.

[KOLANICH]

Here is the test project. https://github.com/KOLANICH/barebones_arduino_mega_2560_clang_test
Feel free to use its contents in Clang tests.

[KOLANICH]

Synced with the latest changes + addressed some review comments.

[benshi001]

Please give me your full name and an email for the author of commit message.

clang/lib/Driver/ToolChains/AVR.cpp

void AVR::Linker::ConstructJob(Compilation &C, const JobAction &JA,
   llvm::Optional<unsigned> SectionAddressData = GetMCUSectionAddressData(CPU);
 
   // Compute the linker program path, and use GNU "avr-ld" as default.
   const Arg *A = Args.getLastArg(options::OPT_fuse_ld_EQ);
   std::string Linker = A ? getToolChain().GetLinkerPath(nullptr)
                          : 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.
if (SectionAddressData) {
     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
   // behavior.
   if (LinkStdlib) {
     assert(!CPU.empty() && "CPU name must be known in order to link stdlibs");
 
     CmdArgs.push_back("--start-group");
 
     // Add the object file for the CRT.
     std::string CrtFileName = std::string("-l:crt") + CPU + std::string(".o");
     CmdArgs.push_back(Args.MakeArgString(CrtFileName));
 
     CmdArgs.push_back("-lgcc");
     CmdArgs.push_back("-lm");
     CmdArgs.push_back("-lc");
 
     // Add the link library specific to the MCU.
     CmdArgs.push_back(Args.MakeArgString(std::string("-l") + CPU));
 
+    AddLinkerInputs(getToolChain(), Inputs, Args, CmdArgs, JA);
     CmdArgs.push_back("--end-group");
 
     // Add user specified linker script.
     Args.AddAllArgs(CmdArgs, options::OPT_T);
 
     // Specify the family name as the emulation mode to use.
     // This is almost always required because otherwise avr-ld
     // will assume 'avr2' and warn about the program being larger
     // than the bare minimum supports.
     if (Linker.find("avr-ld") != std::string::npos)
       CmdArgs.push_back(Args.MakeArgString(std::string("-m") + *FamilyName));
+  } else {
+    AddLinkerInputs(getToolChain(), Inputs, Args, CmdArgs, JA);
   }
 
   C.addCommand(std::make_unique<Command>(
       JA, *this, ResponseFileSupport::AtFileCurCP(), Args.MakeArgString(Linker),
       CmdArgs, Inputs, Output));
 }
 
 llvm::Optional<std::string> AVRToolChain::findAVRLibcInstallation() const {

clang/test/Driver/avr-ld.c

 // RUN: %clang -### --target=avr -mmcu=at90s2313 --sysroot %S/Inputs/basic_avr_tree %s 2>&1 | FileCheck -check-prefix LINKA %s
-// LINKA: {{".*ld.*"}} {{.*}} {{"-L.*tiny-stack"}} {{.*}} "-Tdata=0x800060" "--start-group" {{.*}} "-lat90s2313" "--end-group" "-mavr2"
+// LINKA: {{".*ld.*"}} {{.*}} {{"-L.*tiny-stack"}} {{.*}} "-Tdata=0x800060" "--start-group" {{.*}} "-lat90s2313" {{.*}} "--end-group" "-mavr2"
 
 // RUN: %clang -### --target=avr -mmcu=at90s8515 --sysroot %S/Inputs/basic_avr_tree %s 2>&1 | FileCheck -check-prefix LINKB %s
-// LINKB: {{".*ld.*"}} {{.*}} "-Tdata=0x800060" "--start-group" {{.*}} "-lat90s8515" "--end-group" "-mavr2"
+// LINKB: {{".*ld.*"}} {{.*}} "-Tdata=0x800060" "--start-group" {{.*}} "-lat90s8515" {{.*}} "--end-group" "-mavr2"
 
 // RUN: %clang -### --target=avr -mmcu=attiny13 --sysroot %S/Inputs/basic_avr_tree %s 2>&1 | FileCheck -check-prefix LINKC %s
-// LINKC: {{".*ld.*"}} {{.*}} {{"-L.*avr25/tiny-stack"}} {{.*}} "-Tdata=0x800060" "--start-group" {{.*}} "-lattiny13" "--end-group" "-mavr25"
+// LINKC: {{".*ld.*"}} {{.*}} {{"-L.*avr25/tiny-stack"}} {{.*}} "-Tdata=0x800060" "--start-group" {{.*}} "-lattiny13" {{.*}} "--end-group" "-mavr25"
 
 // RUN: %clang -### --target=avr -mmcu=attiny44 --sysroot %S/Inputs/basic_avr_tree %s 2>&1 | FileCheck -check-prefix LINKD %s
-// LINKD: {{".*ld.*"}} {{.*}} {{"-L.*avr25"}} {{.*}} "-Tdata=0x800060" "--start-group" {{.*}} "-lattiny44" "--end-group" "-mavr25"
+// LINKD: {{".*ld.*"}} {{.*}} {{"-L.*avr25"}} {{.*}} "-Tdata=0x800060" "--start-group" {{.*}} "-lattiny44" {{.*}} "--end-group" "-mavr25"
 
 // RUN: %clang -### --target=avr -mmcu=atmega103 --sysroot %S/Inputs/basic_avr_tree %s 2>&1 | FileCheck -check-prefix LINKE %s
-// LINKE: {{".*ld.*"}} {{.*}} {{"-L.*avr31"}} {{.*}} "-Tdata=0x800060" "--start-group" {{.*}} "-latmega103" "--end-group" "-mavr31"
+// LINKE: {{".*ld.*"}} {{.*}} {{"-L.*avr31"}} {{.*}} "-Tdata=0x800060" "--start-group" {{.*}} "-latmega103" {{.*}} "--end-group" "-mavr31"
 
 // RUN: %clang -### --target=avr -mmcu=atmega8u2 --sysroot %S/Inputs/basic_avr_tree %s 2>&1 | FileCheck -check-prefix LINKF %s
-// LINKF: {{".*ld.*"}} {{.*}} {{"-L.*avr35"}} {{.*}} "-Tdata=0x800100" "--start-group" {{.*}} "-latmega8u2" "--end-group" "-mavr35"
+// LINKF: {{".*ld.*"}} {{.*}} {{"-L.*avr35"}} {{.*}} "-Tdata=0x800100" "--start-group" {{.*}} "-latmega8u2" {{.*}} "--end-group" "-mavr35"
 
 // RUN: %clang -### --target=avr -mmcu=atmega48pa --sysroot %S/Inputs/basic_avr_tree %s 2>&1 | FileCheck -check-prefix LINKG %s
-// LINKG: {{".*ld.*"}} {{.*}} {{"-L.*avr4"}} {{.*}} "-Tdata=0x800100" "--start-group" {{.*}} "-latmega48pa" "--end-group" "-mavr4"
+// LINKG: {{".*ld.*"}} {{.*}} {{"-L.*avr4"}} {{.*}} "-Tdata=0x800100" "--start-group" {{.*}} "-latmega48pa" {{.*}} "--end-group" "-mavr4"
 
 // RUN: %clang -### --target=avr -mmcu=atmega328 --sysroot %S/Inputs/basic_avr_tree %s 2>&1 | FileCheck -check-prefix LINKH %s
-// LINKH: {{".*ld.*"}} {{.*}} {{"-L.*avr5"}} {{.*}} "-Tdata=0x800100" "--start-group" {{.*}} "-latmega328" "--end-group" "-mavr5"
+// LINKH: {{".*ld.*"}} {{.*}} {{"-L.*avr5"}} {{.*}} "-Tdata=0x800100" "--start-group" {{.*}} "-latmega328" {{.*}} "--end-group" "-mavr5"
 
 // RUN: %clang -### --target=avr -mmcu=atmega1281 --sysroot %S/Inputs/basic_avr_tree %s 2>&1 | FileCheck -check-prefix LINKI %s
-// LINKI: {{".*ld.*"}} {{.*}} {{"-L.*avr51"}} {{.*}} "-Tdata=0x800200" "--start-group" {{.*}} "-latmega1281" "--end-group" "-mavr51"
+// LINKI: {{".*ld.*"}} {{.*}} {{"-L.*avr51"}} {{.*}} "-Tdata=0x800200" "--start-group" {{.*}} "-latmega1281" {{.*}} "--end-group" "-mavr51"
 
 // RUN: %clang -### --target=avr -mmcu=atmega2560 --sysroot %S/Inputs/basic_avr_tree %s 2>&1 | FileCheck -check-prefix LINKJ %s
-// LINKJ: {{".*ld.*"}} {{.*}} {{"-L.*avr6"}} {{.*}} "-Tdata=0x800200" "--start-group" {{.*}} "-latmega2560" "--end-group" "-mavr6"
+// LINKJ: {{".*ld.*"}} {{.*}} {{"-L.*avr6"}} {{.*}} "-Tdata=0x800200" "--start-group" {{.*}} "-latmega2560" {{.*}} "--end-group" "-mavr6"
 
 // RUN: %clang -### --target=avr -mmcu=attiny10 --sysroot %S/Inputs/basic_avr_tree %s 2>&1 | FileCheck -check-prefix LINKK %s
-// LINKK: {{".*ld.*"}} {{.*}} {{"-L.*avrtiny"}} {{.*}} "-Tdata=0x800040" "--start-group" {{.*}} "-lattiny10" "--end-group" "-mavrtiny"
+// LINKK: {{".*ld.*"}} {{.*}} {{"-L.*avrtiny"}} {{.*}} "-Tdata=0x800040" "--start-group" {{.*}} "-lattiny10" {{.*}} "--end-group" "-mavrtiny"
 
 // RUN: %clang -### --target=avr -mmcu=atxmega16a4 --sysroot %S/Inputs/basic_avr_tree %s 2>&1 | FileCheck -check-prefix LINKL %s
-// LINKL: {{".*ld.*"}} {{.*}} {{"-L.*avrxmega2"}} {{.*}} "-Tdata=0x802000" "--start-group" {{.*}} "-latxmega16a4" "--end-group" "-mavrxmega2"
+// LINKL: {{".*ld.*"}} {{.*}} {{"-L.*avrxmega2"}} {{.*}} "-Tdata=0x802000" "--start-group" {{.*}} "-latxmega16a4" {{.*}} "--end-group" "-mavrxmega2"
 
 // RUN: %clang -### --target=avr -mmcu=atxmega64b3 --sysroot %S/Inputs/basic_avr_tree %s 2>&1 | FileCheck -check-prefix LINKM %s
-// LINKM: {{".*ld.*"}} {{.*}} {{"-L.*avrxmega4"}} {{.*}} "-Tdata=0x802000" "--start-group" {{.*}} "-latxmega64b3" "--end-group" "-mavrxmega4"
+// LINKM: {{".*ld.*"}} {{.*}} {{"-L.*avrxmega4"}} {{.*}} "-Tdata=0x802000" "--start-group" {{.*}} "-latxmega64b3" {{.*}} "--end-group" "-mavrxmega4"
 
 // RUN: %clang -### --target=avr -mmcu=atxmega128a3u --sysroot %S/Inputs/basic_avr_tree %s 2>&1 | FileCheck -check-prefix LINKN %s
-// LINKN: {{".*ld.*"}} {{.*}} {{"-L.*avrxmega6"}} {{.*}} "-Tdata=0x802000" "--start-group" {{.*}} "-latxmega128a3u" "--end-group" "-mavrxmega6"
+// LINKN: {{".*ld.*"}} {{.*}} {{"-L.*avrxmega6"}} {{.*}} "-Tdata=0x802000" "--start-group" {{.*}} "-latxmega128a3u" {{.*}} "--end-group" "-mavrxmega6"
 
 // RUN: %clang -### --target=avr -mmcu=atxmega128a1 --sysroot %S/Inputs/basic_avr_tree %s 2>&1 | FileCheck -check-prefix LINKO %s
-// LINKO: {{".*ld.*"}} {{.*}} {{"-L.*avrxmega7"}} {{.*}} "-Tdata=0x802000" "--start-group" {{.*}} "-latxmega128a1" "--end-group" "-mavrxmega7"
+// LINKO: {{".*ld.*"}} {{.*}} {{"-L.*avrxmega7"}} {{.*}} "-Tdata=0x802000" "--start-group" {{.*}} "-latxmega128a1" {{.*}} "--end-group" "-mavrxmega7"