Replace trivial use of external rc.exe by writing our own .res file.

[ecbeckmann]

Summary

This patch removes the dependency on the external rc.exe tool by writing a simple .res file using our own library. In this patch I also added an explicit definition for the .res file magic.

[ruiu]

I think a better way of creating manifest file contents is to construct a temporary std::vector and then copy it to a MemoryBuffer after you append everything to the vector. This way, you don't need to compute the size of the result ahead of time.

lld/COFF/DriverUtils.cpp

380

s/get/create/ as it creates a new instance of MemoryBuffer.

381–386

It feels to me that this is cleaner.

size_t Size = alignTo(object::WIN_RES_MAGIC_SIZE +
                      object::WIN_RES_NULL_ENTRY_SIZE +
                      sizeof(object::WinResHeaderPrefix) +
                      sizeof(object::WinResIDs) +
                      sizeof(object::WinResHeaderSuffix) +
                      Manifest.size(),
                      object::WIN_RES_DATA_ALIGNMENT);

426–428

Why do you want to strip all newlines?

427–436

Since you are not using the contents of Manifest, pass Manifest.size() instead of Manifest.

429

Rename Current Buf and use uint8_t instead of char for consistency

llvm/include/llvm/BinaryFormat/COFF.h

50

Since all the other Magics are of const char[], you want to use char.

llvm/include/llvm/Object/WindowsResource.h

49–88

Remove blank lines. Replace sizeof(uint32_t) with 4 because it is always 4.

67–72

You are not really using this struct. If you need only the size of the struct, define it as a constant.

[ecbeckmann]

In D34525#788360, @ruiu wrote:

I think a better way of creating manifest file contents is to construct a temporary std::vector and then copy it to a MemoryBuffer after you append everything to the vector. This way, you don't need to compute the size of the result ahead of time.

Hmmm won't this method require an extra copy? Also, it seems less convenient and more messy to have all these resize() operations before we write anything.

[ruiu]

It indeed will require extra copy, but that is fine. You don't need to avoid extra copy here since the generated file image is small and this is executed just once.

You can use push_back or something to avoid resize, no? Or, you can use an OutputStream backed up by some buffer and write data using operator<< there.

[ecbeckmann]

Made some style changes. Also fixed bug in magic test where the full .res magic number was not recognized.

[ecbeckmann]

lld/COFF/DriverUtils.cpp

426–428

This mimics the behavior of rc.exe, which strips all the newlines from a manifest before putting it in the .res file. However, spaces are left intact.

429

Hmmm there is already a variable named "Buf".

429

Also const_char to type uint8_t* is not possible, and requires an additional cast.

llvm/include/llvm/Object/WindowsResource.h

67–72

I do use in in DriverUtils.cpp for a reinterpret_cast. It helps I think for writing the resource header.

[ruiu]

lld/COFF/DriverUtils.cpp

426–428

If you don't do this and it still works, don't do that. We don't have to mimic all the details.

429

Then please rename the conflicting one. Buf is used as a pointer to uint8_t in LLD very often, so not naming it is a bit confusing.

llvm/include/llvm/Object/WindowsResource.h

67–72

Then maybe you want to remove the default value 0xffff. You are not using it.

[ecbeckmann]

In D34525#788514, @ruiu wrote:

It indeed will require extra copy, but that is fine. You don't need to avoid extra copy here since the generated file image is small and this is executed just once.

You can use push_back or something to avoid resize, no? Or, you can use an OutputStream backed up by some buffer and write data using operator<< there.

How do we use push_back when we are adding all these elements of different sizes? However using a stream may be best.

[ruiu]

Well, operator<< should still work, but if doing that makes code messy, you don't have to do that.

[ecbeckmann]

In D34525#788612, @ruiu wrote:

Well, operator<< should still work, but if doing that makes code messy, you don't have to do that.

Hmm yes I don't think using '<<' necessarily makes it easier. Also reinterpret_cast, increment pointer pattern is well used in LLD already.

[zturner]

lld/COFF/DriverUtils.cpp

394

This is going to trigger the msan bot because we'll be writing uninitialized memory. You probably need to write 0s here.

427–428

