fix passing long argument lists to the linker, PR 15171, take 2

Nathan Froyd froydnj at mozilla.com
Mon May 13 08:49:24 PDT 2013


I haven't lost interest in this patch, it's just become less urgent since
the build is still working. :)  I'll look at addressing the comments you
had this week.

-Nathan

----- Original Message -----
> Reverse ping :-)
> 
> I noticed you guys changed your build to use -flielist on OS X. Have
> you lost interest in this patch then? If so, we should probably drop
> the llvm bits too.
> 
> On 16 April 2013 10:20, Rafael EspĂ­ndola <rafael.espindola at gmail.com>
> wrote:
> > On 15 April 2013 14:10, Nathan Froyd <nfroyd at mozilla.com> wrote:
> >> Hi!
> >>
> >> This patch addresses PR 15171 by checking if we're constructing a
> >> too-long argument list for the linker and writing it to a response
> >> file
> >> if the linker supports it.
> >
> > * Please start function names with a lowercase letter.
> > * The llvm::sys::Path object is only used to delete the file, so
> > you
> > can just say:
> >
> >   llvm::sys::Path(TmpPath).eraseFromDisk();
> >
> > * You don't need to pass the '.' to D.GetTemporaryPath. It was
> > creating a file ending with "..tmp".
> > * Don't use braces for single line blocks.
> >
> > MakeArgString Can take a Twine, so you don't need the FileArg
> > temporary.
> >
> >>  A test case is included that should test the
> >> newly-added behavior on at least some Windows checkouts (depending
> >> on
> >> where the checkout lives on disk).  It doesn't appear that there's
> >> any
> >> tests that clang parses a response file, so this test takes care
> >> of that
> >> too.
> >
> > For the testcase I had something like the attached file in mind. By
> > using the preprocessor we can create a large response file. Testing
> > this on OS X showed that clang does try to use a response file, but
> > the OS X linker doesn't support it. You should probably pass false
> > to
> > SupportsResponseFiles when calling from OS X and xfail the test for
> > OS
> > X.
> >
> >
> >> Tested on linux x86-64.
> >
> > Since this is very host dependent, you should probably also test on
> > windows and OS X at least.
> >
> >> -Nathan
> >>
> >
> > Cheers,
> > Rafael
> 




More information about the cfe-commits mailing list