[clang][driver] Use $ prefix with config file options to have them added after all of the command line options #117573

pawosm-arm · 2024-11-25T16:20:20Z

Currently, if a -l (or -Wl,) flag is added into a config file
(e.g. clang.cfg), it is situated before any object file in the
effective command line. If the library requested by given -l flag is
static, its symbols will not be made visible to any of the object
files provided by the user. Also, the presence of any of the linker
flags in a config file confuses the driver whenever the user invokes
clang without any parameters (see issue #67209).

This patch attempts to solve both of the problems, by allowing a split
of the arguments list into two parts. The head part of the list will
be used as before, but the tail part will be appended after the
command line flags provided by the user and only when it is known
that the linking should occur. The $-prefixed arguments will be added
to the tail part.

llvmbot · 2024-11-25T16:20:55Z

@llvm/pr-subscribers-flang-driver

@llvm/pr-subscribers-clang

Author: Paul Osmialowski (pawosm-arm)

Changes

Currently, if a -l flag is added into a config file (e.g. clang.cfg), it is situated before any object file in the effective command line. If the library requested by given -l flag is static, its symbols will not be made visible to any of the object files provided by the user. Also, the presence of any of the -l flags in a config file confuses the driver whenever the user invokes clang without any parameters.

This patch solves both of those problems, by moving the -lm flags specified in a config file to the end of the input list, and by discarding them when there are no other inputs provided by the user, or the last phase is not the linking phase.

Full diff: https://github.com/llvm/llvm-project/pull/117573.diff

8 Files Affected:

(modified) clang/include/clang/Driver/Driver.h (+3)
(modified) clang/lib/Driver/Driver.cpp (+32)
(added) clang/test/Driver/Inputs/config-l-openmp.cfg (+1)
(added) clang/test/Driver/Inputs/config-l.cfg (+1)
(modified) clang/test/Driver/config-file.c (+19)
(added) flang/test/Driver/Inputs/config-l-openmp.cfg (+1)
(added) flang/test/Driver/Inputs/config-l.cfg (+1)
(modified) flang/test/Driver/config-file.f90 (+19)

diff --git a/clang/include/clang/Driver/Driver.h b/clang/include/clang/Driver/Driver.h
index 9177d56718ee77..b21606b6f54b77 100644
--- a/clang/include/clang/Driver/Driver.h
+++ b/clang/include/clang/Driver/Driver.h
@@ -297,6 +297,9 @@ class Driver {
   /// Object that stores strings read from configuration file.
   llvm::StringSaver Saver;
 
+  /// Linker inputs originated from configuration file.
+  std::unique_ptr<llvm::opt::InputArgList> CfgLinkerInputs;
+
   /// Arguments originated from configuration file.
   std::unique_ptr<llvm::opt::InputArgList> CfgOptions;
 
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index ad14b5c9b6dc80..bfd440461ea601 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -1062,6 +1062,15 @@ bool Driver::readConfigFile(StringRef FileName,
   for (Arg *A : *NewOptions)
     A->claim();
 
+  std::unique_ptr<InputArgList> NewLinkerIns = std::make_unique<InputArgList>();
+  for (Arg *A : NewOptions->filtered(options::OPT_l)) {
+    const Arg *BaseArg = &A->getBaseArg();
+    if (BaseArg == A)
+      BaseArg = nullptr;
+    appendOneArg(*NewLinkerIns, A, BaseArg);
+  }
+  NewOptions->eraseArg(options::OPT_l);
+
   if (!CfgOptions)
     CfgOptions = std::move(NewOptions);
   else {
@@ -1073,6 +1082,19 @@ bool Driver::readConfigFile(StringRef FileName,
       appendOneArg(*CfgOptions, Opt, BaseArg);
     }
   }
+
+  if (!CfgLinkerInputs)
+    CfgLinkerInputs = std::move(NewLinkerIns);
+  else {
+    // If this is a subsequent config file, append options to the previous one.
+    for (auto *Opt : *NewLinkerIns) {
+      const Arg *BaseArg = &Opt->getBaseArg();
+      if (BaseArg == Opt)
+        BaseArg = nullptr;
+      appendOneArg(*CfgLinkerInputs, Opt, BaseArg);
+    }
+  }
+
   ConfigFiles.push_back(std::string(CfgFileName));
   return false;
 }
@@ -1250,6 +1272,7 @@ Compilation *Driver::BuildCompilation(ArrayRef<const char *> ArgList) {
   if (!ContainsError)
     ContainsError = loadConfigFiles();
   bool HasConfigFile = !ContainsError && (CfgOptions.get() != nullptr);
+  bool HasConfigLinkerIn = !ContainsError && (CfgLinkerInputs.get() != nullptr);
 
   // All arguments, from both config file and command line.
   InputArgList Args = std::move(HasConfigFile ? std::move(*CfgOptions)
@@ -1552,6 +1575,15 @@ Compilation *Driver::BuildCompilation(ArrayRef<const char *> ArgList) {
   // Construct the list of inputs.
   InputList Inputs;
   BuildInputs(C->getDefaultToolChain(), *TranslatedArgs, Inputs);
+  if (HasConfigLinkerIn && Inputs.size()) {
+    Arg *FinalPhaseArg;
+    if (getFinalPhase(*TranslatedArgs, &FinalPhaseArg) == phases::Link) {
+      DerivedArgList TranslatedLinkerIns(*CfgLinkerInputs);
+      for (Arg *A : *CfgLinkerInputs)
+        TranslatedLinkerIns.append(A);
+      BuildInputs(C->getDefaultToolChain(), TranslatedLinkerIns, Inputs);
+    }
+  }
 
   // Populate the tool chains for the offloading devices, if any.
   CreateOffloadingDeviceToolChains(*C, Inputs);
diff --git a/clang/test/Driver/Inputs/config-l-openmp.cfg b/clang/test/Driver/Inputs/config-l-openmp.cfg
new file mode 100644
index 00000000000000..91f8884b6e9e23
--- /dev/null
+++ b/clang/test/Driver/Inputs/config-l-openmp.cfg
@@ -0,0 +1 @@
+-Wall -fopenmp -lm -lhappy
diff --git a/clang/test/Driver/Inputs/config-l.cfg b/clang/test/Driver/Inputs/config-l.cfg
new file mode 100644
index 00000000000000..0ebc28baddbe2c
--- /dev/null
+++ b/clang/test/Driver/Inputs/config-l.cfg
@@ -0,0 +1 @@
+-Wall -lm -lhappy
diff --git a/clang/test/Driver/config-file.c b/clang/test/Driver/config-file.c
index 9df830ca4c7538..799e545e1e518b 100644
--- a/clang/test/Driver/config-file.c
+++ b/clang/test/Driver/config-file.c
@@ -82,3 +82,22 @@
 // CHECK-TWO-CONFIGS: -isysroot
 // CHECK-TWO-CONFIGS-SAME: /opt/data
 // CHECK-TWO-CONFIGS-SAME: -Wall
+
+//--- The -l flags should be moved to the end of input list and appear only when linking.
+// RUN: %clang --config %S/Inputs/config-l.cfg -o %s.out %s -### 2>&1 | FileCheck %s -check-prefix CHECK-LINKING
+// RUN: %clang --config %S/Inputs/config-l-openmp.cfg -o %s.out %s -### 2>&1 | FileCheck %s -check-prefix CHECK-LINKING-LIBOMP-GOES-LAST
+// RUN: %clang --config %S/Inputs/config-l.cfg -S %s -### 2>&1 | FileCheck %s -check-prefix CHECK-NOLINKING
+// RUN: %clang --config %S/Inputs/config-l-openmp.cfg -S %s -### 2>&1 | FileCheck %s -check-prefix CHECK-NOLINKING
+// CHECK-LINKING: Configuration file: {{.*}}Inputs{{.}}config-l.cfg
+// CHECK-LINKING: "-Wall"
+// CHECK-LINKING: "/tmp/{{.*}}-{{.*}}.o" "-lm" "-lhappy"
+// CHECK-LINKING-LIBOMP-GOES-LAST: Configuration file: {{.*}}Inputs{{.}}config-l-openmp.cfg
+// CHECK-LINKING-LIBOMP-GOES-LAST: "-Wall"
+// CHECK-LINKING-LIBOMP-GOES-LAST-SAME: "-fopenmp"
+// CHECK-LINKING-LIBOMP-GOES-LAST: "/tmp/{{.*}}-{{.*}}.o" "-lm" "-lhappy"
+// CHECK-LINKING-LIBOMP-GOES-LAST-SAME: "-lomp"
+// CHECK-NOLINKING: Configuration file: {{.*}}Inputs{{.}}config-l{{.*}}.cfg
+// CHECK-NOLINKING: "-Wall"
+// CHECK-NOLINKING-NO: "-lm"
+// CHECK-NOLINKING-NO: "-lhappy"
+// CHECK-NOLINKING-NO: "-lomp"
diff --git a/flang/test/Driver/Inputs/config-l-openmp.cfg b/flang/test/Driver/Inputs/config-l-openmp.cfg
new file mode 100644
index 00000000000000..46ab3273f84f19
--- /dev/null
+++ b/flang/test/Driver/Inputs/config-l-openmp.cfg
@@ -0,0 +1 @@
+-ffast-math -fopenmp -lm -lhappy
diff --git a/flang/test/Driver/Inputs/config-l.cfg b/flang/test/Driver/Inputs/config-l.cfg
new file mode 100644
index 00000000000000..af2d0a99475a2a
--- /dev/null
+++ b/flang/test/Driver/Inputs/config-l.cfg
@@ -0,0 +1 @@
+-ffast-math -lm -lhappy
diff --git a/flang/test/Driver/config-file.f90 b/flang/test/Driver/config-file.f90
index 70316dd971f36b..15b18f4420e6b3 100644
--- a/flang/test/Driver/config-file.f90
+++ b/flang/test/Driver/config-file.f90
@@ -61,3 +61,22 @@
 ! CHECK-TWO-CONFIGS-NEXT: Configuration file: {{.*}}Inputs{{.}}config2{{.}}config-4.cfg
 ! CHECK-TWO-CONFIGS: -ffp-contract=fast
 ! CHECK-TWO-CONFIGS: -O3
+
+!--- The -l flags should be moved to the end of input list and appear only when linking.
+! RUN: %flang --config %S/Inputs/config-l.cfg -o %s.out %s -### 2>&1 | FileCheck %s -check-prefix CHECK-LINKING
+! RUN: %flang --config %S/Inputs/config-l-openmp.cfg -o %s.out %s -### 2>&1 | FileCheck %s -check-prefix CHECK-LINKING-LIBOMP-GOES-LAST
+! RUN: %flang --config %S/Inputs/config-l.cfg -S %s -### 2>&1 | FileCheck %s -check-prefix CHECK-NOLINKING
+! RUN: %flang --config %S/Inputs/config-l-openmp.cfg -S %s -### 2>&1 | FileCheck %s -check-prefix CHECK-NOLINKING
+! CHECK-LINKING: Configuration file: {{.*}}Inputs{{.}}config-l.cfg
+! CHECK-LINKING: "-ffast-math"
+! CHECK-LINKING: "/tmp/{{.*}}-{{.*}}.o" "-lm" "-lhappy"
+! CHECK-LINKING-LIBOMP-GOES-LAST: Configuration file: {{.*}}Inputs{{.}}config-l-openmp.cfg
+! CHECK-LINKING-LIBOMP-GOES-LAST: "-ffast-math"
+! CHECK-LINKING-LIBOMP-GOES-LAST-SAME: "-fopenmp"
+! CHECK-LINKING-LIBOMP-GOES-LAST: "/tmp/{{.*}}-{{.*}}.o" "-lm" "-lhappy"
+! CHECK-LINKING-LIBOMP-GOES-LAST-SAME: "-lomp"
+! CHECK-NOLINKING: Configuration file: {{.*}}Inputs{{.}}config-l{{.*}}.cfg
+! CHECK-NOLINKING: "-ffast-math"
+! CHECK-NOLINKING-NO: "-lm"
+! CHECK-NOLINKING-NO: "-lhappy"
+! CHECK-NOLINKING-NO: "-lomp"

llvmbot · 2024-11-25T16:20:56Z

@llvm/pr-subscribers-clang-driver

Author: Paul Osmialowski (pawosm-arm)

Changes

Currently, if a -l flag is added into a config file (e.g. clang.cfg), it is situated before any object file in the effective command line. If the library requested by given -l flag is static, its symbols will not be made visible to any of the object files provided by the user. Also, the presence of any of the -l flags in a config file confuses the driver whenever the user invokes clang without any parameters.

This patch solves both of those problems, by moving the -lm flags specified in a config file to the end of the input list, and by discarding them when there are no other inputs provided by the user, or the last phase is not the linking phase.

Full diff: https://github.com/llvm/llvm-project/pull/117573.diff

8 Files Affected:

(modified) clang/include/clang/Driver/Driver.h (+3)
(modified) clang/lib/Driver/Driver.cpp (+32)
(added) clang/test/Driver/Inputs/config-l-openmp.cfg (+1)
(added) clang/test/Driver/Inputs/config-l.cfg (+1)
(modified) clang/test/Driver/config-file.c (+19)
(added) flang/test/Driver/Inputs/config-l-openmp.cfg (+1)
(added) flang/test/Driver/Inputs/config-l.cfg (+1)
(modified) flang/test/Driver/config-file.f90 (+19)

diff --git a/clang/include/clang/Driver/Driver.h b/clang/include/clang/Driver/Driver.h
index 9177d56718ee77..b21606b6f54b77 100644
--- a/clang/include/clang/Driver/Driver.h
+++ b/clang/include/clang/Driver/Driver.h
@@ -297,6 +297,9 @@ class Driver {
   /// Object that stores strings read from configuration file.
   llvm::StringSaver Saver;
 
+  /// Linker inputs originated from configuration file.
+  std::unique_ptr<llvm::opt::InputArgList> CfgLinkerInputs;
+
   /// Arguments originated from configuration file.
   std::unique_ptr<llvm::opt::InputArgList> CfgOptions;
 
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index ad14b5c9b6dc80..bfd440461ea601 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -1062,6 +1062,15 @@ bool Driver::readConfigFile(StringRef FileName,
   for (Arg *A : *NewOptions)
     A->claim();
 
+  std::unique_ptr<InputArgList> NewLinkerIns = std::make_unique<InputArgList>();
+  for (Arg *A : NewOptions->filtered(options::OPT_l)) {
+    const Arg *BaseArg = &A->getBaseArg();
+    if (BaseArg == A)
+      BaseArg = nullptr;
+    appendOneArg(*NewLinkerIns, A, BaseArg);
+  }
+  NewOptions->eraseArg(options::OPT_l);
+
   if (!CfgOptions)
     CfgOptions = std::move(NewOptions);
   else {
@@ -1073,6 +1082,19 @@ bool Driver::readConfigFile(StringRef FileName,
       appendOneArg(*CfgOptions, Opt, BaseArg);
     }
   }
+
+  if (!CfgLinkerInputs)
+    CfgLinkerInputs = std::move(NewLinkerIns);
+  else {
+    // If this is a subsequent config file, append options to the previous one.
+    for (auto *Opt : *NewLinkerIns) {
+      const Arg *BaseArg = &Opt->getBaseArg();
+      if (BaseArg == Opt)
+        BaseArg = nullptr;
+      appendOneArg(*CfgLinkerInputs, Opt, BaseArg);
+    }
+  }
+
   ConfigFiles.push_back(std::string(CfgFileName));
   return false;
 }
@@ -1250,6 +1272,7 @@ Compilation *Driver::BuildCompilation(ArrayRef<const char *> ArgList) {
   if (!ContainsError)
     ContainsError = loadConfigFiles();
   bool HasConfigFile = !ContainsError && (CfgOptions.get() != nullptr);
+  bool HasConfigLinkerIn = !ContainsError && (CfgLinkerInputs.get() != nullptr);
 
   // All arguments, from both config file and command line.
   InputArgList Args = std::move(HasConfigFile ? std::move(*CfgOptions)
@@ -1552,6 +1575,15 @@ Compilation *Driver::BuildCompilation(ArrayRef<const char *> ArgList) {
   // Construct the list of inputs.
   InputList Inputs;
   BuildInputs(C->getDefaultToolChain(), *TranslatedArgs, Inputs);
+  if (HasConfigLinkerIn && Inputs.size()) {
+    Arg *FinalPhaseArg;
+    if (getFinalPhase(*TranslatedArgs, &FinalPhaseArg) == phases::Link) {
+      DerivedArgList TranslatedLinkerIns(*CfgLinkerInputs);
+      for (Arg *A : *CfgLinkerInputs)
+        TranslatedLinkerIns.append(A);
+      BuildInputs(C->getDefaultToolChain(), TranslatedLinkerIns, Inputs);
+    }
+  }
 
   // Populate the tool chains for the offloading devices, if any.
   CreateOffloadingDeviceToolChains(*C, Inputs);
diff --git a/clang/test/Driver/Inputs/config-l-openmp.cfg b/clang/test/Driver/Inputs/config-l-openmp.cfg
new file mode 100644
index 00000000000000..91f8884b6e9e23
--- /dev/null
+++ b/clang/test/Driver/Inputs/config-l-openmp.cfg
@@ -0,0 +1 @@
+-Wall -fopenmp -lm -lhappy
diff --git a/clang/test/Driver/Inputs/config-l.cfg b/clang/test/Driver/Inputs/config-l.cfg
new file mode 100644
index 00000000000000..0ebc28baddbe2c
--- /dev/null
+++ b/clang/test/Driver/Inputs/config-l.cfg
@@ -0,0 +1 @@
+-Wall -lm -lhappy
diff --git a/clang/test/Driver/config-file.c b/clang/test/Driver/config-file.c
index 9df830ca4c7538..799e545e1e518b 100644
--- a/clang/test/Driver/config-file.c
+++ b/clang/test/Driver/config-file.c
@@ -82,3 +82,22 @@
 // CHECK-TWO-CONFIGS: -isysroot
 // CHECK-TWO-CONFIGS-SAME: /opt/data
 // CHECK-TWO-CONFIGS-SAME: -Wall
+
+//--- The -l flags should be moved to the end of input list and appear only when linking.
+// RUN: %clang --config %S/Inputs/config-l.cfg -o %s.out %s -### 2>&1 | FileCheck %s -check-prefix CHECK-LINKING
+// RUN: %clang --config %S/Inputs/config-l-openmp.cfg -o %s.out %s -### 2>&1 | FileCheck %s -check-prefix CHECK-LINKING-LIBOMP-GOES-LAST
+// RUN: %clang --config %S/Inputs/config-l.cfg -S %s -### 2>&1 | FileCheck %s -check-prefix CHECK-NOLINKING
+// RUN: %clang --config %S/Inputs/config-l-openmp.cfg -S %s -### 2>&1 | FileCheck %s -check-prefix CHECK-NOLINKING
+// CHECK-LINKING: Configuration file: {{.*}}Inputs{{.}}config-l.cfg
+// CHECK-LINKING: "-Wall"
+// CHECK-LINKING: "/tmp/{{.*}}-{{.*}}.o" "-lm" "-lhappy"
+// CHECK-LINKING-LIBOMP-GOES-LAST: Configuration file: {{.*}}Inputs{{.}}config-l-openmp.cfg
+// CHECK-LINKING-LIBOMP-GOES-LAST: "-Wall"
+// CHECK-LINKING-LIBOMP-GOES-LAST-SAME: "-fopenmp"
+// CHECK-LINKING-LIBOMP-GOES-LAST: "/tmp/{{.*}}-{{.*}}.o" "-lm" "-lhappy"
+// CHECK-LINKING-LIBOMP-GOES-LAST-SAME: "-lomp"
+// CHECK-NOLINKING: Configuration file: {{.*}}Inputs{{.}}config-l{{.*}}.cfg
+// CHECK-NOLINKING: "-Wall"
+// CHECK-NOLINKING-NO: "-lm"
+// CHECK-NOLINKING-NO: "-lhappy"
+// CHECK-NOLINKING-NO: "-lomp"
diff --git a/flang/test/Driver/Inputs/config-l-openmp.cfg b/flang/test/Driver/Inputs/config-l-openmp.cfg
new file mode 100644
index 00000000000000..46ab3273f84f19
--- /dev/null
+++ b/flang/test/Driver/Inputs/config-l-openmp.cfg
@@ -0,0 +1 @@
+-ffast-math -fopenmp -lm -lhappy
diff --git a/flang/test/Driver/Inputs/config-l.cfg b/flang/test/Driver/Inputs/config-l.cfg
new file mode 100644
index 00000000000000..af2d0a99475a2a
--- /dev/null
+++ b/flang/test/Driver/Inputs/config-l.cfg
@@ -0,0 +1 @@
+-ffast-math -lm -lhappy
diff --git a/flang/test/Driver/config-file.f90 b/flang/test/Driver/config-file.f90
index 70316dd971f36b..15b18f4420e6b3 100644
--- a/flang/test/Driver/config-file.f90
+++ b/flang/test/Driver/config-file.f90
@@ -61,3 +61,22 @@
 ! CHECK-TWO-CONFIGS-NEXT: Configuration file: {{.*}}Inputs{{.}}config2{{.}}config-4.cfg
 ! CHECK-TWO-CONFIGS: -ffp-contract=fast
 ! CHECK-TWO-CONFIGS: -O3
+
+!--- The -l flags should be moved to the end of input list and appear only when linking.
+! RUN: %flang --config %S/Inputs/config-l.cfg -o %s.out %s -### 2>&1 | FileCheck %s -check-prefix CHECK-LINKING
+! RUN: %flang --config %S/Inputs/config-l-openmp.cfg -o %s.out %s -### 2>&1 | FileCheck %s -check-prefix CHECK-LINKING-LIBOMP-GOES-LAST
+! RUN: %flang --config %S/Inputs/config-l.cfg -S %s -### 2>&1 | FileCheck %s -check-prefix CHECK-NOLINKING
+! RUN: %flang --config %S/Inputs/config-l-openmp.cfg -S %s -### 2>&1 | FileCheck %s -check-prefix CHECK-NOLINKING
+! CHECK-LINKING: Configuration file: {{.*}}Inputs{{.}}config-l.cfg
+! CHECK-LINKING: "-ffast-math"
+! CHECK-LINKING: "/tmp/{{.*}}-{{.*}}.o" "-lm" "-lhappy"
+! CHECK-LINKING-LIBOMP-GOES-LAST: Configuration file: {{.*}}Inputs{{.}}config-l-openmp.cfg
+! CHECK-LINKING-LIBOMP-GOES-LAST: "-ffast-math"
+! CHECK-LINKING-LIBOMP-GOES-LAST-SAME: "-fopenmp"
+! CHECK-LINKING-LIBOMP-GOES-LAST: "/tmp/{{.*}}-{{.*}}.o" "-lm" "-lhappy"
+! CHECK-LINKING-LIBOMP-GOES-LAST-SAME: "-lomp"
+! CHECK-NOLINKING: Configuration file: {{.*}}Inputs{{.}}config-l{{.*}}.cfg
+! CHECK-NOLINKING: "-ffast-math"
+! CHECK-NOLINKING-NO: "-lm"
+! CHECK-NOLINKING-NO: "-lhappy"
+! CHECK-NOLINKING-NO: "-lomp"

flang/test/Driver/Inputs/config-l.cfg

DavidTruby

I think it makes sense to handle linker options differently so I'm in favour of this change in principle.

Am I right in thinking that if the config file puts things last, the -l options provided by users will come before the config file ones, and unlike other options that will lead to those libraries being chosen first? If so, I think that's the correct way to do things anyway, so I prefer that to the current approach of putting the ones from the config file first. It might be considered a breaking change though.

@mgorny's question has got me thinking and given me concerns; I think if the user passes -Wl,-Bstatic -lmystaticlib and the config file is adding -lmydynamiclib after that, things will fail, because the -Bstatic will also apply to the lib in the config file. So we need to do something to prevent situations like that.

I've also added a few comments so that we can hopefully get this working for Windows as well.

flang/test/Driver/config-file.f90

pawosm-arm · 2024-11-26T00:16:37Z

I think it makes sense to handle linker options differently so I'm in favour of this change in principle.

Am I right in thinking that if the config file puts things last, the -l options provided by users will come before the config file ones, and unlike other options that will lead to those libraries being chosen first? If so, I think that's the correct way to do things anyway, so I prefer that to the current approach of putting the ones from the config file first. It might be considered a breaking change though.

@mgorny's question has got me thinking and given me concerns; I think if the user passes -Wl,-Bstatic -lmystaticlib and the config file is adding -lmydynamiclib after that, things will fail, because the -Bstatic will also apply to the lib in the config file. So we need to do something to prevent situations like that.

I've also added a few comments so that we can hopefully get this working for Windows as well.

That was actually easy to fix.

DavidTruby

Change LGTM in general but I'd like Windows to have tests before approving as we need this to be working there too

flang/test/Driver/config-file.f90

clang/lib/Driver/Driver.cpp

clang/test/Driver/config-file.c

DavidTruby

LGTM. Would be good if someone from the clang side of things can approve as well, as this is a fairly big functionality change for config files.

mgorny

I don't see anything wrong but I don't consider myself an expert either.

MaskRay · 2024-11-30T03:14:36Z

clang/test/Driver/config-file.c

@@ -82,3 +82,35 @@
 // CHECK-TWO-CONFIGS: -isysroot
 // CHECK-TWO-CONFIGS-SAME: /opt/data
 // CHECK-TWO-CONFIGS-SAME: -Wall
+
+//--- The linker input flags should be moved to the end of input list and appear only when linking.
+// RUN: %clang --target=aarch64-unknown-linux-gnu --config %S/Inputs/config-l.cfg -o %s.out %s -### 2>&1 | FileCheck %s -check-prefix CHECK-LINKING


-o %s.out is strange. We typically use -o %t for output. That said, since we use -### and the output isn't useful, just omit it.

Add something like -lxxx and -Wl,foo.a to test the position of config specified -l -Wl,`

Thanks for all the comments.
Added both things, and removed all -o's

MaskRay · 2024-11-30T03:16:01Z

clang/test/Driver/config-file.c

+// CHECK-LINKING-LIBOMP-GOES-LAST-SAME: "-lomp"
+// CHECK-NOLINKING: Configuration file: {{.*}}Inputs{{.}}config-l.cfg
+// CHECK-NOLINKING: "-Wall"
+// CHECK-NOLINKING-NO: "-lm"


When the linker options are known be adjacent, place them on the same CHECK line.

MaskRay · 2024-11-30T03:16:49Z

clang/lib/Driver/Driver.cpp

@@ -1062,6 +1062,16 @@ bool Driver::readConfigFile(StringRef FileName,
  for (Arg *A : *NewOptions)
    A->claim();

+  std::unique_ptr<InputArgList> NewLinkerIns = std::make_unique<InputArgList>();


auto since RHS makes the type clear.

This block of code needs a comment.

changed and added a comment

MaskRay

.

MaskRay · 2024-11-30T18:35:34Z

clang/lib/Driver/Driver.cpp

+  for (Arg *A : NewOptions->filtered(options::OPT_Wl_COMMA, options::OPT_l)) {
+    const Arg *BaseArg = &A->getBaseArg();
+    if (BaseArg == A)
+      BaseArg = nullptr;


getBaseArg() is always nullptr. I've cleaned up the existing code using it. Please rebase

MaskRay · 2024-11-30T18:36:30Z

flang/test/Driver/config-file.f90

+! CHECK-LINKING: "-ffast-math"
+! CHECK-LINKING: "{{.*}}-{{.*}}.o" "-lmylib" "foo.a" "-lm" "-Bstatic" "-lhappy" "-Bdynamic"
+! CHECK-LINKING-LIBOMP-GOES-LAST: Configuration file: {{.*}}Inputs{{.}}config-l.cfg
+! CHECK-LINKING-LIBOMP-GOES-LAST: "-ffast-math"


If two options are adjacent, place them on the same SAME line. Otherwise, if they are on the same line in the output, use -SAME:

I need some help here.

Sorry, perhaps not here. But use -SAME: for some lines below, e.g. ! CHECK-NOLINKING-NO: "-lomp"

Adding "-fopenmp" may result in: "-latomic" "-lm" "-lomp"

Seems I had wrong idea of how "SAME:" works. I'll make adjustments you're suggesting here.

removed all SAME:

MaskRay · 2024-11-30T18:36:51Z

clang/test/Driver/config-file.c

+// CHECK-LINKING: "-Wall"
+// CHECK-LINKING: "{{.*}}-{{.*}}.o" "-lmylib" "foo.a" "-lm" "-Bstatic" "-lhappy" "-Bdynamic"
+// CHECK-LINKING-LIBOMP-GOES-LAST: Configuration file: {{.*}}Inputs{{.}}config-l.cfg
+// CHECK-LINKING-LIBOMP-GOES-LAST: "-Wall"


If two options are adjacent, place them on the same SAME line. Otherwise, if they are on the same line in the output, use -SAME:

the changes have been made

MaskRay · 2024-11-30T18:46:55Z

clang/lib/Driver/Driver.cpp

+  else {
+    // If this is a subsequent config file, append options to the previous one.
+    for (auto *Opt : *NewLinkerIns) {
+      const Arg *BaseArg = &Opt->getBaseArg();


Remove getBaseArg

pawosm-arm · 2024-12-04T15:01:43Z

This LGTM but I think the other reviewers need to take a look too. Can you change the title and description of the PR/commit to represent that what we're doing is different now?

The commit title and message has been changed already, the PR description comment too, I've updated the PR title.

thesamesam · 2024-12-04T15:13:33Z

This looks good to me but no doc updates?

pawosm-arm · 2024-12-04T15:24:23Z

This looks good to me but no doc updates?

Would a paragraph in clang/docs/UsersManual.rst "Configuration files" section do? If so, I'd write one.

pawosm-arm · 2024-12-04T16:28:07Z

This looks good to me but no doc updates?

Added.

MaskRay · 2024-12-05T05:55:27Z

It's worth spending more time discussing the metacharacter.

^ can be interpreted as ^ in regex, which means the beginning. $, on the other side, suggests the end.
While I agree that @ for response files is not a good choice, I personally would not rule out $ just because shell/makefile variable substitution uses it.

In CCC_OVERRIDE_OPTIONS (clang/lib/Driver/Driver.cpp), ^ is to add an option at the beginning, but there is no $. There is +, whose meaning is slightly different.

MaskRay · 2024-12-05T05:58:06Z

clang/include/clang/Driver/Driver.h

@@ -297,6 +297,9 @@ class Driver {
  /// Object that stores strings read from configuration file.
  llvm::StringSaver Saver;

+  /// Linker inputs originated from configuration file.
+  std::unique_ptr<llvm::opt::InputArgList> CfgLinkerInputs;


CfgLinkerInputs is not a good name as the implementation is generic, not coupled with linker options.

pawosm-arm · 2024-12-05T13:10:56Z

It's worth spending more time discussing the metacharacter.

^ can be interpreted as ^ in regex, which means the beginning. $, on the other side, suggests the end. While I agree that @ for response files is not a good choice, I personally would not rule out $ just because shell/makefile variable substitution uses it.

In CCC_OVERRIDE_OPTIONS (clang/lib/Driver/Driver.cpp), ^ is to add an option at the beginning, but there is no $. There is +, whose meaning is slightly different.

I have no spare time for discussing it, so I turned it to $

clang/test/Driver/config-file.c

MaskRay · 2024-12-06T17:55:02Z

clang/lib/Driver/Driver.cpp


  // All arguments, from both config file and command line.
-  InputArgList Args = std::move(HasConfigFile ? std::move(*CfgOptions)
-                                              : std::move(*CLOptions));
+  InputArgList Args = std::move(HasConfigFileHead ? std::move(*CfgOptionsHead)


the outer std::move is unneeded

MaskRay

The subject (linker flags) is no longer accurate.

It's better to mention that Add $ for ....

pawosm-arm · 2024-12-06T22:18:52Z

The subject (linker flags) is no longer accurate.

It's better to mention that Add $ for ....

Reworded

github-actions · 2024-12-06T22:20:54Z

✅ With the latest revision this PR passed the C/C++ code formatter.

…ded after all of the command line options Currently, if a -l (or -Wl,) flag is added into a config file (e.g. clang.cfg), it is situated before any object file in the effective command line. If the library requested by given -l flag is static, its symbols will not be made visible to any of the object files provided by the user. Also, the presence of any of the linker flags in a config file confuses the driver whenever the user invokes clang without any parameters (see issue llvm#67209). This patch attempts to solve both of the problems, by allowing a split of the arguments list into two parts. The head part of the list will be used as before, but the tail part will be appended after the command line flags provided by the user and only when it is known that the linking should occur. The $-prefixed arguments will be added to the tail part.

This was added in llvm#117573 but the options were not being rendered correctly due to the missing newline after `::`.

This was added in #117573 but the options were not being rendered correctly due to the missing newline after `::`.

pawosm-arm requested review from mgorny and DavidTruby November 25, 2024 16:20

llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' flang:driver flang Flang issues not falling into any other category labels Nov 25, 2024

mgorny requested review from MaskRay and thesamesam November 25, 2024 16:48

mgorny reviewed Nov 25, 2024

View reviewed changes

flang/test/Driver/Inputs/config-l.cfg Outdated Show resolved Hide resolved

DavidTruby requested changes Nov 25, 2024

View reviewed changes

flang/test/Driver/config-file.f90 Outdated Show resolved Hide resolved

pawosm-arm force-pushed the config-l branch from 1c4cab5 to 2acf432 Compare November 26, 2024 00:14

pawosm-arm requested a review from DavidTruby November 26, 2024 00:16

DavidTruby reviewed Nov 26, 2024

View reviewed changes

flang/test/Driver/config-file.f90 Outdated Show resolved Hide resolved

clang/lib/Driver/Driver.cpp Outdated Show resolved Hide resolved

clang/test/Driver/config-file.c Outdated Show resolved Hide resolved

pawosm-arm force-pushed the config-l branch from 2acf432 to d5a4244 Compare November 26, 2024 16:58

DavidTruby approved these changes Nov 27, 2024

View reviewed changes

mgorny approved these changes Nov 27, 2024

View reviewed changes

MaskRay reviewed Nov 30, 2024

View reviewed changes

MaskRay requested changes Nov 30, 2024

View reviewed changes

pawosm-arm force-pushed the config-l branch from d5a4244 to e5769df Compare November 30, 2024 17:26

pawosm-arm requested a review from MaskRay November 30, 2024 17:28

MaskRay reviewed Nov 30, 2024

View reviewed changes

pawosm-arm force-pushed the config-l branch from e5769df to 65917ba Compare November 30, 2024 19:45

pawosm-arm force-pushed the config-l branch from bc66202 to c450f66 Compare December 4, 2024 16:27

thesamesam approved these changes Dec 4, 2024

View reviewed changes

pawosm-arm requested a review from MaskRay December 4, 2024 23:10

MaskRay reviewed Dec 5, 2024

View reviewed changes

pawosm-arm force-pushed the config-l branch from c450f66 to 7c7649d Compare December 5, 2024 13:09

carlocab reviewed Dec 6, 2024

View reviewed changes

clang/test/Driver/config-file.c Outdated Show resolved Hide resolved

pawosm-arm force-pushed the config-l branch from 7c7649d to 6595880 Compare December 6, 2024 10:29

MaskRay reviewed Dec 6, 2024

View reviewed changes

MaskRay approved these changes Dec 6, 2024

View reviewed changes

pawosm-arm force-pushed the config-l branch from 6595880 to 537a1c6 Compare December 6, 2024 22:17

pawosm-arm changed the title ~~[clang][driver] Special care for linker flags in config files~~ [clang][driver] Use $ prefix with config file options to have them added after all of the command line options Dec 6, 2024

pawosm-arm force-pushed the config-l branch from 537a1c6 to c599850 Compare December 6, 2024 22:24

MaskRay approved these changes Dec 7, 2024

View reviewed changes

pawosm-arm merged commit 755519f into llvm:main Dec 7, 2024
9 checks passed

carlocab mentioned this pull request Dec 9, 2024

[clang][docs] fix rendering of $-prefixed options #119249

Merged

carlocab added a commit to carlocab/llvm-project that referenced this pull request Dec 9, 2024

[clang][docs] fix rendering of $-prefixed options

ed70bf3

This was added in llvm#117573 but the options were not being rendered correctly due to the missing newline after `::`.

carlocab added a commit that referenced this pull request Dec 9, 2024

[clang][docs] fix rendering of $-prefixed options (#119249)

511e84f

This was added in #117573 but the options were not being rendered correctly due to the missing newline after `::`.

Andarwinux mentioned this pull request Jan 7, 2025

status of clang config files in wrappers mstorsjo/llvm-mingw#469

Open

thesamesam mentioned this pull request Jan 29, 2025

Linker options in Clang config files break clang invocation #67209

Open

[clang][driver] Use $ prefix with config file options to have them added after all of the command line options #117573

[clang][driver] Use $ prefix with config file options to have them added after all of the command line options #117573

Uh oh!

Conversation

pawosm-arm commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Nov 25, 2024

Uh oh!

Uh oh!

DavidTruby left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pawosm-arm commented Nov 26, 2024

Uh oh!

DavidTruby left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DavidTruby left a comment

Choose a reason for hiding this comment

Uh oh!

mgorny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaskRay Nov 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pawosm-arm Nov 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaskRay Nov 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaskRay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pawosm-arm commented Nov 25, 2024 •

edited

Loading

llvmbot commented Nov 25, 2024 •

edited

Loading

MaskRay Nov 30, 2024 •

edited

Loading

pawosm-arm Nov 30, 2024 •

edited

Loading

MaskRay Nov 30, 2024 •

edited

Loading

MaskRay commented Dec 5, 2024 •

edited

Loading

github-actions bot commented Dec 6, 2024 •

edited

Loading