[PATCH] D104246: X86: balance the frame prologue and epilogue on Win64

[Reid Kleckner via Phabricator via llvm-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm

The code change is just adjusting some if conditions and changing indentation, so there's no reason to hold it up with my design questions. Please consider implementing the refactoring suggestion as a follow-up, though.



================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:1486
+    if (!IsFunclet) {
+      if (X86FI->hasSwiftAsyncContext()) {
+        const auto &Attrs = MF.getFunction().getAttributes();
----------------
As a style comment, I think this code would be easier to read if it was moved out of line. The `emitPrologue`/`emitEpilog` functions are really, really long. Anything you can do to shorten them helps with understanding. If you can pair the swift async context setup and teardown code somewhere later in the file, it would help. The main state going in and out is `MBB, MBBI, DL`. See `BuildStackAlignAND` for an example of this.


================
Comment at: llvm/test/CodeGen/X86/swift-async-win64.ll:9-10
+; CHECK64-LABEL: simple:
+; CHECK64: btsq    $60, %rbp
+; CHECK64: pushq   %rbp
+; CHECK64: pushq   %r14
----------------
Maybe I misunderstand, but won't this prevent every existing Windows unwinder, debuggers included, from unwinding the native stack? BTS *sets* bit 60 of RBP before it is pushed. If the caller's frame uses RBP as a frame pointer, how will a Swift-unaware stack unwinder use RBP to continue unwinding? It won't know to clear out bit 60.

Is it really the plan to make it so that you cannot debug Swift code from a Swift-unaware debugger? I'm not sure LLDB on Windows is really ready, so I imagine that folks will want to use windbg to debug Swift code in the near future. This isn't a Windows-specific concern either. Anyone using gdb, rr, perf, and other stack samplers on Linux will have similar issues, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104246