[PATCH] Generate SEH unwinding info on Win64

Reid Kleckner rnk at google.com
Tue Jun 10 11:53:05 PDT 2014


================
Comment at: include/llvm/CodeGen/MachineFrameInfo.h:486
@@ -486,1 +485,3 @@
+  int CreateFixedObject(uint64_t Size, int64_t SPOffset, bool Immutable,
+                        bool isSpillSlot = false);
 
----------------
Let's try to avoid contiguous boolean parameters, especially when some have default arguments.  They make it hard to do refactorings like removing the 'Immutable' parameter here.

Instead, CreateFixedSpillSlot(Size, SPOffset) sounds like the right interface to me.

================
Comment at: lib/MC/MCAsmStreamer.cpp:1138
@@ -1137,3 +1138,1 @@
-  OS << "\t.seh_pushreg ";
-  EmitRegisterName(Register);
   EmitEOL();
----------------
This should be a separate revert.  The existing test case is incorrect, it was doing '.seh_savereg %rsi, 16' and disassembling that with %rbp.

================
Comment at: lib/Target/X86/MCTargetDesc/X86MCAsmInfo.cpp:148
@@ +147,3 @@
+    PointerSize = 8;
+    ExceptionsType = ExceptionHandling::Win64;
+  }
----------------
I think this means we will start attempting to emit cleanups for x86_64-pc-windows-msvc, which we probably aren't ready for.  Can we hold off on this change, or are you reasonably sure it's safe?

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:402-410
@@ +401,11 @@
+//
+// [if needs FP]
+//     .seh_setframe %rbp, KKK
+//
+// ; Currently only Win64 spills non-GPRs
+// [for all callee-saved XMM registers]
+//     [if needs FP]
+//         movaps  %xmmXX, -NNN(%rbp)
+//     [else]
+//         movaps  %xmmXX, NNN(%rsp)
+//
----------------
Are these comments accurate when we have an FP *and* the stack needs realignment?  It seems like we should move the win64-specific XMM CSR saves to happen before the stack realignment.  This should be safe since win64 also gives us 16-byte stack alignment.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:738-742
@@ +737,7 @@
+    if (HasFP) {
+        // We need to set frame base offset low enough such that all saved
+        // register offsets would be positive relative to it, but we can't
+        // just use NumBytes, because .seh_setframe offset must be <=240.
+        // So we pretend to have only allocated enough space to spill the
+        // non-volatile registers.
+
----------------
The comment below about "Don't care about the rest of the stack allocation" should get merged in here.  I spent a while trying to understand why this scary comment is OK, and the comment below explains it.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:744
@@ +743,3 @@
+
+        for (unsigned i = 0, e = CSI.size(); i != e; ++i) {
+          int offset = MFI->getObjectOffset(CSI[i].getFrameIdx());
----------------
We have range based for loops now, so this can be:
  for (const CalleeSavedInfo &Info : CSI) { ... }

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:773
@@ +772,3 @@
+  while (MBBI != MBB.end() &&
+    MBBI->getFlag(MachineInstr::FrameSetup)) {
+    ++MBBI;
----------------
Format.  You can drop the braces.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:910
@@ -799,3 +909,3 @@
 
-    if (Opc != X86::POP32r && Opc != X86::POP64r && Opc != X86::DBG_VALUE &&
-        !PI->isTerminator())
+    if (Opc != X86::POP32r && Opc != X86::POP64r &&
+        Opc != X86::DBG_VALUE && !PI->isTerminator())
----------------
Seems like a superfluous formatting change

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:1092
@@ -983,2 +1091,3 @@
 
+  std::vector<CalleeSavedInfo> &CSI = const_cast<std::vector<CalleeSavedInfo>&>(CSI_);
   DebugLoc DL = MBB.findDebugLoc(MI);
----------------
:(

Is TargetFrameLowering::getCalleeSavedSpillSlots() not powerful enough to express the offsets you need?  If it isn't, I think the right way forward is to add a more flexible interface to TFL, like TFL::appendCalleeSavedSpillSlot(std::vector<CSI> &CSIs), where the default implementation forwards on to check 'TFL->getCalleeSavedSpillSlots(NumFixedSpillSlots)' so existing targets don't require changes.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:600-603
@@ -599,5 +599,6 @@
   // FIXME - use subtarget debug flags
   if (!Subtarget->isTargetDarwin() &&
       !Subtarget->isTargetELF() &&
-      !Subtarget->isTargetCygMing()) {
+      !Subtarget->isTargetCygMing() &&
+      !Subtarget->isTargetWin64()) {
     setOperationAction(ISD::EH_LABEL, MVT::Other, Expand);
----------------
Can we flip this to the positive sense?  What targets actually need it?  win32 only?

http://reviews.llvm.org/D4081






More information about the llvm-commits mailing list