[PATCH] D42592: [COFF] Add minimal support for /guard:cf

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/COFF/Chunks.cpp:459
   size_t Cnt = 0;
-  for (Defined *D : Syms)
-    Begin[Cnt++] = D->getRVA();
+  for (const auto &P : Syms)
+    Begin[Cnt++] = P.first->getRVA() + P.second;
----------------
Please avoid `auto` even if writing a real type is a bit too lengthy. If you want to use `auto`, please do something like this so that a real type is explicit in a local context.

  for (const auto &P : Syms) {
    Defined *Sym = P.first;
    ....
  }


================
Comment at: lld/COFF/DriverUtils.cpp:132-136
+  std::string Lower = Arg.lower();
+  if (Lower == "no")
+    Config->GuardCF = false;
+  else if (Lower == "cf")
+    Config->GuardCF = true;
----------------
You can use StringRef::equals_lower.


================
Comment at: lld/COFF/InputFiles.h:104-109
+// These are the flags set in the @feat.00 absolute symbol. Only bit 1 is
+// documented, so the names are our own.
+enum class CoffFeat00Flags : uint32_t {
+  SafeSEH = 0x00000001,
+  GuardCF = 0x00000800,
+};
----------------
Since these enums are used only by ObjFile, I'd move this into that class (and make them a non-enum class.)


================
Comment at: lld/COFF/InputFiles.h:142-149
+  bool hasSafeSEH() {
+    return Feat00Flags & uint32_t(CoffFeat00Flags::SafeSEH);
+  }
+
+  // True if this file was compiled with /guard:cf.
+  bool hasGuardCF() {
+    return Feat00Flags & uint32_t(CoffFeat00Flags::GuardCF);
----------------
Actually, I'd even use 1 and 0x800 instead of defining them as constants, as they are used only once and already factored out well.


================
Comment at: lld/COFF/Writer.cpp:874
 
+void Writer::createGFIDTable(OutputSection *RData) {
+  SymbolRVASet AddressTakenSyms;
----------------
Can you write a function comment to give a big picture? Once it becomes clear as to what this function should create, the details becomes obvious.


https://reviews.llvm.org/D42592