[lld] r196504 - [PECOFF] Handle .lib files as if they are grouped by --{start, end}-group.
Saleem Abdulrasool
compnerd at compnerd.org
Thu Dec 5 23:23:41 PST 2013
On Thu, Dec 5, 2013 at 5:07 AM, Rui Ueyama <ruiu at google.com> wrote:
> Author: ruiu
> Date: Thu Dec 5 07:07:49 2013
> New Revision: 196504
>
> URL: http://llvm.org/viewvc/llvm-project?rev=196504&view=rev
> Log:
> [PECOFF] Handle .lib files as if they are grouped by --{start,end}-group.
>
> Currently we do not de-duplicate library files specified by /defaultlib
> option.
> As a result, the same files are added multiple times to the input graph. In
> particular, some popular files, such as kernel32.lib or oldnames.lib, are
> added
> more than 10 times during linking of LLD. That makes the linker slower, as
> it
> needs to parse the same file again and again.
>
Just a quick comment on this change (I should really verify this, but, I
figure that just bringing it up might be a good idea anyways). As
previously noted, the symbol handling for PE/COFF is not entirely correct.
I think that this change also has the potential for introducing errors in
symbol resolution for PE/COFF.
PE/COFF does not take contributions for any unreferenced symbols. Ordering
is important since first definition takes precedence. Import libraries can
contain non short ILF content. The contributions from that content may
potentially introduce new unresolved symbols which must then be re-resolved
against another import library. If you need to re-refer to a previously
read library with a symbol definition in a second library that happens to
be ordered afterwards, I believe, it could result in the incorrect
definition.
> This patch solves the issue by de-duplicating. The same file will be added
> only
> once to the input graph. This patch improved the LLD linking time from 10.5
> seconds to 7.7 seconds on my 4-core Core i7 Macbook Pro.
>
> Modified:
> lld/trunk/include/lld/Driver/WinLinkInputGraph.h
> lld/trunk/include/lld/ReaderWriter/PECOFFLinkingContext.h
> lld/trunk/lib/Driver/WinLinkDriver.cpp
> lld/trunk/unittests/DriverTests/DriverTest.h
> lld/trunk/unittests/DriverTests/WinLinkDriverTest.cpp
>
> Modified: lld/trunk/include/lld/Driver/WinLinkInputGraph.h
> URL:
> http://llvm.org/viewvc/llvm-project/lld/trunk/include/lld/Driver/WinLinkInputGraph.h?rev=196504&r1=196503&r2=196504&view=diff
>
> ==============================================================================
> --- lld/trunk/include/lld/Driver/WinLinkInputGraph.h (original)
> +++ lld/trunk/include/lld/Driver/WinLinkInputGraph.h Thu Dec 5 07:07:49
> 2013
> @@ -61,6 +61,27 @@ public:
> virtual ErrorOr<StringRef> getPath(const LinkingContext &ctx) const;
> };
>
> +/// \brief Represents a ELF control node
> +class PECOFFGroup : public Group {
> +public:
> + PECOFFGroup() : Group(0) {}
> +
> + static inline bool classof(const InputElement *a) {
> + return a->kind() == InputElement::Kind::Control;
> + }
> +
> + virtual bool validate() { return true; }
> + virtual bool dump(raw_ostream &) { return true; }
> +
> + /// \brief Parse the group members.
> + error_code parse(const LinkingContext &ctx, raw_ostream &diagnostics) {
> + for (auto &elem : _elements)
> + if (error_code ec = elem->parse(ctx, diagnostics))
> + return ec;
> + return error_code::success();
> + }
> +};
> +
> } // namespace lld
>
> #endif
>
> Modified: lld/trunk/include/lld/ReaderWriter/PECOFFLinkingContext.h
> URL:
> http://llvm.org/viewvc/llvm-project/lld/trunk/include/lld/ReaderWriter/PECOFFLinkingContext.h?rev=196504&r1=196503&r2=196504&view=diff
>
> ==============================================================================
> --- lld/trunk/include/lld/ReaderWriter/PECOFFLinkingContext.h (original)
> +++ lld/trunk/include/lld/ReaderWriter/PECOFFLinkingContext.h Thu Dec 5
> 07:07:49 2013
> @@ -30,6 +30,7 @@ using llvm::COFF::WindowsSubsystem;
> static const uint8_t DEFAULT_DOS_STUB[128] = {'M', 'Z'};
>
> namespace lld {
> +class Group;
>
> class PECOFFLinkingContext : public LinkingContext {
> public:
> @@ -235,6 +236,9 @@ public:
> return false;
> }
>
> + void setLibraryGroup(Group *group) { _libraryGroup = group; }
> + Group *getLibraryGroup() const { return _libraryGroup; }
> +
> protected:
> /// Method to create a internal file for the entry symbol
> virtual std::unique_ptr<File> createEntrySymbolFile() const;
> @@ -299,6 +303,9 @@ private:
> // a small DOS program that prints out a message "This program requires
> // Microsoft Windows." This feature was somewhat useful before Windows
> 95.
> ArrayRef<uint8_t> _dosStub;
> +
> + // The PECOFFGroup that contains all the .lib files.
> + Group *_libraryGroup;
> };
>
> } // end namespace lld
>
> Modified: lld/trunk/lib/Driver/WinLinkDriver.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/lld/trunk/lib/Driver/WinLinkDriver.cpp?rev=196504&r1=196503&r2=196504&view=diff
>
> ==============================================================================
> --- lld/trunk/lib/Driver/WinLinkDriver.cpp (original)
> +++ lld/trunk/lib/Driver/WinLinkDriver.cpp Thu Dec 5 07:07:49 2013
> @@ -564,6 +564,19 @@ parseArgs(int argc, const char *argv[],
> return parsedArgs;
> }
>
> +// Returns true if the given file node has already been added to the input
> +// graph.
> +bool hasLibrary(const PECOFFLinkingContext &ctx, FileNode *fileNode) {
> + ErrorOr<StringRef> path = fileNode->getPath(ctx);
> + if (!path)
> + return false;
> + for (std::unique_ptr<InputElement> &p :
> ctx.getLibraryGroup()->elements())
> + if (auto *f = dyn_cast<FileNode>(p.get()))
> + if (*path == *f->getPath(ctx))
> + return true;
> + return false;
> +}
> +
> } // namespace
>
> //
> @@ -597,7 +610,8 @@ WinLinkDriver::parse(int argc, const cha
> return false;
>
> // The list of input files.
> - std::vector<std::unique_ptr<InputElement> > inputElements;
> + std::vector<std::unique_ptr<FileNode> > files;
> + std::vector<std::unique_ptr<FileNode> > libraries;
>
> // Handle /help
> if (parsedArgs->getLastArg(OPT_help)) {
> @@ -899,8 +913,7 @@ WinLinkDriver::parse(int argc, const cha
> };
> std::stable_sort(inputFiles.begin(), inputFiles.end(), compfn);
> for (StringRef path : inputFiles)
> - inputElements.push_back(std::unique_ptr<InputElement>(
> - new PECOFFFileNode(ctx, path)));
> + files.push_back(std::unique_ptr<FileNode>(new PECOFFFileNode(ctx,
> path)));
>
> // Use the default entry name if /entry option is not given.
> if (ctx.entrySymbolName().empty() &&
> !parsedArgs->getLastArg(OPT_noentry))
> @@ -933,9 +946,9 @@ WinLinkDriver::parse(int argc, const cha
> // but useful for us to test lld on Unix.
> if (llvm::opt::Arg *dashdash = parsedArgs->getLastArg(OPT_DASH_DASH)) {
> for (const StringRef value : dashdash->getValues()) {
> - std::unique_ptr<InputElement> elem(
> + std::unique_ptr<FileNode> elem(
> new PECOFFFileNode(ctx, ctx.allocate(value)));
> - inputElements.push_back(std::move(elem));
> + files.push_back(std::move(elem));
> }
> }
>
> @@ -944,10 +957,10 @@ WinLinkDriver::parse(int argc, const cha
> if (!ctx.getNoDefaultLibAll())
> for (const StringRef path : defaultLibs)
> if (!ctx.hasNoDefaultLib(path))
> - inputElements.push_back(std::unique_ptr<InputElement>(
> - new PECOFFLibraryNode(ctx, path)));
> + libraries.push_back(std::unique_ptr<FileNode>(
> + new PECOFFLibraryNode(ctx,
> ctx.allocate(path.lower()))));
>
> - if (inputElements.empty() && !isReadingDirectiveSection) {
> + if (files.empty() && !isReadingDirectiveSection) {
> diagnostics << "No input files\n";
> return false;
> }
> @@ -956,7 +969,7 @@ WinLinkDriver::parse(int argc, const cha
> // constructed by replacing an extension of the first input file
> // with ".exe".
> if (ctx.outputPath().empty()) {
> - StringRef path =
> *dyn_cast<FileNode>(&*inputElements[0])->getPath(ctx);
> + StringRef path = *dyn_cast<FileNode>(&*files[0])->getPath(ctx);
> ctx.setOutputPath(replaceExtension(ctx, path, ".exe"));
> }
>
> @@ -968,19 +981,32 @@ WinLinkDriver::parse(int argc, const cha
> ctx.setManifestOutputPath(ctx.allocate(path));
> }
>
> - // If the core linker already started, we need to explicitly call
> parse() for
> - // each input element, because the pass to parse input files in
> Driver::link
> - // has already done.
> - if (isReadingDirectiveSection)
> - for (auto &e : inputElements)
> - if (e->parse(ctx, diagnostics))
> - return false;
> -
> // Add the input files to the input graph.
> if (!ctx.hasInputGraph())
> ctx.setInputGraph(std::unique_ptr<InputGraph>(new InputGraph()));
> - for (auto &e : inputElements)
> - ctx.inputGraph().addInputElement(std::move(e));
> + for (auto &file : files) {
> + if (isReadingDirectiveSection)
> + if (file->parse(ctx, diagnostics))
> + return false;
> + ctx.inputGraph().addInputElement(std::move(file));
> + }
> +
> + // Add the library group to the input graph.
> + if (!isReadingDirectiveSection) {
> + auto group = std::unique_ptr<Group>(new PECOFFGroup());
> + ctx.setLibraryGroup(group.get());
> + ctx.inputGraph().addInputElement(std::move(group));
> + }
> +
> + // Add the library files to the library group.
> + for (std::unique_ptr<FileNode> &lib : libraries) {
> + if (!hasLibrary(ctx, lib.get())) {
> + if (isReadingDirectiveSection)
> + if (lib->parse(ctx, diagnostics))
> + return false;
> + ctx.getLibraryGroup()->processInputElement(std::move(lib));
> + }
> + }
>
> // Validate the combination of options used.
> return ctx.validate(diagnostics);
>
> Modified: lld/trunk/unittests/DriverTests/DriverTest.h
> URL:
> http://llvm.org/viewvc/llvm-project/lld/trunk/unittests/DriverTests/DriverTest.h?rev=196504&r1=196503&r2=196504&view=diff
>
> ==============================================================================
> --- lld/trunk/unittests/DriverTests/DriverTest.h (original)
> +++ lld/trunk/unittests/DriverTests/DriverTest.h Thu Dec 5 07:07:49 2013
> @@ -32,13 +32,24 @@ protected:
> int inputFileCount() { return linkingContext()->inputGraph().size(); }
>
> // Convenience method for getting i'th input files name.
> - std::string inputFile(unsigned index) {
> + std::string inputFile(int index) {
> const InputElement &inputElement =
> linkingContext()->inputGraph()[index];
> if (inputElement.kind() == InputElement::Kind::File)
> return
> *dyn_cast<FileNode>(&inputElement)->getPath(*linkingContext());
> llvm_unreachable("not handling other types of input files");
> }
>
> + // Convenience method for getting i'th input files name.
> + std::string inputFile(int index1, int index2) {
> + Group *group =
> dyn_cast<Group>(&linkingContext()->inputGraph()[index1]);
> + if (!group)
> + llvm_unreachable("not handling other types of input files");
> + FileNode *file = dyn_cast<FileNode>(group->elements()[index2].get());
> + if (!file)
> + llvm_unreachable("not handling other types of input files");
> + return *file->getPath(*linkingContext());
> + }
> +
> // For unit tests to call driver with various command lines.
> bool parse(const char *args, ...) {
> // Construct command line options from varargs.
>
> Modified: lld/trunk/unittests/DriverTests/WinLinkDriverTest.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/lld/trunk/unittests/DriverTests/WinLinkDriverTest.cpp?rev=196504&r1=196503&r2=196504&view=diff
>
> ==============================================================================
> --- lld/trunk/unittests/DriverTests/WinLinkDriverTest.cpp (original)
> +++ lld/trunk/unittests/DriverTests/WinLinkDriverTest.cpp Thu Dec 5
> 07:07:49 2013
> @@ -38,7 +38,7 @@ TEST_F(WinLinkParserTest, Basic) {
> EXPECT_EQ(llvm::COFF::IMAGE_FILE_MACHINE_I386,
> _context.getMachineType());
> EXPECT_EQ("a.exe", _context.outputPath());
> EXPECT_EQ("_start", _context.entrySymbolName());
> - EXPECT_EQ(3, inputFileCount());
> + EXPECT_EQ(4, inputFileCount());
> EXPECT_EQ("a.obj", inputFile(0));
> EXPECT_EQ("b.obj", inputFile(1));
> EXPECT_EQ("c.obj", inputFile(2));
> @@ -77,7 +77,7 @@ TEST_F(WinLinkParserTest, StartsWithHyph
> parse("link.exe", "-subsystem:console", "-out:a.exe", "a.obj",
> nullptr));
> EXPECT_EQ(llvm::COFF::IMAGE_SUBSYSTEM_WINDOWS_CUI,
> _context.getSubsystem());
> EXPECT_EQ("a.exe", _context.outputPath());
> - EXPECT_EQ(1, inputFileCount());
> + EXPECT_EQ(2, inputFileCount());
> EXPECT_EQ("a.obj", inputFile(0));
> }
>
> @@ -86,7 +86,7 @@ TEST_F(WinLinkParserTest, UppercaseOptio
> parse("link.exe", "/SUBSYSTEM:CONSOLE", "/OUT:a.exe", "a.obj",
> nullptr));
> EXPECT_EQ(llvm::COFF::IMAGE_SUBSYSTEM_WINDOWS_CUI,
> _context.getSubsystem());
> EXPECT_EQ("a.exe", _context.outputPath());
> - EXPECT_EQ(1, inputFileCount());
> + EXPECT_EQ(2, inputFileCount());
> EXPECT_EQ("a.obj", inputFile(0));
> }
>
> @@ -109,7 +109,7 @@ TEST_F(WinLinkParserTest, NoInputFiles)
> TEST_F(WinLinkParserTest, NoFileExtension) {
> EXPECT_TRUE(parse("link.exe", "foo", "bar", nullptr));
> EXPECT_EQ("foo.exe", _context.outputPath());
> - EXPECT_EQ(2, inputFileCount());
> + EXPECT_EQ(3, inputFileCount());
> EXPECT_EQ("foo.obj", inputFile(0));
> EXPECT_EQ("bar.obj", inputFile(1));
> }
> @@ -117,7 +117,7 @@ TEST_F(WinLinkParserTest, NoFileExtensio
> TEST_F(WinLinkParserTest, NonStandardFileExtension) {
> EXPECT_TRUE(parse("link.exe", "foo.o", nullptr));
> EXPECT_EQ("foo.exe", _context.outputPath());
> - EXPECT_EQ(1, inputFileCount());
> + EXPECT_EQ(2, inputFileCount());
> EXPECT_EQ("foo.o", inputFile(0));
> }
>
> @@ -137,7 +137,7 @@ TEST_F(WinLinkParserTest, Libpath) {
> 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(6, inputFileCount());
> EXPECT_EQ("b.obj", inputFile(0));
> EXPECT_EQ("c.obj", inputFile(1));
> EXPECT_EQ("a.obj", inputFile(2));
> @@ -319,19 +319,18 @@ TEST_F(WinLinkParserTest, SectionMultipl
> TEST_F(WinLinkParserTest, DefaultLib) {
> EXPECT_TRUE(parse("link.exe", "/defaultlib:user32.lib",
> "/defaultlib:kernel32", "a.obj", nullptr));
> - EXPECT_EQ(3, inputFileCount());
> + EXPECT_EQ(2, inputFileCount());
> EXPECT_EQ("a.obj", inputFile(0));
> - EXPECT_EQ("user32.lib", inputFile(1));
> - EXPECT_EQ("kernel32.lib", inputFile(2));
> + EXPECT_EQ("user32.lib", inputFile(1, 0));
> + EXPECT_EQ("kernel32.lib", inputFile(1, 1));
> }
>
> TEST_F(WinLinkParserTest, DefaultLibDuplicates) {
> EXPECT_TRUE(parse("link.exe", "/defaultlib:user32.lib",
> "/defaultlib:user32.lib", "a.obj", nullptr));
> - EXPECT_EQ(3, inputFileCount());
> + EXPECT_EQ(2, inputFileCount());
> EXPECT_EQ("a.obj", inputFile(0));
> - EXPECT_EQ("user32.lib", inputFile(1));
> - EXPECT_EQ("user32.lib", inputFile(2));
> + EXPECT_EQ("user32.lib", inputFile(1, 0));
> }
>
> TEST_F(WinLinkParserTest, NoDefaultLib) {
> @@ -340,13 +339,13 @@ TEST_F(WinLinkParserTest, NoDefaultLib)
> nullptr));
> EXPECT_EQ(2, inputFileCount());
> EXPECT_EQ("a.obj", inputFile(0));
> - EXPECT_EQ("kernel32.lib", inputFile(1));
> + EXPECT_EQ("kernel32.lib", inputFile(1, 0));
> }
>
> TEST_F(WinLinkParserTest, NoDefaultLibAll) {
> EXPECT_TRUE(parse("link.exe", "/defaultlib:user32.lib",
> "/defaultlib:kernel32", "/nodefaultlib", "a.obj",
> nullptr));
> - EXPECT_EQ(1, inputFileCount());
> + EXPECT_EQ(2, inputFileCount());
> EXPECT_EQ("a.obj", inputFile(0));
> }
>
> @@ -356,7 +355,7 @@ TEST_F(WinLinkParserTest, DisallowLib) {
> nullptr));
> EXPECT_EQ(2, inputFileCount());
> EXPECT_EQ("a.obj", inputFile(0));
> - EXPECT_EQ("kernel32.lib", inputFile(1));
> + EXPECT_EQ("kernel32.lib", inputFile(1, 0));
> }
>
> //
> @@ -567,7 +566,7 @@ TEST_F(WinLinkParserTest, Ignore) {
> "/ignoreidl", "/implib:foo", "/safeseh",
> "/safeseh:no",
> "/functionpadmin", "a.obj", nullptr));
> EXPECT_EQ("", errorMessage());
> - EXPECT_EQ(1, inputFileCount());
> + EXPECT_EQ(2, inputFileCount());
> EXPECT_EQ("a.obj", inputFile(0));
> }
>
> @@ -580,7 +579,7 @@ TEST_F(WinLinkParserTest, DashDash) {
> "--", "b.obj", "-c.obj", nullptr));
> EXPECT_EQ(llvm::COFF::IMAGE_SUBSYSTEM_WINDOWS_CUI,
> _context.getSubsystem());
> EXPECT_EQ("a.exe", _context.outputPath());
> - EXPECT_EQ(3, inputFileCount());
> + EXPECT_EQ(4, inputFileCount());
> EXPECT_EQ("a.obj", inputFile(0));
> EXPECT_EQ("b.obj", inputFile(1));
> EXPECT_EQ("-c.obj", inputFile(2));
>
>
> _______________________________________________
> 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/20131205/adbae3c7/attachment.html>
More information about the llvm-commits
mailing list