[Driver][AVR] Don't emit default '-Tdata' when '-T' is specified

[benshi001]

Summary

Updates https://github.com/llvm/llvm-project/issues/60362

[MaskRay]

This is probably not a good idea. See https://github.com/llvm/llvm-project/issues/60203#issuecomment-1407532083

[benshi001]

In D142595#4088364, @MaskRay wrote:

This is probably not a good idea. See https://github.com/llvm/llvm-project/issues/60203#issuecomment-1407532083

I agree with your comment in https://github.com/llvm/llvm-project/issues/60203#issuecomment-1407532083, But I still think my patch is worth to be applied.

Since you mentioned It's not a problem to have multiple -Tdata=. The last wins. But what will happen if both a linker script and a -Wl,-Tdata are specified? ASFAIK, the command line option will win.

But the default "-Tdata" is added according to each specific AVR device SRAM layout (https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/AVR.cpp#L35).

My opinion is:

1. We should add a default SRAM address to the linker, if user does not specify any link options. This is current behaviour of clang's main branch.
2. If the user explictly specifies a linker scrpit, we should follow the user's willing, and do not decide the SRAM address for him.

[benshi001]

clang/lib/Driver/ToolChains/AVR.cpp

499–500

I have added comment about why we need this change, it has nothing to do with https://github.com/llvm/llvm-project/issues/60203.

[MaskRay]

OK. If this doesn't add -Tdata= driver options, I'm fine with it.
But why is default -Tdata added in the first place?

Most linker scripts are added as -Wl,-T,a.lds (-Wl, values are opaque to the driver), so the driver cannot really know whether a linker script is used.

[benshi001]

In D142595#4089124, @MaskRay wrote:

OK. If this doesn't add -Tdata= driver options, I'm fine with it.
But why is default -Tdata added in the first place?

Most linker scripts are added as -Wl,-T,a.lds (-Wl, values are opaque to the driver), so the driver cannot really know whether a linker script is used.

So how about change to

1. move the default -Tdata to a later position.
2. clang driver just checks -T but omits -Wl, this can not 100% fix the issue, but at least improve it. Current most users are from avr-gcc, who have get used to -T.

A good solution is using default linker script provided by the avr-libc, which will be overwritten by user's explicit specification.

I will do that solution in a different patch, it needs more extra work. Currently we just guarantee avr-gcc users's -T option will not be broken.

[benshi001]

clang/lib/Driver/ToolChains/AVR.cpp

544

Just moving the default -T from previous position to here, without any change.

[MaskRay]

In D142595#4089391, @benshi001 wrote:

In D142595#4089124, @MaskRay wrote:

OK. If this doesn't add -Tdata= driver options, I'm fine with it.
But why is default -Tdata added in the first place?

Most linker scripts are added as -Wl,-T,a.lds (-Wl, values are opaque to the driver), so the driver cannot really know whether a linker script is used.

So how about change to

1. move the default -Tdata to a later position.
2. clang driver just checks -T but omits -Wl, this can not 100% fix the issue, but at least improve it. Current most users are from avr-gcc, who have get used to -T.

A good solution is using default linker script provided by the avr-libc, which will be overwritten by user's explicit specification.

I will do that solution in a different patch, it needs more extra work. Currently we just guarantee avr-gcc users's -T option will not be broken.

I think checking whether there is a linker script in any way is not a right solution.
GNU ld supports -dT which lld doesn't support. A linker script can be used without a -T/-dT option as well (it gets appended).
Any chance to remove -Tdata= and require the user to specify a -Tdata=?

[benshi001]

In D142595#4091176, @MaskRay wrote:

In D142595#4089391, @benshi001 wrote:

In D142595#4089124, @MaskRay wrote:

OK. If this doesn't add -Tdata= driver options, I'm fine with it.
But why is default -Tdata added in the first place?

Most linker scripts are added as -Wl,-T,a.lds (-Wl, values are opaque to the driver), so the driver cannot really know whether a linker script is used.

So how about change to

1. move the default -Tdata to a later position.
2. clang driver just checks -T but omits -Wl, this can not 100% fix the issue, but at least improve it. Current most users are from avr-gcc, who have get used to -T.

A good solution is using default linker script provided by the avr-libc, which will be overwritten by user's explicit specification.

I will do that solution in a different patch, it needs more extra work. Currently we just guarantee avr-gcc users's -T option will not be broken.

I think checking whether there is a linker script in any way is not a right solution.
GNU ld supports -dT which lld doesn't support. A linker script can be used without a -T/-dT option as well (it gets appended).
Any chance to remove -Tdata= and require the user to specify a -Tdata=?

@MaskRay

Newer avr-gcc's solution is adding a default linker script (provided by binutils, locates at $binutils-gdb/ld/scripttempl/avr.sc). And user can specifies his own one, which will win against the default one.

I will remove the default -Tdata, and append a default -Tavr.sc along with several --defsym (required by avr.sc).

How do you think about this solution ?

[MaskRay]

How does avr-gcc know whether the user provides a linker script?

I really don't like adding a default -Tdata= when it can interfere with common uses of specifying a linker script. In these situations drivers not adding default options is usually better than guessing the user intention.
If the use case is somewhat strange, we can document it.

[benshi001]

In D142595#4092435, @MaskRay wrote:

How does avr-gcc know whether the user provides a linker script?

I really don't like adding a default -Tdata= when it can interfere with common uses of specifying a linker script. In these situations drivers not adding default options is usually better than guessing the user intention.
If the use case is somewhat strange, we can document it.

My option,

1. we remove default -Tdata
2. we add a default -T avr.sc (provided by bintuils as default) at an early position of avr-ld's options.
3. If user specified -T xx.lds, append user's at an late position, then user's will win, the default will lose.

[benshi001]

In D142595#4092435, @MaskRay wrote:

How does avr-gcc know whether the user provides a linker script?

I really don't like adding a default -Tdata= when it can interfere with common uses of specifying a linker script. In these situations drivers not adding default options is usually better than guessing the user intention.
If the use case is somewhat strange, we can document it.

I have a question. linux-x86 has its default linker script, which can also be specified by user. How does clang driver implement that? Could you please show me where clang handle this ?

I hope avr also follow this logic. User's willing go first, then default linker script.

clang/lib/Driver/ToolChains/AVR.cpp

if (!CPU.empty()) {
           CmdArgs.push_back(Args.MakeArgString("-L" + TC.getGCCInstallPath() +
                                                "/" + SubPath));
         LinkStdlib = true;
       }
     }
     if (!LinkStdlib)
       D.Diag(diag::warn_drv_avr_stdlib_not_linked);
   }
 
