[PATCH] [WIP] MSVC FPO debug information

[David Majnemer]

================
Comment at: include/llvm/CodeGen/MachineModuleInfo.h:51-53
@@ +50,5 @@
+struct MCWinFPOEntry {
+    MCWinFPOEntry(SmallString<64> str) : str(str) {
+    }
+    SmallString<64> str;
+    // XXX: If we move this out of here into the main function
----------------
Variable names should be upper case.  Also, why not let the constructor take a `const Twine &` instead of `SmallString`?

================
Comment at: include/llvm/Target/TargetOpcodes.h:125
@@ -124,2 +124,3 @@
   FRAME_ALLOC = 21,
+  FPO_INSTRUCTION = 20,
 };
----------------
`FPO_INSTRUCTION` is assigned the same value as `STATEPOINT`.  Is this intentional?

================
Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.cpp:81-82
@@ -80,1 +80,4 @@
 
+void WinCodeViewLineTables::recordFPOEntry(MCWinFPOEntry *Entry)
+{
+  assert(CurFn);
----------------
The curly bracne shouldn't be on its own line.

================
Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.cpp:193-194
@@ +192,4 @@
+
+void WinCodeViewLineTables::emitFPOSegment(const MCWinFPOEntry *entry, bool entrypoint, const MCSymbol *Fn, const MCSymbol *From, const MCSymbol *To)
+{
+/* We should be able to set LocalsSize, ParamSize, MaxStackSize, SavedRegSize and PrologSize
----------------
Please clang-format this.

================
Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.cpp:197-222
@@ +196,28 @@
+   all to 0 without any real problems. */
+    //FPOSegment [
+      //Start: 0x0
+      EmitLabelDiff(*Asm->OutStreamer, Fn, From);
+      //CodeSize: 0x14
+      EmitLabelDiff(*Asm->OutStreamer, From, To);
+      //LocalsSize: 0x0 // often 0 but not always
+      Asm->EmitInt32(0);
+      //ParamsSize: 0x0 //  ParamsSize stays the same for nested blocks
+      Asm->EmitInt32(0);
+      //MaxStackSize: 0x0 // always seems to be 0
+      Asm->EmitInt32(0);
+      //ProgramStr: $T0 $ebp = $eip $T0 4 + ^ = $ebp $T0 ^ = $esp $
+      //Asm->EmitInt32(0);
+      Asm->EmitInt32(FileNameRegistry.Infos[entry->str].StartOffset);
+// PrologSize is how much of the prolog is in the current block
+// i.e. if the PrologSize is 9 bytes and the second block begins at byte 5 it will have
+// a prolog size of 4 bytes
+      //PrologSize: 0x3
+      Asm->EmitInt16(0);
+      //SavedRegSize: 0x0 // This goes up predictably
+      Asm->EmitInt16(0);
+      /*Flags [ (0x4)
+        FPO_FUNCTION_ENTRY (0x4)
+      ]*/
+      Asm->EmitInt32(entrypoint ? 0x4 : 0);
+    //]
+}
----------------
These comments are very hard for me to parse.  Can you have a single block comment?

================
Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.cpp:226-227
@@ +225,4 @@
+
+void WinCodeViewLineTables::emitFrameInfo(const MCSymbol *Fn, const FunctionInfo &FI)
+{
+  Asm->OutStreamer->AddComment("FPO subsection");
----------------
Please clang-format this.

================
Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.cpp:241-256
@@ +240,18 @@
+
+  bool start = true;
+  // Frame.begin is no good. We need the actual function
+  // Frame.begin just points to some random label
+  Asm->OutStreamer->EmitCOFFSecRel32(Fn);
+  for (auto entry = FI.FPOEntries.begin(); entry != FI.FPOEntries.end(); ) {
+      auto next = entry+1;
+      MCSymbol *end;
+      if (next == FI.FPOEntries.end()) {
+          end = FI.End;
+      } else {
+          end = (*next)->Label;
+      }
+      emitFPOSegment(*entry, start, Fn, (*entry)->Label, end);
+      start = false;
+      entry = next;
+  }
+
----------------
Variables start with a capital letter.  Also, please avoid recalculating the end iterator on each iteration.

================
Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.cpp:288
@@ -202,2 +287,3 @@
     FuncName = GVName.substr(1);
+
   // Emit a symbol subsection, required by VS2012+ to find function boundaries.
----------------
This change seems unrelated.

================
Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.h:41
@@ -40,2 +40,3 @@
     MCSymbol *End;
+    SmallVector<MCWinFPOEntry*, 10> FPOEntries;
     FunctionInfo() : End(nullptr) {}
----------------
Please have a space before the * token.

================
Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.h:115
@@ -113,1 +114,3 @@
 
+  void emitFPOSegment(const MCWinFPOEntry *entry, bool entrypoint, const MCSymbol *Fn, const MCSymbol *From, const MCSymbol *To);
+  void emitFrameInfo(const MCSymbol *Fn, const FunctionInfo &FI);
----------------
Please clang-format this line.

================
Comment at: lib/MC/MCStreamer.cpp:23
@@ -22,2 +22,3 @@
 #include "llvm/MC/MCWin64EH.h"
+#include "llvm/MC/MCWinFPO.h"
 #include "llvm/Support/ErrorHandling.h"
----------------
Why did you have to make this change?

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:586
@@ -585,2 +585,3 @@
   bool NeedsWinEH = IsWinEH && Fn->needsUnwindTableEntry();
+  bool NeedsWinFPO = true;
   bool NeedsDwarfCFI =
----------------
You never reassign to this variable.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:1108
@@ -1036,2 +1107,3 @@
          "We shouldn't have allowed this insertion point");
+  bool NeedsWinFPO = true;
 
----------------
You never reassign to this variable.

http://reviews.llvm.org/D10432

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/