[PATCH] D38776: [codeview] Implement FPO data assembler directives

[Reid Kleckner via Phabricator via llvm-commits]

rnk marked 4 inline comments as done.
rnk added inline comments.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86WinCOFFTargetStreamer.cpp:263-266
+    // Otherwise, get the codeview register number and print $N.
+    default:
+      OS << '$' << MRI->getCodeViewRegNum(LLVMReg);
+      break;
----------------
majnemer wrote:
> majnemer wrote:
> > Shouldn't X86::EAX be $eax, etc?
> Woah, apparently *both* work:
> 
> https://github.com/GPUOpen-Tools/CodeXL/blob/6f0aa6b42ae47028981058ccc3c505a5884d2c2e/CodeXL/Components/CpuProfiling/AMDTCpuCallstackSampling/src/StackWalker/x86/PostfixFrameEvaluator.cpp#L129
> https://github.com/GPUOpen-Tools/CodeXL/blob/6f0aa6b42ae47028981058ccc3c505a5884d2c2e/CodeXL/Components/CpuProfiling/AMDTCpuCallstackSampling/src/StackWalker/x86/PostfixFrameEvaluator.cpp#L188
> 
> I guess this way is more general?
BTW, breakpad provides some other FPO data references:
https://chromium.googlesource.com/breakpad/breakpad/+/master/src/processor/stackwalker_x86.cc

I guess I'll put all the GPRs in here, but leave the CV regnum fallback. Realistically only these registers and the other CSRs (ESI, EDI, and EBX) will appear in these postfix programs.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86WinCOFFTargetStreamer.cpp:315
+  OS.EmitIntValue(LocalSize, 4);
+  OS.EmitIntValue(0, 4); // FIXME: ParamsSize
+  OS.EmitIntValue(0, 4); // MaxStackSize
----------------
majnemer wrote:
> How would you extend the assembler directive to support this?
I added a number to `.cv_proc` for this. Might as well do it now.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86WinCOFFTargetStreamer.cpp:316
+  OS.EmitIntValue(0, 4); // FIXME: ParamsSize
+  OS.EmitIntValue(0, 4); // MaxStackSize
+  OS.EmitIntValue(FrameFuncStrTabOff, 4); // FrameFunc
----------------
majnemer wrote:
> Does MSVC stick anything interesting in here? What's it for?
I've only seen zeros so far. I think you'd have to screw around with dynamic allocas to make it do something.


================
Comment at: llvm/test/MC/COFF/cv-fpo-setframe.s:48
+	# Epilogue
+	# FIXME: Get FPO data for this once we get it for DWARF.
+	addl $20, %esp
----------------
majnemer wrote:
> How would this work with this FPO format?
The FrameData records act like nested scopes, where inner ones override the outer ones. They seem to assume the epilogue is at the end of the function, and the PC ranges look like nested intervals like this:
    FrameData {
      RvaStart: 0x0
      CodeSize: 0x3C
    FrameData {
      RvaStart: 0x1
      CodeSize: 0x3A
    FrameData {
      RvaStart: 0x2
      CodeSize: 0x38
    FrameData {
      RvaStart: 0x3
      CodeSize: 0x36
    FrameData {
      RvaStart: 0x4
      CodeSize: 0x34

The start increases by one each time as it steps over each `push`, but the size decreases by two, which uncovers the corresponding `pop`.

Obviously, this is problematic in the face of LLVM's tail duplication and mid-function epilogues, so I doubt it'll ever get done.


https://reviews.llvm.org/D38776