[PATCH] Driver: add support for linker file lists
Alp Toker
alp at nuanti.com
Wed Jun 11 15:59:01 PDT 2014
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 at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
--
http://www.nuanti.com
the browser experts
More information about the cfe-commits
mailing list