Re: [PATCH] Driver: add support for linker file lists

[Alp Toker]

On 12/06/2014 00:40, Saleem Abdulrasool wrote:
Hi rui314,
Add support for linker file lists.  A file list is similar to linker response
files, but slightly more restricted.  A file list may only contain a list of
input files.  Flags must remain on the actual command invocation.  Runs of files
are collapsed into a single input which is marked as a file list by prepending
the filename with an '@' symbol.  The linker will read this file and expand the
contents as the input items.

So, I had to read through the code a couple of times to take a guess 
what it was actually for.

How about starting with something like "Driver: generate input file 
lists to pass through to the linker" before providing the detailed 
description above?

I still don't have fully clarify (correct me if I misunderstood!) but 
here are some early thoughts inline...


This is supported by at least the GNU linker as well as Microsoft's link.exe
linker.  The use of the option is controlled through a driver flag, defaulting
to disabled.

This functionality aids in reducing the length of the subcommand invoked to
perform the final link.  Some systems have a limit on the command line, and when
linking a large number of object files, it is possible to overrun that limit.

http://reviews.llvm.org/D4104

Files:
   include/clang/Basic/DiagnosticDriverKinds.td
   include/clang/Driver/Options.td
   lib/Driver/Tools.cpp
   test/Driver/Inputs/filelist/a.o
   test/Driver/Inputs/filelist/a.obj
   test/Driver/Inputs/filelist/b.o
   test/Driver/Inputs/filelist/c.a
   test/Driver/filelist.c

D4104.10334.patch


Index: include/clang/Basic/DiagnosticDriverKinds.td
===================================================================
--- include/clang/Basic/DiagnosticDriverKinds.td
+++ include/clang/Basic/DiagnosticDriverKinds.td
@@ -39,6 +39,7 @@
    "cannot specify '%0%1' when compiling multiple source files">;
  def err_no_external_assembler : Error<
    "there is no external assembler that can be used on this platform">;
+def err_drv_unable_to_open_file : Error< "unable to open file: %0">;

'open', though technically correct, is pretty confusing when the problem 
is related to a file we're creating to write to. It gives the wrong 
impression the user was somehow expected to provide the file as an 
input, when in fact it's a kind of temporary intermediate output, right?

  def err_drv_unable_to_remove_file : Error<
    "unable to remove file: %0">;
  def err_drv_command_failure : Error<
Index: include/clang/Driver/Options.td
===================================================================
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1384,6 +1384,8 @@
  def : Flag<["-"], "no-integrated-as">, Alias<fno_integrated_as>,
        Flags<[CC1Option, DriverOption]>;
  
+def fuse_linker_response_files : Joined<["-"], "fuse-linker-response-files">;
+

To be clear, GCC does not have this option so it would be a 
clang-specific extension to the driver. If we add such a thing, it 
should be tucked away/hidden with a note.

However a smarter approach might be to generate the response file 
automatically, and only if the list exceeds some known OS limitation? It 
feels like an implementation detail we can hide from the GCC-compatible 
driver.


  def working_directory : JoinedOrSeparate<["-"], "working-directory">, 
Flags<[CC1Option]>,
    HelpText<"Resolve file paths relative to the specified directory">;
  def working_directory_EQ : Joined<["-"], "working-directory=">, 
Flags<[CC1Option]>,
Index: lib/Driver/Tools.cpp
===================================================================
--- lib/Driver/Tools.cpp
+++ lib/Driver/Tools.cpp
@@ -39,6 +39,7 @@
  #include "llvm/Support/Program.h"
  #include "llvm/Support/raw_ostream.h"
  #include <sys/stat.h>
+#include <fstream>
  
  using namespace clang::driver;
  using namespace clang::driver::tools;
@@ -164,11 +165,36 @@
    }
  }
  
