[PATCH] D34525: Replace trivial use of external rc.exe by writing our own .res file.

Zachary Turner via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jun 22 20:04:00 PDT 2017


zturner added inline comments.


================
Comment at: lld/COFF/DriverUtils.cpp:394
+  Buf += sizeof(COFF::WinResMagic);
+  Buf += object::WIN_RES_NULL_ENTRY_SIZE;
+}
----------------
This is going to trigger the msan bot because we'll be writing uninitialized memory.  You probably need to write 0s here.


================
Comment at: lld/COFF/DriverUtils.cpp:426-427
+  // Strip all newlines from the Manifest.
+  Manifest.erase(std::remove(Manifest.begin(), Manifest.end(), '\n'),
+                 Manifest.end());
 
----------------
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)


================
Comment at: llvm/unittests/BinaryFormat/TestFileMagic.cpp:79
     "\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[] =
----------------
This is modified but no test is modified.  Is that normal?


================
Comment at: llvm/unittests/BinaryFormat/TestFileMagic.cpp:125
     file.close();
+    llvm::errs() << magic << "\n";
     EXPECT_EQ(i->magic, identify_magic(magic));
----------------
Should this be removed?


https://reviews.llvm.org/D34525





More information about the llvm-commits mailing list