[PATCH] D49366: [LLD][COFF] Add support for /FUNCTIONPADMIN command-line option

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: COFF/Chunks.cpp:609
+  if (Data.size() < 4)
+    fatal("The section is too short. " + SectionName);
+
----------------
ruiu wrote:
> Error messages should start with lowercase letter.
`too short.` -> `too short:`.


================
Comment at: COFF/Chunks.cpp:609-621
+    fatal("The section is too short. " + SectionName);
+
+  if (!SectionName.startswith(".debug$"))
+    fatal("Invalid section. " + SectionName);
+
+  unsigned Magic = support::endian::read32le(Data.data());
+
----------------
Error messages should start with lowercase letter.


================
Comment at: COFF/Chunks.cpp:612
+  if (!SectionName.startswith(".debug$"))
+    fatal("Invalid section. " + SectionName);
+
----------------
. -> :


================
Comment at: COFF/Chunks.cpp:617-618
+  if (SectionName == ".debug$H") {
+    if (Magic != DEBUG_HASHES_SECTION_MAGIC)
+      fatal("Section has an invalid magic. " + SectionName);
+  } else {
----------------
Ditto


================
Comment at: COFF/Chunks.cpp:619-620
+      fatal("Section has an invalid magic. " + SectionName);
+  } else {
+    if (Magic != DEBUG_SECTION_MAGIC)
+      fatal("Section has an invalid magic. " + SectionName +
----------------
`else } if` -> `else if`


================
Comment at: COFF/Driver.cpp:1387-1390
+  // Handle /functionpadmin
+  for (auto *Arg : Args.filtered(OPT_functionpadmin))
+    parseFunctionPadMin(Arg->getValue(), Config->Machine);
+
----------------
This place does not seem like a right place to add this code. We parse command line arguments much earlier.


================
Comment at: COFF/InputFiles.cpp:602
+ArrayRef<uint8_t> ObjFile::getDebugSection(StringRef SecName) {
+  if (SectionChunk *Sec = SectionChunk::findByName(DebugChunks, SecName)) {
+    return Sec->consumeDebugMagic();
----------------
nit: omit {}


================
Comment at: COFF/InputFiles.cpp:608
+
+// OBJ files systematically store critical informations in a .debug$S stream,
+// even if the TU was compiled with no debug info. At least two records are
----------------
Can you mention that this function initializes `PCHSignature` and `HotPatchable` by reading `.debug$S` seciton?


================
Comment at: COFF/Options.td:35
 def failifmismatch : P<"failifmismatch", "">;
+def functionpadmin : Joined<["/", "-", "-?"], "functionpadmin">, HelpText<"Prepares an image for hotpatching">;
 def guard   : P<"guard", "Control flow guard">;
----------------
Newline before `HelpText` just like other definitions.


================
Comment at: test/COFF/functionpadmin.test:1
+
+// ---- precomp-a.obj - x86_64, hotpatch
----------------
Blank line?



Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49366/new/

https://reviews.llvm.org/D49366