[PATCH] D34072: Fix alignment bug in COFF emission.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: llvm/trunk/lib/Object/WindowsResource.cpp:35
+// 8-byte because it makes everyone happy.
+const uint32_t SECTION_ALIGNMENT = sizeof(uint64_t);
+
----------------
static?

The following two constants are not ALL_IN_UPPERCAPS, but this is. Since it is not a CPP macro or something, this should be SectionAlignment.

Also, why `sizeof(uint64_t)` instead of just 8?


================
Comment at: llvm/trunk/lib/Object/WindowsResource.cpp:469
   // Write the first section header.
-  Current += sizeof(llvm::object::coff_file_header);
-  auto *SectionOneHeader =
-      reinterpret_cast<llvm::object::coff_section *>(Current);
+  CurrentOffset += sizeof(llvm::object::coff_file_header);
+  auto *SectionOneHeader = reinterpret_cast<llvm::object::coff_section *>(
----------------
It is better to increment CurrentOffset in a function that writes something rather than incrementing it in a function that is called after you write it. So move this to writeCOFFHeader.


================
Comment at: llvm/trunk/lib/Object/WindowsResource.cpp:601-603
+  auto COFFStringTable =
+      reinterpret_cast<uint32_t *>(BufferStart + CurrentOffset);
+  *COFFStringTable = 0;
----------------
  write32le(BufferStart + CurrentOffset, 0);


https://reviews.llvm.org/D34072