Yea I would rather not do this as well. (FWIW, you also haven't handled carriage returns, which are likely to be present, so to keep it simple let's just leave it as is)

llvm/unittests/BinaryFormat/TestFileMagic.cpp

79–80

This is modified but no test is modified. Is that normal?

126

Should this be removed?

[ecbeckmann]

Removed new line stripping for manifests, initialize 0s for null entry.

llvm/include/llvm/Object/WindowsResource.h

67–72

Oh shoot you're right. These fields must be set to 0xffff when the Type and Names are IDs, however. I should make a method for setting them.

llvm/unittests/BinaryFormat/TestFileMagic.cpp

79–80

The change was as follows: I added the WinResMagic constant definition in BinaryFormat/COFF.h. I then modified Magic.cpp to compare based on this constant instead of it's previous method of comparing to a string which, in fact, did not contain all the magic bytes. This broke the test, because the test magic didn't contain the full signature, so it would fail the comparison in Magic.cpp

[ruiu]

Looks much better. A few minor comments.

llvm/include/llvm/Object/WindowsResource.h

70

Instead of defining setType and setName, you can just set TypeFlag and NameID to 0xffff in writeResEntryHeader, no?

llvm/lib/BinaryFormat/Magic.cpp

54–55

You want to make sure that Magic.data() is at least the same size as WinResMagic.

[ecbeckmann]

Check size of magic before comparing

llvm/include/llvm/Object/WindowsResource.h

70

I think it would be better if this level were abstracted away from the user class. After all, it is a detail of the Type and Name fields that the most significant bytes are always set to high when number IDs are used. This is an implementation detail about the .res spec that should be hidden.

[ruiu]

LGTM

[ecbeckmann]

Fix a bug where incorrect characteristic flags are set in section header.

[ecbeckmann]

Added a unittest for the embedded manifest case.

[ecbeckmann]

Fixed formatting issue.

[ruiu]

Still LGTM

lld/COFF/DriverUtils.cpp

 
 #include "Config.h"
 #include "Driver.h"
 #include "Error.h"
 #include "Memory.h"
 #include "Symbols.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/BinaryFormat/COFF.h"
 #include "llvm/Object/COFF.h"
 #include "llvm/Object/WindowsResource.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileUtilities.h"
