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

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd added inline comments.


================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:1486
+    if (!IsFunclet) {
+      if (X86FI->hasSwiftAsyncContext()) {
+        const auto &Attrs = MF.getFunction().getAttributes();
----------------
rnk wrote:
> 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.
I completely agree with you on this point.  I really want to see if I can split out the functions somewhat in follow ups.  The reason is that right now the code is really difficult to read as there are the following paths:

- Win32
  * Funclet
  * Non-Funclet
- Win64
  * Funclet
  * Non-Funclet
- Non-Windows

It really feels like this should be three different implementations but I dont fully see how to structure this yet.


================
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
----------------
rnk wrote:
> 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?
Yes, it does break other unwinders AIUI.  The bit 60 is the marker that debugger uses to determine if the frame contains an async context (or NULL), which is expected to be the next value after the base pointer.

Swift debugging is already broken entirely with non-lldb debuggers sadly.  However, that does does misrepresent the reality that WinDBG is really the debugger of choice currently on Windows.  Debugging Swift on Linux already requires LLDB on Linux, and that includes gdb, rr, perf, or any other stack samplers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104246