[PATCH] Driver: add support for linker file lists
Saleem Abdulrasool
abdulras at fb.com
Wed Jun 11 16:58:58 PDT 2014
On Jun 11, 2014, at 3:59 PM, Alp Toker <alp at nuanti.com> wrote:
>
> 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.
Sorry.
> How about starting with something like "Driver: generate input file lists to pass through to the linker" before providing the detailed description above?
Sure, that sounds good to me.
> I still don't have fully clarify (correct me if I misunderstood!) but here are some early thoughts inline…
Its pretty simple. Any runs of object files are written to a temporary “response file” (which technically is a file list). This single file takes the place of all of the items in the run. The linker is notified that it needs to consume the file to get the list of items taking place of the file list by prepending the @ character to the file name.
Since this is supposed to be a file list (a response file can actually pass flags as well in the file, but the file list is the least common denominator between various linkers), any interspersed flags cause a split, which we track via sequence numbers.
>>
>> 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.
>>
>> https://urldefense.proofpoint.com/v1/url?u=http://reviews.llvm.org/D4104&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=CchYc4lrV44%2BZqxZADw0BQ%3D%3D%0A&m=huMRJpU7vYKds86SFHZIGFiPnow6vtbJLtyyWs%2FvfZI%3D%0A&s=544158009b15089c7946c55ff925a2c369a3e78e8afd486c7f77e48ddceef3f8
>>
>> 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?
I welcome suggestions to improve the diagnostics; I really do care about them.
>> 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.
GCC does not have this option because it is hardcoded at build time (—with-gnu-ld will enable this feature). We could mark it as a clang extension if you like.
> 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.
That is an option. However, because you don’t know what the linker is, I think it is safer to permit the user to explicitly state that they want this feature. By leaving it off by default, we ensure that we don’t break compatibility with any linker that doesn’t support it and we still permit the user to indicate to us that their compiler can support this.
>
>> 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.
Good idea.
>> + } 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?
Im afraid I don’t follow what you are suggesting here.
>> +
>> + // Skip the leading '@'
>> + FileList.open(FileListName.substr(1).data(),
>
> drop_front()? If you keep this approach, I suggest an assert() that front() == '@' as well.
Oh, right, I forgot about drop_front(). But, I like the assert idea irrespective, and will add that.
> 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.
I can change the signature from a StringRef to a const char *. The string is guaranteed null terminated due to the Args.MakeArgString.
>
>> + 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?
The reason that this is done this way is because you may have a number of these files being created as they are limited to a run of object files. It isn’t scope limited, but arbitrary dependent on the user’s input.
> 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
>> https://urldefense.proofpoint.com/v1/url?u=http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=CchYc4lrV44%2BZqxZADw0BQ%3D%3D%0A&m=huMRJpU7vYKds86SFHZIGFiPnow6vtbJLtyyWs%2FvfZI%3D%0A&s=c985b623f0e548f958682863086a3c43aa4e583b6e79ac9838c08b6d7acfcd49
>
> --
> https://urldefense.proofpoint.com/v1/url?u=http://www.nuanti.com/&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=CchYc4lrV44%2BZqxZADw0BQ%3D%3D%0A&m=huMRJpU7vYKds86SFHZIGFiPnow6vtbJLtyyWs%2FvfZI%3D%0A&s=f97ebb537a728935fbb0e595c2784c386225311d65d2d2c11daec99ed435f895
> the browser experts
>
--
Saleem Abdulrasool
abdulras (at) fb (dot) com
More information about the cfe-commits
mailing list