[lld] r182912 - [WinLink][Driver] Handle file extensions and defualt output file name.

[David Blaikie]

[readding Reid and llvm-commits]

On Thu, May 30, 2013 at 10:33 AM, Rui Ueyama <ruiu at google.com> wrote:
> Thank you for reviewing. Looks like this is indeed use-after-return :/ I
> will address this issue in another patch, in the way as suggested by David.
> We can use std::move in lld because lld already requires C++11 to compile.
>
> Shankar,
> Adding ".obj" to a path without extension is for compatibility with
> link.exe. Link.exe adds the extension to files who doesn't have extension
> first, then try to open the files. I will try to write a unit test for
> WinLinkDriver. We should write unit tests for other drivers too.
>
>
> On Thu, May 30, 2013 at 10:08 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> On Thu, May 30, 2013 at 9:16 AM, Reid Kleckner <rnk at google.com> wrote:
>> > On Thu, May 30, 2013 at 2:00 AM, Rui Ueyama <ruiu at google.com> wrote:
>> >>
>> >> Author: ruiu
>> >> Date: Thu May 30 01:00:10 2013
>> >> New Revision: 182912
>> >>
>> >> URL: http://llvm.org/viewvc/llvm-project?rev=182912&view=rev
>> >> Log:
>> >> [WinLink][Driver] Handle file extensions and defualt output file name.
>> >>
>> >> Modified:
>> >>     lld/trunk/lib/Driver/WinLinkDriver.cpp
>> >>
>> >> Modified: lld/trunk/lib/Driver/WinLinkDriver.cpp
>> >> URL:
>> >>
>> >> http://llvm.org/viewvc/llvm-project/lld/trunk/lib/Driver/WinLinkDriver.cpp?rev=182912&r1=182911&r2=182912&view=diff
>> >>
>> >>
>> >> ==============================================================================
>> >> --- lld/trunk/lib/Driver/WinLinkDriver.cpp (original)
>> >> +++ lld/trunk/lib/Driver/WinLinkDriver.cpp Thu May 30 01:00:10 2013
>> >> @@ -16,6 +16,7 @@
>> >>  #include "llvm/ADT/StringSwitch.h"
>> >>  #include "llvm/Option/Arg.h"
>> >>  #include "llvm/Option/Option.h"
>> >> +#include "llvm/Support/PathV2.h"
>> >>
>> >>  #include "lld/Driver/Driver.h"
>> >>  #include "lld/ReaderWriter/PECOFFTargetInfo.h"
>> >> @@ -72,6 +73,21 @@ llvm::COFF::WindowsSubsystem strToWinSub
>> >>        .Default(llvm::COFF::IMAGE_SUBSYSTEM_UNKNOWN);
>> >>  }
>> >>
>> >> +// Add ".obj" extension if the given path name has no file extension.
>> >> +StringRef canonicalizeInputFileName(StringRef path) {
>> >> +  if (llvm::sys::path::extension(path).empty())
>> >> +    return path.str() + ".obj";
>> >
>> >
>> > Isn't this a use-after-return?  path.str() makes a temp std::string,
>> > which
>> > is concatenated.  Then we construct the StringRef from the string.
>> >
>> > Probably the right thing is to take a std::string& and modify it in
>> > place.
>>
>> Pushing the limits a little:
>>
>> Alternatively - take a std::string by value, mutate it, then return it
>> by value. We only actually care about the performance of Clang/LLVM
>> when built as C++11 anyway (though, true, we'd need some way to say
>> "std::move" (should probably have some LLVM compatibility function
>> that's a no-op in C++98 and does std::move in C++11) at the call site
>> - and maybe that's more hassle than its worth if every caller likely
>> wants to mutate an existing string anyway)
>>
>> >
>> >>
>> >> +  return path;
>> >> +}
>> >> +
>> >> +// Replace a file extension with ".exe". If the given file has no
>> >> +// extension, just add ".exe".
>> >> +StringRef getDefaultOutputFileName(StringRef path) {
>> >> +  StringRef ext = llvm::sys::path::extension(path);
>> >> +  StringRef filename = ext.empty() ? path :
>> >> path.drop_back(ext.size());
>> >> +  return filename.str() + ".exe";
>> >
>> >
>> > ditto?
>> >
>> >>
>> >> +}
>> >> +
>> >>  } // namespace
>> >>
>> >>
>> >> @@ -141,16 +157,26 @@ bool WinLinkDriver::parse(int argc, cons
>> >>      info.setOutputPath(outpath->getValue());
>> >>
>> >>    // Add input files
>> >> +  std::vector<StringRef> inputPaths;
>> >>    for (llvm::opt::arg_iterator it =
>> >> parsedArgs->filtered_begin(OPT_INPUT),
>> >>                                 ie = parsedArgs->filtered_end();
>> >>         it != ie; ++it) {
>> >> -    info.appendInputFile((*it)->getValue());
>> >> +    inputPaths.push_back((*it)->getValue());
>> >>    }
>> >>
>> >>    // Arguments after "--" are also input files
>> >>    if (doubleDashPosition > 0)
>> >>      for (int i = doubleDashPosition + 1; i < argc; ++i)
>> >> -      info.appendInputFile(argv[i]);
>> >> +      inputPaths.push_back(argv[i]);
>> >> +
>> >> +  // Add ".obj" extension for those who have no file extension.
>> >> +  for (const StringRef &path : inputPaths)
>> >> +    info.appendInputFile(canonicalizeInputFileName(path));
>> >> +
>> >> +  // If -out option was not specified, the default output file name is
>> >> +  // constructed by replacing an extension with ".exe".
>> >> +  if (info.outputPath().empty() && !inputPaths.empty())
>> >> +    info.setOutputPath(getDefaultOutputFileName(inputPaths[0]));
>> >>
>> >>    // Validate the combination of options used.
>> >>    return info.validate(diagnostics);
>> >>
>> >>
>> >> _______________________________________________
>> >> llvm-commits mailing list
>> >> llvm-commits at cs.uiuc.edu
>> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> >
>> >
>> >
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> >
>
>