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