[PECOFF] Invoke cvtres.exe in the driver.

Previously we invoked cvtres.exe for each compiled Windows
resource file. The generated files were then concatenated
and embedded to the executable.

That was not the correct way to merge compiled Windows
resource files. If you just concatenate generated files,
only the first file would be recognized and the rest would
be ignored as trailing garbage.

The right way to merge them is to call cvtres.exe with
multiple input files. In this patch we do that in the
Windows driver.

llvm-svn: 212763

Author: rui314

lld/include/lld/ReaderWriter/Reader.h

@@ -122,7 +122,6 @@ class Registry {
   void addSupportNativeObjects();
   void addSupportCOFFObjects(PECOFFLinkingContext &);
   void addSupportCOFFImportLibraries();
-  void addSupportWindowsResourceFiles();
   void addSupportMachOObjects(StringRef archName);
   void addSupportELFObjects(bool atomizeStrings, TargetHandlerBase *handler);
   void addSupportELFDynamicSharedObjects(bool useShlibUndefines,

lld/lib/Driver/WinLinkDriver.cpp

@@ -282,6 +282,53 @@ static bool parseManifest(StringRef option, bool &enable, bool &embed,
   return true;
 }
 
+// Returns true if the given file is a Windows resource file.
+static bool isResoruceFile(StringRef path) {
+  llvm::sys::fs::file_magic fileType;
+  if (llvm::sys::fs::identify_magic(path, fileType)) {
+    // If we cannot read the file, assume it's not a resource file.
+    // The further stage will raise an error on this unreadable file.
+    return false;
+  }
+  return fileType == llvm::sys::fs::file_magic::windows_resource;
+}
+
+// Merge Windows resource files and convert them to a single COFF file.
+// The temporary file path is set to result.
+static bool convertResourceFiles(std::vector<std::string> inFiles,
+                                 std::string &result) {
+  // Create an output file path.
+  SmallString<128> outFile;
+  if (llvm::sys::fs::createTemporaryFile("resource", "obj", outFile))
+    return false;
+  std::string outFileArg = ("/out:" + outFile).str();
+
+  // Construct CVTRES.EXE command line and execute it.
+  std::string program = "cvtres.exe";
+  std::string programPath = llvm::sys::FindProgramByName(program);
+  if (programPath.empty()) {
+    llvm::errs() << "Unable to find " << program << " in PATH\n";
+    return false;
+  }
+
+  std::vector<const char *> args;
+  args.push_back(programPath.c_str());
+  args.push_back("/machine:x86");
+  args.push_back("/readonly");
+  args.push_back("/nologo");
+  args.push_back(outFileArg.c_str());
+  for (const std::string &path : inFiles)
+    args.push_back(path.c_str());
+  args.push_back(nullptr);
+
+  if (llvm::sys::ExecuteAndWait(programPath.c_str(), &args[0]) != 0) {
+    llvm::errs() << program << " failed\n";
+    return false;
+  }
+  result = outFile.str();
+  return true;
+}
+
 // Parse /manifestuac:(level=<string>|uiAccess=<string>).
 //
 // The arguments will be embedded to the manifest XML file with no error check,
@@ -503,43 +550,37 @@ static bool createManifestResourceFile(PECOFFLinkingContext &ctx,
   return true;
 }
 
-// Create a side-by-side manifest file. The side-by-side manifest file is a
-// separate XML file having ".manifest" extension. It will be created in the
-// same directory as the resulting executable.
+
+// Create the a side-by-side manifest file.
+//
+// The manifest file will convey some information to the linker, such as whether
+// the binary needs to run as Administrator or not. Instead of being placed in
+// the PE/COFF header, it's in XML format for some reason -- I guess it's
+// probably because it's invented in the early dot-com era.
+//
+// The side-by-side manifest file is a separate XML file having ".manifest"
+// extension. It will be created in the same directory as the resulting
+// executable.
 static bool createSideBySideManifestFile(PECOFFLinkingContext &ctx,
                                          raw_ostream &diag) {
+  std::string path = ctx.getManifestOutputPath();
+  if (path.empty()) {
+    // Default name of the manifest file is "foo.exe.manifest" where "foo.exe" is
+    // the output path.
+    path = ctx.outputPath();
+    path.append(".manifest");
+  }
+
   std::string errorInfo;
-  llvm::raw_fd_ostream out(ctx.getManifestOutputPath().data(), errorInfo,
-                           llvm::sys::fs::F_Text);
+  llvm::raw_fd_ostream out(path.c_str(), errorInfo, llvm::sys::fs::F_Text);
   if (!errorInfo.empty()) {
-    diag << "Failed to open " << ctx.getManifestOutputPath() << ": "
-         << errorInfo << "\n";
+    diag << errorInfo << "\n";
     return false;
   }
   out << createManifestXml(ctx);
   return true;
 }
 
-// Create the a side-by-side manifest file, or create a resource file for the
-// manifest file and add it to the input graph.
-//
-// The manifest file will convey some information to the linker, such as whether
-// the binary needs to run as Administrator or not. Instead of being placed in
-// the PE/COFF header, it's in XML format for some reason -- I guess it's
-// probably because it's invented in the early dot-com era.
-static bool createManifest(PECOFFLinkingContext &ctx, raw_ostream &diag) {
-  if (ctx.getEmbedManifest()) {
-    std::string resourceFilePath;
-    if (!createManifestResourceFile(ctx, diag, resourceFilePath))
-      return false;
-    std::unique_ptr<InputElement> inputElement(
-        new PECOFFFileNode(ctx, ctx.allocate(resourceFilePath)));
-    ctx.getInputGraph().addInputElement(std::move(inputElement));
-    return true;
-  }
-  return createSideBySideManifestFile(ctx, diag);
-}
-
 // Handle /failifmismatch option.
 static bool
 handleFailIfMismatchOption(StringRef option,
@@ -770,14 +811,13 @@ bool WinLinkDriver::linkPECOFF(int argc, const char **argv, raw_ostream &diag) {
     return false;
 
   // Create the file if needed.
-  if (context.getCreateManifest())
-    if (!createManifest(context, diag))
+  if (context.getCreateManifest() && !context.getEmbedManifest())
+    if (!createSideBySideManifestFile(context, diag))
       return false;
 
   // Register possible input file parsers.
   context.registry().addSupportCOFFObjects(context);
   context.registry().addSupportCOFFImportLibraries();
-  context.registry().addSupportWindowsResourceFiles();
   context.registry().addSupportArchives(context.logInputFiles());
   context.registry().addSupportNativeObjects();
   context.registry().addSupportYamlFiles();
@@ -1202,6 +1242,35 @@ bool WinLinkDriver::parse(int argc, const char *argv[],
     for (const StringRef value : dashdash->getValues())
       inputFiles.push_back(value);
 
+  // Compile Windows resource files to compiled resource file.
+  if (ctx.getCreateManifest() && ctx.getEmbedManifest() &&
+      !isReadingDirectiveSection) {
+    std::string resFile;
+    if (!createManifestResourceFile(ctx, diag, resFile))
+      return false;
+    inputFiles.push_back(ctx.allocate(resFile));
+  }
+
+  // A Windows Resource file is not an object file. It contains data,
+  // such as an icon image, and is not in COFF file format. If resource
+  // files are given, the linker merge them into one COFF file using
+  // CVTRES.EXE and then link the resulting file.
+  {
+    auto it = std::partition(inputFiles.begin(), inputFiles.end(),
+                             isResoruceFile);
+    if (it != inputFiles.begin()) {
+      std::vector<std::string> resFiles(inputFiles.begin(), it);
+      std::string resObj;
+      if (!convertResourceFiles(resFiles, resObj)) {
+        diag << "Failed to convert resource files\n";
+        return false;
+      }
+      inputFiles = std::vector<StringRef>(it, inputFiles.end());
+      inputFiles.push_back(ctx.allocate(resObj));
+      ctx.registerTemporaryFile(resObj);
+    }
+  }
+
   // Prepare objects to add them to input graph.
   for (StringRef path : inputFiles) {
     path = ctx.allocate(path);
@@ -1253,14 +1322,6 @@ bool WinLinkDriver::parse(int argc, const char *argv[],
     ctx.setOutputPath(replaceExtension(ctx, path, ".exe"));
   }
 
-  // Default name of the manifest file is "foo.exe.manifest" where "foo.exe" is
-  // the output path.
-  if (ctx.getManifestOutputPath().empty()) {
-    std::string path = ctx.outputPath();
-    path.append(".manifest");
-    ctx.setManifestOutputPath(ctx.allocate(path));
-  }
-
   // Add the input files to the input graph.
   if (!ctx.hasInputGraph())
     ctx.setInputGraph(std::unique_ptr<InputGraph>(new InputGraph()));

lld/lib/ReaderWriter/PECOFF/ReaderCOFF.cpp

@@ -935,112 +935,6 @@ StringRef FileCOFF::ArrayRefToString(ArrayRef<uint8_t> array) {
   return StringRef(*contents).trim();
 }
 
-// Convert .res file to .coff file and then parse it. Resource file is a file
-// containing various types of data, such as icons, translation texts,
-// etc. "cvtres.exe" command reads an RC file to create a COFF file which
-// encapsulates resource data into rsrc$N sections, where N is an integer.
-//
-// The linker is not capable to handle RC files directly. Instead, it runs
-// cvtres.exe on RC files and then then link its outputs.
-class ResourceFileReader : public Reader {
-public:
-  bool canParse(file_magic magic, StringRef ext,
-                const MemoryBuffer &) const override {
-    return (magic == llvm::sys::fs::file_magic::windows_resource);
-  }
-
-  std::error_code
-  parseFile(std::unique_ptr<MemoryBuffer> &mb, const class Registry &,
-            std::vector<std::unique_ptr<File>> &result) const override {
-    // Convert RC file to COFF
-    ErrorOr<std::string> coffPath = convertResourceFileToCOFF(std::move(mb));
-    if (std::error_code ec = coffPath.getError())
-      return ec;
-    llvm::FileRemover coffFileRemover(*coffPath);
-
-    // Read and parse the COFF
-    ErrorOr<std::unique_ptr<MemoryBuffer>> newmb =
-        MemoryBuffer::getFile(*coffPath);
-    if (std::error_code ec = newmb.getError())
-      return ec;
-    std::error_code ec;
-    std::unique_ptr<FileCOFF> file(new FileCOFF(std::move(newmb.get()), ec));
-    if (ec)
-      return ec;
-    if (std::error_code ec = file->parse())
-      return ec;
-    result.push_back(std::move(file));
-    return std::error_code();
-  }
-
-private:
-  static ErrorOr<std::string>
-  writeResToTemporaryFile(std::unique_ptr<MemoryBuffer> mb) {
-    // Get a temporary file path for .res file.
-    SmallString<128> tempFilePath;
-    if (std::error_code ec =
-            llvm::sys::fs::createTemporaryFile("tmp", "res", tempFilePath))
-      return ec;
-
-    // Write the memory buffer contents to .res file, so that we can run
-    // cvtres.exe on it.
-    std::unique_ptr<llvm::FileOutputBuffer> buffer;
-    if (std::error_code ec = llvm::FileOutputBuffer::create(
-            tempFilePath.str(), mb->getBufferSize(), buffer))
-      return ec;
-    memcpy(buffer->getBufferStart(), mb->getBufferStart(), mb->getBufferSize());
-    if (std::error_code ec = buffer->commit())
-      return ec;
-
-    // Convert SmallString -> StringRef -> std::string.
-    return tempFilePath.str().str();
-  }
-
-  static ErrorOr<std::string>
-  convertResourceFileToCOFF(std::unique_ptr<MemoryBuffer> mb) {
-    // Write the resource file to a temporary file.
-    ErrorOr<std::string> inFilePath = writeResToTemporaryFile(std::move(mb));
-    if (std::error_code ec = inFilePath.getError())
-      return ec;
-    llvm::FileRemover inFileRemover(*inFilePath);
-
-    // Create an output file path.
-    SmallString<128> outFilePath;
-    if (std::error_code ec =
-            llvm::sys::fs::createTemporaryFile("tmp", "obj", outFilePath))
-      return ec;
-    std::string outFileArg = ("/out:" + outFilePath).str();
-
-    // Construct CVTRES.EXE command line and execute it.
-    std::string program = "cvtres.exe";
-    std::string programPath = llvm::sys::FindProgramByName(program);
-    if (programPath.empty()) {
-      llvm::errs() << "Unable to find " << program << " in PATH\n";
-      return llvm::errc::broken_pipe;
-    }
-    std::vector<const char *> args;
-    args.push_back(programPath.c_str());
-    args.push_back("/machine:x86");
-    args.push_back("/readonly");
-    args.push_back("/nologo");
-    args.push_back(outFileArg.c_str());
-    args.push_back(inFilePath->c_str());
-    args.push_back(nullptr);
-
-    DEBUG({
-      for (const char **p = &args[0]; *p; ++p)
-        llvm::dbgs() << *p << " ";
-      llvm::dbgs() << "\n";
-    });
-
-    if (llvm::sys::ExecuteAndWait(programPath.c_str(), &args[0]) != 0) {
-      llvm::errs() << program << " failed\n";
-      return llvm::errc::broken_pipe;
-    }
-    return outFilePath.str().str();
-  }
-};
-
 class COFFObjectReader : public Reader {
 public:
   COFFObjectReader(PECOFFLinkingContext &ctx) : _ctx(ctx) {}
@@ -1204,7 +1098,4 @@ void Registry::addSupportCOFFObjects(PECOFFLinkingContext &ctx) {
                kindStringsAMD64);
 }
 
-void Registry::addSupportWindowsResourceFiles() {
-  add(std::unique_ptr<Reader>(new ResourceFileReader()));
-}
 }

lld/test/pecoff/resource.test

@@ -1,9 +1,15 @@
 # REQUIRES: winres
 
 # RUN: yaml2obj %p/Inputs/nop.obj.yaml > %t.obj
-#
+
 # RUN: lld -flavor link /out:%t.exe /subsystem:console /entry:start /opt:noref \
 # RUN:   -- %t.obj %p/Inputs/resource.res
 
 # Check if the binary contains UTF-16LE string "Hello" copied from resource.res.
 # RUN: cat %t.exe | grep 'H.e.l.l.o'
+
+# RUN: lld -flavor link /out:%t.exe /subsystem:console /entry:start /opt:noref \
+# RUN:   -- %t.obj %p/Inputs/resource.res %p/Inputs/resource.res
+
+# Multiple resource files are concatenated.
+# RUN: cat %t.exe | grep 'H.e.l.l.o.H.e.l.l.o.'

lld/unittests/DriverTests/WinLinkDriverTest.cpp

@@ -64,7 +64,6 @@ TEST_F(WinLinkParserTest, Basic) {
   EXPECT_TRUE(_context.isTerminalServerAware());
   EXPECT_TRUE(_context.getDynamicBaseEnabled());
   EXPECT_TRUE(_context.getCreateManifest());
-  EXPECT_EQ("a.exe.manifest", _context.getManifestOutputPath());
   EXPECT_EQ("", _context.getManifestDependency());
   EXPECT_FALSE(_context.getEmbedManifest());
   EXPECT_EQ(1, _context.getManifestId());
@@ -595,24 +594,6 @@ TEST_F(WinLinkParserTest, Manifest_No) {
   EXPECT_FALSE(_context.getCreateManifest());
 }
 
-TEST_F(WinLinkParserTest, Manifest_Embed) {
-  EXPECT_TRUE(parse("link.exe", "/manifest:embed", "a.out", nullptr));
-  EXPECT_TRUE(_context.getCreateManifest());
-  EXPECT_TRUE(_context.getEmbedManifest());
-  EXPECT_EQ(1, _context.getManifestId());
-  EXPECT_EQ("'asInvoker'", _context.getManifestLevel());
-  EXPECT_EQ("'false'", _context.getManifestUiAccess());
-}
-
-TEST_F(WinLinkParserTest, Manifest_Embed_ID42) {
-  EXPECT_TRUE(parse("link.exe", "/manifest:embed,id=42", "a.out", nullptr));
-  EXPECT_TRUE(_context.getCreateManifest());
-  EXPECT_TRUE(_context.getEmbedManifest());
-  EXPECT_EQ(42, _context.getManifestId());
-  EXPECT_EQ("'asInvoker'", _context.getManifestLevel());
-  EXPECT_EQ("'false'", _context.getManifestUiAccess());
-}
-
 TEST_F(WinLinkParserTest, Manifestuac_no) {
   EXPECT_TRUE(parse("link.exe", "/manifestuac:NO", "a.out", nullptr));
   EXPECT_FALSE(_context.getManifestUAC());