-static void AddLinkerInputs(const ToolChain &TC,
+static StringRef GetFileListName(const ArgList &Args,
+                                 const InputInfoList &Inputs, int Sequence) {
+  std::string Buffer;
+  llvm::raw_string_ostream Name(Buffer);
+
+  // Add the prefix so as to ensure that it is made part of the argument string
+  Name << "@";
+
+  if (Arg *Output = Args.getLastArg(options::OPT_o)) {
+    StringRef Value = Output->getValue();
+    Name << Value.substr(0, Value.rfind('.'));

Avoid duplication with the '.' search in getBaseInputStem(). Factor them 
together if needed instead of if/elsing.

+  } else {
+    Name << Clang::getBaseInputStem(Args, Inputs);
+  }
+
+  Name << Sequence << ".rsp";
+  return Args.MakeArgString(Name.str());
+}
+
+static void AddLinkerInputs(Compilation &C, const ToolChain &TC,
                              const InputInfoList &Inputs, const ArgList &Args,
                              ArgStringList &CmdArgs) {
+  const bool UseLinkerResponseFiles =
+    Args.hasArg(options::OPT_fuse_linker_response_files);
    const Driver &D = TC.getDriver();
  
+  std::ofstream FileList;
+  StringRef FileListName;
+  int FileListSequence = 0;
+
    // Add extra linker input arguments which are not treated as inputs
    // (constructed via -Xarch_).
    Args.AddAllArgValues(CmdArgs, options::OPT_Zlinker_input);
@@ -186,10 +212,33 @@
  
      // Add filenames immediately.
      if (II.isFilename()) {
-      CmdArgs.push_back(II.getFilename());
+      if (UseLinkerResponseFiles && II.getType() == types::TY_Object) {
+        if (!FileList.is_open()) {
+          FileListName = GetFileListName(Args, Inputs, FileListSequence++);

Again, I might not have fully understood your change, but why not just 
create a temporary file at a path that's been provided to us for such use?

+
+          // Skip the leading '@'
+          FileList.open(FileListName.substr(1).data(),

drop_front()? If you keep this approach, I suggest an assert() that 
front() == '@' as well.

The use of data() doesn't feel comfortable here without an obvious 
assurance that the string is null-terminated.

It's safer to pass around a const char* if that's that's your requirement.


+                        std::ios_base::trunc | std::ios_base::out);
+          if (!FileList.is_open())
+            D.Diag(diag::err_drv_unable_to_open_file) << FileListName;
+          C.addTempFile(FileListName.data());
+        }
+        FileList << II.getFilename() << std::endl;
+      } else {
+        if (FileList.is_open()) {
+          FileList.close();
+          CmdArgs.push_back(FileListName.data());
+        }
+        CmdArgs.push_back(II.getFilename());
+      }
        continue;
      }
  
+    if (FileList.is_open()) {
+      FileList.close();

"if is_open() then close()" doesn't look like a clear way of managing 
file handles to me. (More on that below)
+      CmdArgs.push_back(FileListName.data());


Ditto re. data().

+    }
+
      // Otherwise, this is a linker input argument.
      const Arg &A = II.getInputArg();
  
@@ -202,6 +251,13 @@
        A.renderAsInput(Args, CmdArgs);
    }
  
+  // NOTE this is needed here to handle the case where an object file is the
+  // last argument; otherwise, we don't properly close the file list.
+  if (FileList.is_open()) {
+    FileList.close();
+    CmdArgs.push_back(FileListName.data());
+  }
+

So, this ad-hoc open/close file handling is concerning. Can you 
investigate some form of RAII management, or at the very least 
streamlining the code flow to avoid special cases?

Alp.

    // LIBRARY_PATH - included following the user specified library paths.
    //                and only supported on native toolchains.
    if (!TC.isCrossCompiling())
@@ -4882,7 +4938,7 @@
    Args.AddAllArgs(CmdArgs, options::OPT_t);
    Args.AddAllArgs(CmdArgs, options::OPT_u_Group);
  
-  AddLinkerInputs(ToolChain, Inputs, Args, CmdArgs);
+  AddLinkerInputs(C, ToolChain, Inputs, Args, CmdArgs);
  
    //----------------------------------------------------------------------------
    // Libraries
@@ -5499,7 +5555,7 @@
      break;
    }
  
-  AddLinkerInputs(getToolChain(), Inputs, Args, CmdArgs);
+  AddLinkerInputs(C, getToolChain(), Inputs, Args, CmdArgs);
    
    if (isObjCRuntimeLinked(Args) &&
        !Args.hasArg(options::OPT_nostdlib) &&
@@ -5541,8 +5597,7 @@
    Args.AddAllArgs(CmdArgs, options::OPT_T_Group);
    Args.AddAllArgs(CmdArgs, options::OPT_F);
  
-  const char *Exec =
-    Args.MakeArgString(getToolChain().GetProgramPath("ld"));
+  const char *Exec = Args.MakeArgString(getToolChain().GetProgramPath("ld"));
    C.addCommand(new Command(JA, *this, Exec, CmdArgs));
  }
  
@@ -5709,7 +5764,7 @@
    Args.AddAllArgs(CmdArgs, options::OPT_e);
    Args.AddAllArgs(CmdArgs, options::OPT_r);
  