-  if (SectionAddressData) {
-    CmdArgs.push_back(Args.MakeArgString(
-        "-Tdata=0x" + Twine::utohexstr(*SectionAddressData)));
-  } 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.
if (RtLib == ToolChain::RLT_CompilerRT) {
       std::string RtLib =
           getToolChain().getCompilerRT(Args, "builtins", ToolChain::FT_Static);
       if (llvm::sys::fs::exists(RtLib))
         CmdArgs.push_back(Args.MakeArgString(RtLib));
     }
 
     CmdArgs.push_back("--end-group");
 
+    // If user explicitly specifies a linker script via '-T', we should follow.
+    // Otherwise we must add a default '-Tdata' option to the linker, according
+    // to each AVR device's specific memory layout.
+    // TODO: Use the default linker script provided by avr-libc, which can be
+    // overwritten if user specifies via '-T' or '-Wl,-T'.
+    if (!Args.hasArg(options::OPT_T)) {
+      if (SectionAddressData) {
+        CmdArgs.push_back(Args.MakeArgString(
+            "-Tdata=0x" + Twine::utohexstr(*SectionAddressData)));
+      } 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;
+      }
+    } else {
       // 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 {

clang/test/Driver/avr-ld.c

 // RUN: %clang -### --target=avr -mmcu=at90s2313 --rtlib=libgcc --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"}} {{.*}} "--start-group" {{.*}} "-lat90s2313" {{.*}} "--end-group" "-Tdata=0x800060" "-mavr2"
 
 // RUN: %clang -### --target=avr -mmcu=at90s8515 --rtlib=libgcc --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.*"}} {{.*}} "--start-group" {{.*}} "-lat90s8515" {{.*}} "--end-group" "-Tdata=0x800060" "-mavr2"
 
 // RUN: %clang -### --target=avr -mmcu=attiny13 --rtlib=libgcc --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"}} {{.*}} "--start-group" {{.*}} "-lattiny13" {{.*}} "--end-group" "-Tdata=0x800060" "-mavr25"
 
 // RUN: %clang -### --target=avr -mmcu=attiny44 --rtlib=libgcc --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"}} {{.*}} "--start-group" {{.*}} "-lattiny44" {{.*}} "--end-group" "-Tdata=0x800060" "-mavr25"
 
 // RUN: %clang -### --target=avr -mmcu=atmega103 --rtlib=libgcc --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"}} {{.*}} "--start-group" {{.*}} "-latmega103" {{.*}} "--end-group" "-Tdata=0x800060" "-mavr31"
 
 // RUN: %clang -### --target=avr -mmcu=atmega8u2 --rtlib=libgcc --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"}} {{.*}} "--start-group" {{.*}} "-latmega8u2" {{.*}} "--end-group" "-Tdata=0x800100" "-mavr35"
 
 // RUN: %clang -### --target=avr -mmcu=atmega48pa --rtlib=libgcc --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"}} {{.*}} "--start-group" {{.*}} "-latmega48pa" {{.*}} "--end-group" "-Tdata=0x800100" "-mavr4"
 
 // RUN: %clang -### --target=avr -mmcu=atmega328 --rtlib=libgcc --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"}} {{.*}} "--start-group" {{.*}} "-latmega328" {{.*}} "--end-group" "-Tdata=0x800100" "-mavr5"
 
 // RUN: %clang -### --target=avr -mmcu=atmega1281 --rtlib=libgcc --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"}} {{.*}} "--start-group" {{.*}} "-latmega1281" {{.*}} "--end-group" "-Tdata=0x800200" "-mavr51"
 
 // RUN: %clang -### --target=avr -mmcu=atmega2560 --rtlib=libgcc --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"}} {{.*}} "--start-group" {{.*}} "-latmega2560" {{.*}} "--end-group" "-Tdata=0x800200" "-mavr6"
 
 // RUN: %clang -### --target=avr -mmcu=attiny10 --rtlib=libgcc --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"}} {{.*}} "--start-group" {{.*}} "-lattiny10" {{.*}} "--end-group" "-Tdata=0x800040" "-mavrtiny"
 
 // RUN: %clang -### --target=avr -mmcu=atxmega16a4 --rtlib=libgcc --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"}} {{.*}} "--start-group" {{.*}} "-latxmega16a4" {{.*}} "--end-group" "-Tdata=0x802000" "-mavrxmega2"
 
 // RUN: %clang -### --target=avr -mmcu=atxmega64b3 --rtlib=libgcc --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"}} {{.*}} "--start-group" {{.*}} "-latxmega64b3" {{.*}} "--end-group" "-Tdata=0x802000" "-mavrxmega4"
 
 // RUN: %clang -### --target=avr -mmcu=atxmega128a3u --rtlib=libgcc --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"}} {{.*}} "--start-group" {{.*}} "-latxmega128a3u" {{.*}} "--end-group" "-Tdata=0x802000" "-mavrxmega6"
 
 // RUN: %clang -### --target=avr -mmcu=atxmega128a1 --rtlib=libgcc --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"}} {{.*}} "--start-group" {{.*}} "-latxmega128a1" {{.*}} "--end-group" "-Tdata=0x802000" "-mavrxmega7"
+
+// RUN: %clang -### --target=avr -mmcu=atmega8u2 --sysroot %S/Inputs/basic_avr_tree %s -T %S/Inputs/basic_avr_tree/usr/lib/avr/lib/avr35.lds 2>&1 | FileCheck -check-prefix LINKP %s
+// LINKP: {{".*ld.*"}} {{.*}} {{"-L.*avr35"}} {{.*}} "--start-group" {{.*}} "-latmega8u2" {{.*}} "--end-group" "-T" {{".*avr35.lds"}} "-mavr35"
+// LINKP-NOT: "-Tdata"
+
+// RUN: %clang -### --target=avr --sysroot %S/Inputs/basic_avr_tree %s -T %S/Inputs/basic_avr_tree/usr/lib/avr/lib/avr51.lds 2>&1 | FileCheck -check-prefix LINKQ %s
+// LINKQ-NOT: "-Tdata"
+// LINKQ-NOT: "warning:" {{.*}} "section address"

clang/test/Driver/avr-toolchain.c

 // UNSUPPORTED: system-windows
 // A basic clang -cc1 command-line.
 
 // RUN: %clang -### %s --target=avr --sysroot=%S/Inputs/basic_avr_tree -resource-dir=%S/Inputs/resource_dir 2>&1 | FileCheck --check-prefix=CHECK1 %s
 // CHECK1: "-cc1" "-triple" "avr"
 // CHECK1-SAME: "-resource-dir" "[[RESOURCE:[^"]+]]"
 // CHECK1-SAME: "-isysroot" "[[SYSROOT:[^"]+/basic_avr_tree]]"
 // CHECK1-SAME: "-internal-isystem"
 // CHECK1-SAME: {{^}} "[[SYSROOT]]/usr/lib/gcc/avr/5.4.0/../../../avr/include"
-// CHECK1-NOT:  "-L
-// CHECK1:      avr-ld"
+// CHECK1-NOT:  "-L"
+// CHECK1:      {{".*avr-ld"}}
 // CHECK1-SAME: "-o" "a.out"
 // CHECK1-SAME: {{^}} "--gc-sections"
 
 // RUN: %clang -### %s --target=avr --sysroot=%S/Inputs/basic_avr_tree_2/opt/local -S 2>&1 | FileCheck --check-prefix=CHECK2 %s
 // CHECK2: "-cc1" "-triple" "avr"
 // CHECK2-SAME: "-isysroot" "[[SYSROOT:[^"]+/basic_avr_tree_2/opt/local]]"
 // CHECK2-SAME: "-internal-isystem"
 // CHECK2-SAME: {{^}} "[[SYSROOT]]/lib/gcc/avr/10.3.0/../../../../avr/include"

 // RUN: %clang -### --target=avr --sysroot=%S/Inputs/ -mmcu=atmega328 -S %s 2>&1 | FileCheck --check-prefix=NOWARN %s
 // NOWARN-NOT: warning:
 
 // RUN: %clang -### --target=avr --sysroot=%S/Inputs/basic_avr_tree -S %s 2>&1 | FileCheck --check-prefixes=NOMCU,LINKA %s
 // RUN: %clang -### --target=avr --sysroot=%S/Inputs/ -S %s 2>&1 | FileCheck --check-prefixes=NOMCU,LINKA %s
 // RUN: %clang -### --target=avr --sysroot=%S/Inputs/basic_avr_tree %s 2>&1 | FileCheck --check-prefixes=NOMCU,LINKB %s
 // NOMCU: warning: no target microcontroller specified on command line, cannot link standard libraries, please pass -mmcu=<mcu name>
 // LINKB: warning: standard library not linked and so no interrupt vector table or compiler runtime routines will be linked
-// LINKB: warning: support for passing the data section address to the linker for microcontroller '' is not implemented
 // NOMCU-NOT: warning: {{.*}} avr-gcc
 // NOMCU-NOT: warning: {{.*}} avr-libc
 // LINKA-NOT: warning: {{.*}} interrupt vector
 // LINKA-NOT: warning: {{.*}} data section address
 
 // RUN: %clang -### --target=avr --sysroot=%S/Inputs/ -mmcu=atmega328 %s 2>&1 | FileCheck --check-prefixes=NOGCC %s
 // NOGCC: warning: standard library not linked and so no interrupt vector table or compiler runtime routines will be linked
 // NOGCC-NOT: warning: {{.*}} microcontroller