+#include "llvm/Support/MathExtras.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/Program.h"
 #include "llvm/Support/raw_ostream.h"
 #include <memory>
 
 using namespace llvm::COFF;
 using namespace llvm;
 using llvm::cl::ExpandResponseFiles;
 using llvm::cl::TokenizeWindowsCommandLine;
 using llvm::sys::Process;
 
 namespace lld {
 namespace coff {
 namespace {
 
+const uint16_t SUBLANG_ENGLISH_US = 0x0409;
+const uint16_t RT_MANIFEST = 24;
+
 class Executor {
 public:
   explicit Executor(StringRef S) : Prog(Saver.save(S)) {}
   void add(StringRef S) { Args.push_back(Saver.save(S)); }
   void add(std::string &S) { Args.push_back(Saver.save(S)); }
   void add(Twine S) { Args.push_back(Saver.save(S)); }
   void add(const char *S) { Args.push_back(Saver.save(S)); }
 
if (Arg.startswith_lower("uiaccess=")) {
       Arg = Arg.substr(strlen("uiaccess="));
       std::tie(Config->ManifestUIAccess, Arg) = Arg.split(" ");
       continue;
     }
     fatal("invalid option " + Arg);
   }
 }
 
-// Quote each line with "". Existing double-quote is converted
-// to two double-quotes.
-static void quoteAndPrint(raw_ostream &Out, StringRef S) {
-  while (!S.empty()) {
-    StringRef Line;
-    std::tie(Line, S) = S.split("\n");
-    if (Line.empty())
-      continue;
-    Out << '\"';
-    for (int I = 0, E = Line.size(); I != E; ++I) {
-      if (Line[I] == '\"') {
-        Out << "\"\"";
-      } else {
-        Out << Line[I];
-      }
-    }
-    Out << "\"\n";
-  }
-}
-
 // An RAII temporary file class that automatically removes a temporary file.
 namespace {
 class TemporaryFile {
 public:
   TemporaryFile(StringRef Prefix, StringRef Extn, StringRef Contents = "") {
     SmallString<128> S;
     if (auto EC = sys::fs::createTemporaryFile("lld-" + Prefix, Extn, S))
       fatal(EC, "cannot create a temporary file");
for (StringRef Filename : Config->ManifestInput) {
     E.add(Filename);
   }
   E.add("/nologo");
   E.add("/out:" + StringRef(File2.Path));
   E.run();
   return readFile(File2.Path);
 }
 
+static std::unique_ptr<MemoryBuffer>
+createMemoryBufferForManifestRes(size_t ManifestSize) {
+  size_t ResSize = alignTo(object::WIN_RES_MAGIC_SIZE +
+                           object::WIN_RES_NULL_ENTRY_SIZE +
+                           sizeof(object::WinResHeaderPrefix) +
+                           sizeof(object::WinResIDs) +
+                           sizeof(object::WinResHeaderSuffix) +
+                           ManifestSize,
+                           object::WIN_RES_DATA_ALIGNMENT);
+  return MemoryBuffer::getNewMemBuffer(ResSize);
+}
+
+static void writeResFileHeader(char *&Buf) {
+  memcpy(Buf, COFF::WinResMagic, sizeof(COFF::WinResMagic));
+  Buf += sizeof(COFF::WinResMagic);
+  memset(Buf, 0, object::WIN_RES_NULL_ENTRY_SIZE);
+  Buf += object::WIN_RES_NULL_ENTRY_SIZE;
+}
+
+static void writeResEntryHeader(char *&Buf, size_t ManifestSize) {
+  // Write the prefix.
+  auto *Prefix = reinterpret_cast<object::WinResHeaderPrefix *>(Buf);
+  Prefix->DataSize = ManifestSize;
+  Prefix->HeaderSize = sizeof(object::WinResHeaderPrefix) +
+                       sizeof(object::WinResIDs) +
+                       sizeof(object::WinResHeaderSuffix);
+  Buf += sizeof(object::WinResHeaderPrefix);
+
+  // Write the Type/Name IDs.
+  auto *IDs = reinterpret_cast<object::WinResIDs *>(Buf);
+  IDs->setType(RT_MANIFEST);
+  IDs->setName(Config->ManifestID);
+  Buf += sizeof(object::WinResIDs);
+
+  // Write the suffix.
+  auto *Suffix = reinterpret_cast<object::WinResHeaderSuffix *>(Buf);
+  Suffix->DataVersion = 0;
+  Suffix->MemoryFlags = object::WIN_RES_PURE_MOVEABLE;
+  Suffix->Language = SUBLANG_ENGLISH_US;
+  Suffix->Version = 0;
+  Suffix->Characteristics = 0;
+  Buf += sizeof(object::WinResHeaderSuffix);
+}
+
 // Create a resource file containing a manifest XML.
 std::unique_ptr<MemoryBuffer> createManifestRes() {
-  // Create a temporary file for the resource script file.
-  TemporaryFile RCFile("manifest", "rc");
+  std::string Manifest = createManifestXml();
 
-  // Open the temporary file for writing.
-  std::error_code EC;
-  raw_fd_ostream Out(RCFile.Path, EC, sys::fs::F_Text);
-  if (EC)
-    fatal(EC, "failed to open " + RCFile.Path);
+  std::unique_ptr<MemoryBuffer> Res =
+      createMemoryBufferForManifestRes(Manifest.size());
 
-  // Write resource script to the RC file.
-  Out << "#define LANG_ENGLISH 9\n"
-      << "#define SUBLANG_DEFAULT 1\n"
-      << "#define APP_MANIFEST " << Config->ManifestID << "\n"
-      << "#define RT_MANIFEST 24\n"
-      << "LANGUAGE LANG_ENGLISH, SUBLANG_DEFAULT\n"
-      << "APP_MANIFEST RT_MANIFEST {\n";
-  quoteAndPrint(Out, createManifestXml());
-  Out << "}\n";
-  Out.close();
-
-  // Create output resource file.
-  TemporaryFile ResFile("output-resource", "res");
-
-  Executor E("rc.exe");
-  E.add("/fo");
-  E.add(ResFile.Path);
-  E.add("/nologo");
-  E.add(RCFile.Path);
-  E.run();
-  return ResFile.getMemoryBuffer();
+  char *Buf = const_cast<char *>(Res->getBufferStart());
+  writeResFileHeader(Buf);
+  writeResEntryHeader(Buf, Manifest.size());
+
+  // Copy the manifest data into the .res file.
+  std::copy(Manifest.begin(), Manifest.end(), Buf);
+  return std::move(Res);
 }
 
 void createSideBySideManifest() {
   std::string Path = Config->ManifestFile;
   if (Path == "")
     Path = Config->OutputFile + ".manifest";
   std::error_code EC;
   raw_fd_ostream Out(Path, EC, sys::fs::F_Text);

lld/test/COFF/manifestinput.test

 # REQUIRES: win_mt
 
 # RUN: yaml2obj %p/Inputs/ret42.yaml > %t.obj
 # RUN: lld-link /out:%t.exe /entry:main \
 # RUN:   /manifestuac:"level='requireAdministrator'" \
 # RUN:   /manifestinput:%p/Inputs/manifestinput.test %t.obj
 # RUN: FileCheck %s < %t.exe.manifest
 
 CHECK: <?xml version="1.0" encoding="UTF-8" standalone="yes"?>
 CHECK: <assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0"><dependency><dependentAssembly><assemblyIdentity type="win32" name="Microsoft.Windows.Common-Controls" version="6.0.0.0" processorArchitecture="*" publicKeyToken="6595b64144ccf1df" language="*"></assemblyIdentity></dependentAssembly></dependency><trustInfo><security><requestedPrivileges><requestedExecutionLevel level="requireAdministrator" uiAccess="false"></requestedExecutionLevel></requestedPrivileges></security></trustInfo></assembly>
+
+# RUN: yaml2obj %p/Inputs/ret42.yaml > %t.obj
+# RUN: lld-link /out:%t.exe /entry:main \
+# RUN:   /manifest:embed \
+# RUN:   /manifestuac:"level='requireAdministrator'" \
+# RUN:   /manifestinput:%p/Inputs/manifestinput.test %t.obj
+# RUN: llvm-readobj -coff-resources -file-headers %t.exe | FileCheck %s \
+# RUN:   -check-prefix TEST_EMBED
+
+TEST_EMBED:          ResourceTableRVA: 0x1000
+TEST_EMBED-NEXT:     ResourceTableSize: 0x298
+TEST_EMBED-DAG:      Resources [
+TEST_EMBED-NEXT:       Total Number of Resources: 1 
+TEST_EMBED-DAG:        Number of String Entries: 0
+TEST_EMBED-NEXT:       Number of ID Entries: 1
+TEST_EMBED-NEXT:       Type: kRT_MANIFEST (ID 24) [
+TEST_EMBED-NEXT:         Table Offset: 0x18
+TEST_EMBED-NEXT:         Number of String Entries: 0
+TEST_EMBED-NEXT:         Number of ID Entries: 1
+TEST_EMBED-NEXT:         Name: (ID 1) [
+TEST_EMBED-NEXT:           Table Offset: 0x30
+TEST_EMBED-NEXT:           Number of String Entries: 0
+TEST_EMBED-NEXT:           Number of ID Entries: 1
+TEST_EMBED-NEXT:           Language: (ID 1033) [
+TEST_EMBED-NEXT:             Entry Offset: 0x48

lld/test/lit.cfg

if re.search(r'PowerPC', archs):
     config.available_features.add('ppc')
 if re.search(r'AMDGPU', archs):
     config.available_features.add('amdgpu')
 llvm_config_cmd.wait()
 
 # Set a fake constant version so that we get consitent output.
 config.environment['LLD_VERSION'] = 'LLD 1.0'
 
-# Check if the mt.exe Microsoft utility exists.
-if lit.util.which('mt.exe', config.environment['PATH']):
+# Indirectly check if the mt.exe Microsoft utility exists by searching for
+# cvtres, which always accompanies it.
+if lit.util.which('cvtres', config.environment['PATH']):
     config.available_features.add('win_mt')

llvm/include/llvm/BinaryFormat/COFF.h

static const char BigObjMagic[] = {
     '\xaf', '\x20', '\xfa', '\xf6', '\x6a', '\xa4', '\xdc', '\xb8',
 };
 
 static const char ClGlObjMagic[] = {
     '\x38', '\xfe', '\xb3', '\x0c', '\xa5', '\xd9', '\xab', '\x4d',
     '\xac', '\x9b', '\xd6', '\xb6', '\x22', '\x26', '\x53', '\xc2',
 };
 
+// The signature bytes that start a .res file.
+static const char WinResMagic[] = {
+    '\x00', '\x00', '\x00', '\x00', '\x20', '\x00', '\x00', '\x00',
+    '\xff', '\xff', '\x00', '\x00', '\xff', '\xff', '\x00', '\x00',
+};
+
 // Sizes in bytes of various things in the COFF format.
 enum {
   Header16Size = 20,
   Header32Size = 56,
   NameSize = 8,
   Symbol16Size = 18,
   Symbol32Size = 20,
   SectionSize = 40,

llvm/include/llvm/Object/WindowsResource.h

 #include "llvm/Support/ScopedPrinter.h"
 
 #include <map>
 
 namespace llvm {
 namespace object {
 
 class WindowsResource;
 
+const size_t WIN_RES_MAGIC_SIZE = 16;
+const size_t WIN_RES_NULL_ENTRY_SIZE = 16;
+const uint32_t WIN_RES_HEADER_ALIGNMENT = 4;
+const uint32_t WIN_RES_DATA_ALIGNMENT = 4;
+const uint16_t WIN_RES_PURE_MOVEABLE = 0x0030;
+
+struct WinResHeaderPrefix {
+  support::ulittle32_t DataSize;
+  support::ulittle32_t HeaderSize;
+};
+
+// Type and Name may each either be an integer ID or a string.  This struct is
+// only used in the case where they are both IDs.
+struct WinResIDs {
+  uint16_t TypeFlag;
+  support::ulittle16_t TypeID;
+  uint16_t NameFlag;
+  support::ulittle16_t NameID;
+
+  void setType(uint16_t ID) {
+    TypeFlag = 0xffff;
+    TypeID = ID;
+  }
+
+  void setName(uint16_t ID) {
+    NameFlag = 0xffff;
+    NameID = ID;
+  }
+};
+
+struct WinResHeaderSuffix {
+  support::ulittle32_t DataVersion;
+  support::ulittle16_t MemoryFlags;
+  support::ulittle16_t Language;
+  support::ulittle32_t Version;
+  support::ulittle32_t Characteristics;
+};
+
 class ResourceEntryRef {
 public:
   Error moveNext(bool &End);
   bool checkTypeString() const { return IsStringType; }
   ArrayRef<UTF16> getTypeString() const { return Type; }
   uint16_t getTypeID() const { return TypeID; }
   bool checkNameString() const { return IsStringName; }
   ArrayRef<UTF16> getNameString() const { return Name; }
   uint16_t getNameID() const { return NameID; }
   uint16_t getLanguage() const { return Suffix->Language; }
   uint16_t getMajorVersion() const { return Suffix->Version >> 16; }
   uint16_t getMinorVersion() const { return Suffix->Version; }
   uint32_t getCharacteristics() const { return Suffix->Characteristics; }
   ArrayRef<uint8_t> getData() const { return Data; }
 
 private:
   friend class WindowsResource;
 
   ResourceEntryRef(BinaryStreamRef Ref, const WindowsResource *Owner,
                    Error &Err);
 
   Error loadNext();
 
-  struct HeaderSuffix {
-    support::ulittle32_t DataVersion;
-    support::ulittle16_t MemoryFlags;
-    support::ulittle16_t Language;
-    support::ulittle32_t Version;
-    support::ulittle32_t Characteristics;
-  };
-
   BinaryStreamReader Reader;
   bool IsStringType;
   ArrayRef<UTF16> Type;
   uint16_t TypeID;
   bool IsStringName;
   ArrayRef<UTF16> Name;
   uint16_t NameID;
-  const HeaderSuffix *Suffix = nullptr;
+  const WinResHeaderSuffix *Suffix = nullptr;
   ArrayRef<uint8_t> Data;
   const WindowsResource *OwningRes = nullptr;
 };
 
 class WindowsResource : public Binary {
 public:
   Expected<ResourceEntryRef> getHeadEntry();
 

llvm/lib/BinaryFormat/Magic.cpp

if (startswith(Magic, "\0\0\xFF\xFF")) {
       const char *Start = Magic.data() + offsetof(COFF::BigObjHeader, UUID);
       if (memcmp(Start, COFF::BigObjMagic, sizeof(COFF::BigObjMagic)) == 0)
         return file_magic::coff_object;
       if (memcmp(Start, COFF::ClGlObjMagic, sizeof(COFF::BigObjMagic)) == 0)
         return file_magic::coff_cl_gl_object;
       return file_magic::coff_import_library;
     }
     // Windows resource file
-    if (startswith(Magic, "\0\0\0\0\x20\0\0\0\xFF"))
+    if (Magic.size() >= sizeof(COFF::WinResMagic) &&
+        memcmp(Magic.data(), COFF::WinResMagic, sizeof(COFF::WinResMagic)) == 0)
       return file_magic::windows_resource;
     // 0x0000 = COFF unknown machine type
     if (Magic[1] == 0)
       return file_magic::coff_object;
     if (startswith(Magic, "\0asm"))
       return file_magic::wasm_object;
     break;
   }

llvm/lib/Object/WindowsResource.cpp

if (auto EC = X)                                                             \
     return EC;
 
 const uint32_t MIN_HEADER_SIZE = 7 * sizeof(uint32_t) + 2 * sizeof(uint16_t);
 
 // COFF files seem to be inconsistent with alignment between sections, just use
 // 8-byte because it makes everyone happy.
 const uint32_t SECTION_ALIGNMENT = sizeof(uint64_t);
 
-static const size_t ResourceMagicSize = 16;
-
-static const size_t NullEntrySize = 16;
-
 uint32_t WindowsResourceParser::TreeNode::StringCount = 0;
 uint32_t WindowsResourceParser::TreeNode::DataCount = 0;
 
 WindowsResource::WindowsResource(MemoryBufferRef Source)
     : Binary(Binary::ID_WinRes, Source) {
-  size_t LeadingSize = ResourceMagicSize + NullEntrySize;
+  size_t LeadingSize = WIN_RES_MAGIC_SIZE + WIN_RES_NULL_ENTRY_SIZE;
   BBS = BinaryByteStream(Data.getBuffer().drop_front(LeadingSize),
                          support::little);
 }
 
 Expected<std::unique_ptr<WindowsResource>>
 WindowsResource::createWindowsResource(MemoryBufferRef Source) {
-  if (Source.getBufferSize() < ResourceMagicSize + NullEntrySize)
+  if (Source.getBufferSize() < WIN_RES_MAGIC_SIZE + WIN_RES_NULL_ENTRY_SIZE)
     return make_error<GenericBinaryError>(
         "File too small to be a resource file",
         object_error::invalid_file_type);
   std::unique_ptr<WindowsResource> Ret(new WindowsResource(Source));
   return std::move(Ret);
 }
 
 Expected<ResourceEntryRef> WindowsResource::getHeadEntry() {
if (IsString) {
     RETURN_IF_ERROR(Reader.readWideString(Str));
   } else
     RETURN_IF_ERROR(Reader.readInteger(ID));
 
   return Error::success();
 }
 
 Error ResourceEntryRef::loadNext() {
-  uint32_t DataSize;
-  RETURN_IF_ERROR(Reader.readInteger(DataSize));
-  uint32_t HeaderSize;
-  RETURN_IF_ERROR(Reader.readInteger(HeaderSize));
+  const WinResHeaderPrefix *Prefix;
+  RETURN_IF_ERROR(Reader.readObject(Prefix));
 
-  if (HeaderSize < MIN_HEADER_SIZE)
+  if (Prefix->HeaderSize < MIN_HEADER_SIZE)
     return make_error<GenericBinaryError>("Header size is too small.",
                                           object_error::parse_failed);
 
   RETURN_IF_ERROR(readStringOrId(Reader, TypeID, Type, IsStringType));
 
   RETURN_IF_ERROR(readStringOrId(Reader, NameID, Name, IsStringName));
 
-  RETURN_IF_ERROR(Reader.padToAlignment(sizeof(uint32_t)));
+  RETURN_IF_ERROR(Reader.padToAlignment(WIN_RES_HEADER_ALIGNMENT));
 
   RETURN_IF_ERROR(Reader.readObject(Suffix));
 
-  RETURN_IF_ERROR(Reader.readArray(Data, DataSize));
+  RETURN_IF_ERROR(Reader.readArray(Data, Prefix->DataSize));
 
-  RETURN_IF_ERROR(Reader.padToAlignment(sizeof(uint32_t)));
+  RETURN_IF_ERROR(Reader.padToAlignment(WIN_RES_DATA_ALIGNMENT));
 
   return Error::success();
 }
 
 WindowsResourceParser::WindowsResourceParser() : Root(false) {}
 
 Error WindowsResourceParser::parse(WindowsResource *WR) {
   auto EntryOrErr = WR->getHeadEntry();
void WindowsResourceCOFFWriter::writeFirstSectionHeader() {
   SectionOneHeader->VirtualSize = 0;
   SectionOneHeader->VirtualAddress = 0;
   SectionOneHeader->SizeOfRawData = SectionOneSize;
   SectionOneHeader->PointerToRawData = SectionOneOffset;
   SectionOneHeader->PointerToRelocations = SectionOneRelocations;
   SectionOneHeader->PointerToLinenumbers = 0;
   SectionOneHeader->NumberOfRelocations = Data.size();
   SectionOneHeader->NumberOfLinenumbers = 0;
-  SectionOneHeader->Characteristics = COFF::IMAGE_SCN_ALIGN_1BYTES;
-  SectionOneHeader->Characteristics += COFF::IMAGE_SCN_CNT_INITIALIZED_DATA;
   SectionOneHeader->Characteristics += COFF::IMAGE_SCN_CNT_INITIALIZED_DATA;
   SectionOneHeader->Characteristics += COFF::IMAGE_SCN_MEM_READ;
 }
 
 void WindowsResourceCOFFWriter::writeSecondSectionHeader() {
   // Write the second section header.
   CurrentOffset += sizeof(coff_section);
   auto *SectionTwoHeader =

llvm/unittests/BinaryFormat/TestFileMagic.cpp

 const char macho_dynamic_linker[] =
     "\xfe\xed\xfa\xce........\x00\x00\x00\x07............";
 const char macho_bundle[] =
     "\xfe\xed\xfa\xce........\x00\x00\x00\x08............";
 const char macho_dsym_companion[] =
     "\xfe\xed\xfa\xce........\x00\x00\x00\x0a............";
 const char macho_kext_bundle[] =
     "\xfe\xed\xfa\xce........\x00\x00\x00\x0b............";
-const char windows_resource[] = "\x00\x00\x00\x00\x020\x00\x00\x00\xff";
+const char windows_resource[] =
+    "\x00\x00\x00\x00\x020\x00\x00\x00\xff\xff\x00\x00\xff\xff\x00\x00";
 const char macho_dynamically_linked_shared_lib_stub[] =
     "\xfe\xed\xfa\xce........\x00\x00\x00\x09............";
 
 TEST_F(MagicTest, Magic) {
   struct type {
     const char *filename;
     const char *magic_str;
     size_t magic_str_len;
for (type *i = types, *e = types + (sizeof(types) / sizeof(type)); i != e;
     SmallString<128> file_pathname(TestDirectory);
     llvm::sys::path::append(file_pathname, i->filename);
     std::error_code EC;
     raw_fd_ostream file(file_pathname, EC, sys::fs::F_None);
     ASSERT_FALSE(file.has_error());
     StringRef magic(i->magic_str, i->magic_str_len);
     file << magic;
     file.close();
     EXPECT_EQ(i->magic, identify_magic(magic));
     ASSERT_NO_ERROR(fs::remove(Twine(file_pathname)));
   }
 }