-  AddLinkerInputs(getToolChain(), Inputs, Args, CmdArgs);
+  AddLinkerInputs(C, getToolChain(), Inputs, Args, CmdArgs);
  
    if (!Args.hasArg(options::OPT_nostdlib) &&
        !Args.hasArg(options::OPT_nodefaultlibs)) {
@@ -5731,8 +5786,7 @@
  
    addProfileRT(getToolChain(), Args, CmdArgs);
  
-  const char *Exec =
-    Args.MakeArgString(getToolChain().GetProgramPath("ld"));
+  const char *Exec = Args.MakeArgString(getToolChain().GetProgramPath("ld"));
    C.addCommand(new Command(JA, *this, Exec, CmdArgs));
  }
  
@@ -5815,7 +5869,7 @@
    Args.AddAllArgs(CmdArgs, options::OPT_T_Group);
    Args.AddAllArgs(CmdArgs, options::OPT_e);
  
-  AddLinkerInputs(getToolChain(), Inputs, Args, CmdArgs);
+  AddLinkerInputs(C, getToolChain(), Inputs, Args, CmdArgs);
  
    if (!Args.hasArg(options::OPT_nostdlib) &&
        !Args.hasArg(options::OPT_nodefaultlibs)) {
@@ -5997,7 +6051,7 @@
    Args.AddAllArgs(CmdArgs, options::OPT_Z_Flag);
    Args.AddAllArgs(CmdArgs, options::OPT_r);
  
-  AddLinkerInputs(getToolChain(), Inputs, Args, CmdArgs);
+  AddLinkerInputs(C, getToolChain(), Inputs, Args, CmdArgs);
  
    if (!Args.hasArg(options::OPT_nostdlib) &&
        !Args.hasArg(options::OPT_nodefaultlibs)) {
@@ -6123,7 +6177,7 @@
    Args.AddAllArgs(CmdArgs, options::OPT_T_Group);
    Args.AddAllArgs(CmdArgs, options::OPT_e);
  
-  AddLinkerInputs(getToolChain(), Inputs, Args, CmdArgs);
+  AddLinkerInputs(C, getToolChain(), Inputs, Args, CmdArgs);
  
    if (!Args.hasArg(options::OPT_nostdlib) &&
        !Args.hasArg(options::OPT_nodefaultlibs)) {
@@ -6372,7 +6426,7 @@
    if (D.IsUsingLTO(Args))
      AddGoldPlugin(ToolChain, Args, CmdArgs);
  
-  AddLinkerInputs(ToolChain, Inputs, Args, CmdArgs);
+  AddLinkerInputs(C, ToolChain, Inputs, Args, CmdArgs);
  
    if (!Args.hasArg(options::OPT_nostdlib) &&
        !Args.hasArg(options::OPT_nodefaultlibs)) {
@@ -6634,7 +6688,7 @@
    Args.AddAllArgs(CmdArgs, options::OPT_Z_Flag);
    Args.AddAllArgs(CmdArgs, options::OPT_r);
  
-  AddLinkerInputs(getToolChain(), Inputs, Args, CmdArgs);
+  AddLinkerInputs(C, getToolChain(), Inputs, Args, CmdArgs);
  
    unsigned Major, Minor, Micro;
    getToolChain().getTriple().getOSVersion(Major, Minor, Micro);
@@ -7096,7 +7150,7 @@
    if (Args.hasArg(options::OPT_Z_Xlinker__no_demangle))
      CmdArgs.push_back("--no-demangle");
  
-  AddLinkerInputs(ToolChain, Inputs, Args, CmdArgs);
+  AddLinkerInputs(C, ToolChain, Inputs, Args, CmdArgs);
  
    addSanitizerRuntimes(getToolChain(), Args, CmdArgs);
    // The profile runtime also needs access to system libraries.
@@ -7225,7 +7279,7 @@
    Args.AddAllArgs(CmdArgs, options::OPT_T_Group);
    Args.AddAllArgs(CmdArgs, options::OPT_e);
  
-  AddLinkerInputs(getToolChain(), Inputs, Args, CmdArgs);
+  AddLinkerInputs(C, getToolChain(), Inputs, Args, CmdArgs);
  
    addProfileRT(getToolChain(), Args, CmdArgs);
  
@@ -7353,7 +7407,7 @@
    Args.AddAllArgs(CmdArgs, options::OPT_T_Group);
    Args.AddAllArgs(CmdArgs, options::OPT_e);
  
-  AddLinkerInputs(getToolChain(), Inputs, Args, CmdArgs);
+  AddLinkerInputs(C, getToolChain(), Inputs, Args, CmdArgs);
  
    if (!Args.hasArg(options::OPT_nostdlib) &&
        !Args.hasArg(options::OPT_nodefaultlibs)) {
@@ -7689,7 +7743,7 @@
    if (EH.ShouldUseExceptionTables)
      CmdArgs.push_back("-fexceptions");
  
-  AddLinkerInputs(getToolChain(), Inputs, Args, CmdArgs);
+  AddLinkerInputs(C, getToolChain(), Inputs, Args, CmdArgs);
  
    const char *Exec = Args.MakeArgString(getToolChain().GetProgramPath("xcc"));
    C.addCommand(new Command(JA, *this, Exec, CmdArgs));
Index: test/Driver/filelist.c
===================================================================
--- /dev/null
+++ test/Driver/filelist.c
@@ -0,0 +1,54 @@
+// Test handling of -fuse-linker-response-files
+
+// RUN: %clang -### %S/Inputs/filelist/a.o -o test 2>&1 \
+// RUN:   | FileCheck %s -check-prefix CHECK-NO-FILELIST
+
+// CHECK-NO-FILELIST-NOT: "@test0.rsp"
+// CHECK-NO-FILELIST: "{{.*}}/test/Driver/Inputs/filelist/a.o"
+
+// RUN: %clang -### -fuse-linker-response-files -save-temps %S/Inputs/filelist/a.o -o 
test 2>&1 \
+// RUN:   | FileCheck %s -check-prefix CHECK-FILELIST
+// RUN: FileCheck -check-prefix CHECK-FILELIST-0 %s < test0.rsp
+
+// CHECK-FILELIST-NOT: "{{.*}}/test/Driver/Inputs/filelist/a.o"
+// CHECK-FILELIST: "@test0.rsp"
+
+// CHECK-FILELIST-0: {{.*}}/test/Driver/Inputs/filelist/a.o
+
+// RUN: %clang -### -fuse-linker-response-files -save-temps %S/Inputs/filelist/a.o 
-library %S/Inputs/filelist/b.o -o test 2>&1 \
+// RUN:   | FileCheck %s -check-prefix CHECK-FILELIST-SPLIT
+// RUN: FileCheck -check-prefix CHECK-FILELIST-SPLIT-0 %s < test0.rsp
+// RUN: FileCheck -check-prefix CHECK-FILELIST-SPLIT-1 %s < test1.rsp
+
+// CHECK-FILELIST-SPLIT-NOT: "{{.*}}/test/Driver/Inputs/filelist/a.o"
+// CHECK-FILELIST-SPLIT: @test0.rsp
+// CHECK-FILELIST-SPLIT: -library
+// CHECK-FILELIST-SPLIT-NOT: "{{.*}}/test/Driver/Inputs/filelist/b.o"
+// CHECK-FILELIST-SPLIT: @test1.rsp
+
+// CHECK-FILELIST-SPLIT-0: a.o
+
+// CHECK-FILELIST-SPLIT-1: b.o
+
+// RUN: %clang '-###' -fuse-linker-response-files -save-temps %S/Inputs/filelist/a.o 
%S/Inputs/filelist/c.a %S/Inputs/filelist/b.o -o test 2>&1 \
+// RUN:   | FileCheck %s -check-prefix CHECK-FILELIST-ARCHIVES
+// RUN: FileCheck -check-prefix CHECK-FILELIST-ARCHIVES-0 %s < test0.rsp
+
+// CHECK-FILELIST-ARCHIVES-NOT: "{{.*}}/test/Driver/Inputs/filelist/a.o"
+// CHECK-FILELIST-ARCHIVES-NOT: "{{.*}}/test/Driver/Inputs/filelist/c.a"
+// CHECK-FILELIST-ARCHIVES-NOT: "{{.*}}/test/Driver/Inputs/filelist/b.o"
+// CHECK-FILELIST-ARCHIVES: @test0.rsp
+
+// CHECK-FILELIST-ARCHIVES-0: {{.*}}/test/Driver/Inputs/filelist/a.o
+// CHECK-FILELIST-ARCHIVES-0: {{.*}}/test/Driver/Inputs/filelist/c.a
+// CHECK-FILELIST-ARCHIVES-0: {{.*}}/test/Driver/Inputs/filelist/b.o
+
+// RUN: %clang -### -fuse-linker-response-files -save-temps %S/Inputs/filelist/a.obj 
-o test 2>&1 \
+// RUN:   | FileCheck %s -check-prefix CHECK-ALT-SUFFIX
+// RUN: FileCheck -check-prefix CHECK-ALT-SUFFIX-0 %s < test0.rsp
+
+// CHECK-ALT-SUFFIX-NOT: "{{.*}}/test/Driver/Inputs/filelist/a.obj"
+// CHECK-ALT-SUFFIX: "@test0.rsp"
+
+// CHECK-ALT-SUFFIX-0: {{.*}}/test/Driver/Inputs/filelist/a.obj
+


_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

--
http://www.nuanti.com
the browser experts

_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits