[lld] r195295 - [PECOFF] Move files with ".lib" extension to the end of the input file list.

[Rui Ueyama]

On Wed, Nov 20, 2013 at 7:59 PM, Saleem Abdulrasool
<compnerd at compnerd.org>wrote:

> On Wed, Nov 20, 2013 at 5:08 PM, Rui Ueyama <ruiu at google.com> wrote:
>
>> Author: ruiu
>> Date: Wed Nov 20 19:08:53 2013
>> New Revision: 195295
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=195295&view=rev
>> Log:
>> [PECOFF] Move files with ".lib" extension to the end of the input file
>> list.
>>
>> It's allowed to specify library files *before* object files in the command
>> line. Object files seems to be processed first, and then their undefined
>> symbols are resolved from the libraries. This patch implements the
>> compatible
>> behavior.
>>
>
> All object files are considered for their contributions.  Any unresolved
> symbols are searched for in the import libraries (which may contain object
> files, e.g {msv,}crt{,d}.lib provide entry point stubs via this method).
>
> Reordering these concerns me slightly as even though you use a stable sort
> to preserve the ordering, it seems like you don't account for the fact that
> someone may directly link against a module rather than an import library.
>  This can be problematic.  You may end up resolving functions from the
> wrong source as a result of the reordering.  If the unfortunate
> mis-resolved function is {Dll,Win}MainCRTStartup, you may end up invoking
> constructors multiple times for a given module, and none for another.
>  Perhaps Im missing something subtle, but, this change, as is, seems like
> it can generate binaries which behave incorrectly and are tricky to
> diagnose what went wrong.
>

As Mikael pointed out in a reply for r195289, this behavior is not very
accurate as a Windows linker. It seems that in most cases I believe the LLD
will create valid executables, but there could be a chance that it
mis-link. We definitely need to revisit this to fix the link order.

 Modified:
>>     lld/trunk/lib/Driver/WinLinkDriver.cpp
>>     lld/trunk/unittests/DriverTests/WinLinkDriverTest.cpp
>>
>> Modified: lld/trunk/lib/Driver/WinLinkDriver.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/lld/trunk/lib/Driver/WinLinkDriver.cpp?rev=195295&r1=195294&r2=195295&view=diff
>>
>> ==============================================================================
>> --- lld/trunk/lib/Driver/WinLinkDriver.cpp (original)
>> +++ lld/trunk/lib/Driver/WinLinkDriver.cpp Wed Nov 20 19:08:53 2013
>> @@ -13,6 +13,7 @@
>>  ///
>>
>>  //===----------------------------------------------------------------------===//
>>
>> +#include <algorithm>
>>  #include <cctype>
>>  #include <sstream>
>>  #include <map>
>> @@ -600,6 +601,8 @@ WinLinkDriver::parse(int argc, const cha
>>      defaultLibs.push_back((*it)->getValue());
>>    }
>>
>> +  std::vector<StringRef> inputFiles;
>> +
>>    // Process all the arguments and create Input Elements
>>    for (auto inputArg : *parsedArgs) {
>>      switch (inputArg->getOption().getID()) {
>> @@ -850,8 +853,7 @@ WinLinkDriver::parse(int argc, const cha
>>        break;
>>
>>      case OPT_INPUT:
>> -      inputElements.push_back(std::unique_ptr<InputElement>(
>> -          new PECOFFFileNode(ctx, ctx.allocate(inputArg->getValue()))));
>> +      inputFiles.push_back(ctx.allocate(inputArg->getValue()));
>>        break;
>>
>>  #define DEFINE_BOOLEAN_FLAG(name, setter)       \
>> @@ -877,6 +879,17 @@ WinLinkDriver::parse(int argc, const cha
>>      }
>>    }
>>
>> +  // Move files with ".lib" extension at the end of the input file list.
>> Say
>> +  // foo.obj depends on bar.lib. The linker needs to accept both "bar.lib
>> +  // foo.obj" and "foo.obj bar.lib".
>> +  auto compfn = [](StringRef a, StringRef b) {
>> +    return !a.endswith_lower(".lib") && b.endswith_lower(".lib");
>> +  };
>> +  std::stable_sort(inputFiles.begin(), inputFiles.end(), compfn);
>> +  for (StringRef path : inputFiles)
>> +    inputElements.push_back(std::unique_ptr<InputElement>(
>> +        new PECOFFFileNode(ctx, path)));
>> +
>>    // Use the default entry name if /entry option is not given.
>>    if (ctx.entrySymbolName().empty())
>>      ctx.setEntrySymbolName(getDefaultEntrySymbolName(ctx));
>>
>> Modified: lld/trunk/unittests/DriverTests/WinLinkDriverTest.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/lld/trunk/unittests/DriverTests/WinLinkDriverTest.cpp?rev=195295&r1=195294&r2=195295&view=diff
>>
>> ==============================================================================
>> --- lld/trunk/unittests/DriverTests/WinLinkDriverTest.cpp (original)
>> +++ lld/trunk/unittests/DriverTests/WinLinkDriverTest.cpp Wed Nov 20
>> 19:08:53 2013
>> @@ -131,6 +131,21 @@ TEST_F(WinLinkParserTest, Libpath) {
>>  }
>>
>>  //
>> +// Tests for input file order
>> +//
>> +
>> +TEST_F(WinLinkParserTest, InputOrder) {
>> +  EXPECT_TRUE(parse("link.exe", "b.lib", "b.obj", "c.obj", "a.lib",
>> "a.obj",
>> +                    nullptr));
>> +  EXPECT_EQ(5, inputFileCount());
>> +  EXPECT_EQ("b.obj", inputFile(0));
>> +  EXPECT_EQ("c.obj", inputFile(1));
>> +  EXPECT_EQ("a.obj", inputFile(2));
>> +  EXPECT_EQ("b.lib", inputFile(3));
>> +  EXPECT_EQ("a.lib", inputFile(4));
>> +}
>> +
>> +//
>>  // Tests for command line options that take values.
>>  //
>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>
>
>
> --
> Saleem Abdulrasool
> compnerd (at) compnerd (dot) org
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131121/229e58a8/attachment.html>