Fix SEH table addresses for Windows

[dpaoliello]

Summary

Issue Details:
The addresses for SEH tables for Windows are incorrect as 1 was unconditionally being added to all addresses. +1 is required for the SEH end address (as it is exclusive), but the SEH start addresses is inclusive and so should be used as-is.

In the IP2State tables, the addresses are +1 for AMD64 to handle the return address for a call being after the actual call instruction but are as-is for ARM and ARM64 as the StateFromIp function in the VC runtime automatically takes this into account and adjusts the address that is it looking up.

Fix Details:

• Split the getLabel function into two: getLabel (used for the SEH start address and ARM+ARM64 IP2State addresses) and getLabelPlusOne (for the SEH end address, and AMD64 IP2State addresses).

[dpaoliello]

Fix typo

[rnk]

llvm/lib/CodeGen/AsmPrinter/WinException.cpp

336

It seems that after several rounds of refactoring, there are no comments here explaining why this happens. :(

668

Arguably, this is the special case that opens a range but does not end one, so the modification would be here.

963

This is the other call site of getLabel for C++.

llvm/test/CodeGen/WinEH/wineh-noret-cleanup.ll

49

These changes are incorrect. The C++ ip2state table doesn't have a begin/end pair, it has a series of labels. When an exception is thrown from the last instruction in a range, these labels must be strictly greater than the return address.

Fortunately, there are no single-byte instructions that can throw a C++ exception, so the entire table can be biased forwards by one byte.

[dpaoliello]

Only avoid +1 for SEH start addresses

[dpaoliello]

llvm/test/CodeGen/WinEH/wineh-noret-cleanup.ll

49

Ah, ok, this makes sense:
In all of the caller frames we need to consider the calling instruction, not the instruction that we will be returning to.
For the throwing frame itself, we're off by 1, however the address of the throwing instruction is always less than the next IP2State transition since there is no one byte throwing instruction.

[rnk]

llvm/lib/CodeGen/AsmPrinter/WinException.cpp

333

I think you've lost this ARM change unintentionally. For AArch64, we don't bias the label forward. However, that seems wrong to me based on MSVC's output:
https://gcc.godbolt.org/z/3TxMfqoTh

MSVC inserts a nop instruction after the bl instruction implementing the call, and if I read the ip2state table correctly, the ip2state entry is the address following the nop instruction. So, I strongly suspect there is a bug here for ARM, but it's an existing problem.

You can see in the history that this code changed in 2018, we used to have a getLabelPlusOne method, but it was renamed:
https://reviews.llvm.org/D50166

[dpaoliello]

Fix ARM IP2State

[dpaoliello]

llvm/lib/CodeGen/AsmPrinter/WinException.cpp

333

I've dug into this a bit more: ARM and ARM64 don't need the +1 as the StateFromIp function in the VC Runtime adjusts the input address to compensate for call-return-address issue, whereas it does not for AMD64 (probably for legacy reasons).

[rnk]

lgtm

llvm/lib/CodeGen/AsmPrinter/WinException.cpp

670

So, this *will* be a behavior change for ARM. Is that desired? Are there issues with adding one to ARM PCs? It sets the thumb bit, but maybe that doesn't matter to __C_specific_handler on ARM. Neither ARM ISA has single byte instructions (right?), so adding one isn't likely to accidentally cause an exception from the next instruction to go to the wrong handler.

955–958

Truthfully, it feels to me like this is an x64-specific bug. If Windows were to be ported to a new architecture in five years, I bet they'd stick with the ARM behavior, and we wouldn't do the +1. However, that's all very hypothetical, I don't see a need for any code changes.

[dpaoliello]

llvm/lib/CodeGen/AsmPrinter/WinException.cpp

670

__C_specific_handler has the same call-return-address compensation on ARM/ARM64 as FrameFromIp. That being said, this change is desired as the end address is exclusive, so we should +1 it to make sure that the comparison works correctly.

955–958

Sorry, I don't really have any explanation here either ¯\_(ツ)_/¯

[dpaoliello]

llvm/lib/CodeGen/AsmPrinter/WinException.h

43

@rnk Would you prefer if I followed the existing casing here, or used the casing that clang-tidy suggests?

[rnk]

llvm/lib/CodeGen/AsmPrinter/WinException.h

43

Let's stay consistent with what's here and ignore clang-tidy for now.

[dpaoliello]

@rnk Can you please merge this in?

[dpaoliello]

@rnk Who can help merge this in?

[mstorsjo]

In D107784#2957747, @dpaoliello wrote:

@rnk Who can help merge this in?

I might have time to apply, run tests and push it - can you provide the git author line, ie “Real Name <email@address>” to use for the commit?

[dpaoliello]

In D107784#2957846, @mstorsjo wrote:

In D107784#2957747, @dpaoliello wrote:

@rnk Who can help merge this in?

I might have time to apply, run tests and push it - can you provide the git author line, ie “Real Name <email@address>” to use for the commit?

Daniel Paoliello <[e-mail]>

[rnk]

Thanks, I think I patched it in and tested it but didn't push it.

llvm/lib/CodeGen/AsmPrinter/WinException.h

class LLVM_LIBRARY_VISIBILITY WinException : public EHStreamer {
   bool shouldEmitMoves = false;
 
   /// True if this is a 64-bit target and we should use image relative offsets.
   bool useImageRel32 = false;
 
   /// True if we are generating exception handling on Windows for ARM64.
   bool isAArch64 = false;
 
+  /// True if we are generating exception handling on Windows for ARM (Thumb).
+  bool isThumb = false;
+
   /// Pointer to the current funclet entry BB.
   const MachineBasicBlock *CurrentFuncletEntry = nullptr;
 
   /// The section of the last funclet start.
   MCSection *CurrentFuncletTextSection = nullptr;
 
   /// The list of symbols to add to the ehcont section
   std::vector<const MCSymbol *> EHContTargets;
class LLVM_LIBRARY_VISIBILITY WinException : public EHStreamer {
   /// Emits the label used with llvm.eh.recoverfp, which is used by
   /// outlined funclets.
   void emitEHRegistrationOffsetLabel(const WinEHFuncInfo &FuncInfo,
                                      StringRef FLinkageName);
 
   const MCExpr *create32bitRef(const MCSymbol *Value);
   const MCExpr *create32bitRef(const GlobalValue *GV);
   const MCExpr *getLabel(const MCSymbol *Label);
+  const MCExpr *getLabelPlusOne(const MCSymbol *Label);
   const MCExpr *getOffset(const MCSymbol *OffsetOf, const MCSymbol *OffsetFrom);
   const MCExpr *getOffsetPlusOne(const MCSymbol *OffsetOf,
                                  const MCSymbol *OffsetFrom);
 
   /// Gets the offset that we should use in a table for a stack object with the
   /// given index. For targets using CFI (Win64, etc), this is relative to the
   /// established SP at the end of the prologue. For targets without CFI (Win32
   /// only), it is relative to the frame pointer.

llvm/lib/CodeGen/AsmPrinter/WinException.cpp

 #include "llvm/Target/TargetOptions.h"
 using namespace llvm;
 
 WinException::WinException(AsmPrinter *A) : EHStreamer(A) {
   // MSVC's EH tables are always composed of 32-bit words.  All known 64-bit
   // platforms use an imagerel32 relocation to refer to symbols.
   useImageRel32 = (A->getDataLayout().getPointerSizeInBits() == 64);
   isAArch64 = Asm->TM.getTargetTriple().isAArch64();
+  isThumb = Asm->TM.getTargetTriple().isThumb();
 }
 
 WinException::~WinException() {}
 
 /// endModule - Emit all exception information that should come after the
 /// content.
 void WinException::endModule() {
   auto &OS = *Asm->OutStreamer;

 
 const MCExpr *WinException::create32bitRef(const GlobalValue *GV) {
   if (!GV)
     return MCConstantExpr::create(0, Asm->OutContext);
   return create32bitRef(Asm->getSymbol(GV));
 }
 
 const MCExpr *WinException::getLabel(const MCSymbol *Label) {
-  if (isAArch64)
-    return MCSymbolRefExpr::create(Label, MCSymbolRefExpr::VK_COFF_IMGREL32,
-                                   Asm->OutContext);
-  return MCBinaryExpr::createAdd(create32bitRef(Label),
+}
+
+const MCExpr *WinException::getLabelPlusOne(const MCSymbol *Label) {
+  return MCBinaryExpr::createAdd(getLabel(Label),
                                  MCConstantExpr::create(1, Asm->OutContext),
                                  Asm->OutContext);
 }
 
 const MCExpr *WinException::getOffset(const MCSymbol *OffsetOf,
                                       const MCSymbol *OffsetFrom) {
   return MCBinaryExpr::createSub(
       MCSymbolRefExpr::create(OffsetOf, Asm->OutContext),

 ///     Continue searching this table or continue unwinding.
 ///   EXCEPTION_CONTINUE_EXECUTION (-1):
 ///     Resume execution at the trapping PC.
 ///
 /// Inferred table structure:
 ///   struct Table {
 ///     int NumEntries;
 ///     struct Entry {
-///       imagerel32 LabelStart;
-///       imagerel32 LabelEnd;
+///       imagerel32 LabelStart;       // Inclusive
+///       imagerel32 LabelEnd;         // Exclusive
 ///       imagerel32 FilterOrFinally;  // One means catch-all.
 ///       imagerel32 LabelLPad;        // Zero means __finally.
 ///     } Entries[NumEntries];
 ///   };
 void WinException::emitCSpecificHandlerTable(const MachineFunction *MF) {
   auto &OS = *Asm->OutStreamer;
   MCContext &Ctx = Asm->OutContext;
   const WinEHFuncInfo &FuncInfo = *MF->getWinEHFuncInfo();
if (UME.IsFinally) {
       // For an except, the filter can be 1 (catch-all) or a function
       // label.
       FilterOrFinally = UME.Filter ? create32bitRef(UME.Filter)
                                    : MCConstantExpr::create(1, Ctx);
       ExceptOrNull = create32bitRef(Handler->getSymbol());
     }
 
     AddComment("LabelStart");
     OS.emitValue(getLabel(BeginLabel), 4);
     AddComment("LabelEnd");
-    OS.emitValue(getLabel(EndLabel), 4);
+    OS.emitValue(getLabelPlusOne(EndLabel), 4);
     AddComment(UME.IsFinally ? "FinallyFunclet" : UME.Filter ? "FilterFunction"
                                                              : "CatchAll");
     OS.emitValue(FilterOrFinally, 4);
     AddComment(UME.IsFinally ? "Null" : "ExceptionHandler");
     OS.emitValue(ExceptOrNull, 4);
 
     assert(UME.ToState < State && "states should decrease");
     State = UME.ToState;
for (const auto &StateChange : InvokeStateChangeIterator::range(
       // Compute the label to report as the start of this entry; use the EH
       // start label for the invoke if we have one, otherwise (this is a call
       // which may unwind to our caller and does not have an EH start label, so)
       // use the previous end label.
       const MCSymbol *ChangeLabel = StateChange.NewStartLabel;
       if (!ChangeLabel)
         ChangeLabel = StateChange.PreviousEndLabel;
       // Emit an entry indicating that PCs after 'Label' have this EH state.
+      // NOTE: On ARM architectures, the StateFromIp automatically takes into
+      // account that the return address is after the call instruction (whose EH
+      // state we should be using), but on other platforms we need to +1 to the
+      // label so that we are using the correct EH state.
+      const MCExpr *LabelExpression = (isAArch64 || isThumb)
+                                          ? getLabel(ChangeLabel)
+                                          : getLabelPlusOne(ChangeLabel);
       IPToStateTable.push_back(
-          std::make_pair(getLabel(ChangeLabel), StateChange.NewState));
+          std::make_pair(LabelExpression, StateChange.NewState));
       // FIXME: assert that NewState is between CatchLow and CatchHigh.
     }
   }
 }
 
 void WinException::emitEHRegistrationOffsetLabel(const WinEHFuncInfo &FuncInfo,
                                                  StringRef FLinkageName) {
   // Outlined helpers called by the EH runtime need to know the offset of the EH

llvm/test/CodeGen/WinEH/wineh-noret-cleanup.ll

 ;Seh: %catch2 = catchpad within %cs2 [void ()* @dummy_filter]
   unreachable
 }
 
 ; CXX-LABEL: test:
 ; CXX-LABEL: $ip2state$test:
 ; CXX-NEXT:   .long   .Lfunc_begin0@IMGREL
 ; CXX-NEXT:   .long   -1
 ; CXX-NEXT:   .long   .Ltmp0@IMGREL+1
 ; CXX-NEXT:   .long   1
 ; CXX-NEXT:   .long   .Ltmp1@IMGREL+1
 ; CXX-NEXT:   .long   -1
 ; CXX-NEXT:   .long   "?catch$3@?0?test@4HA"@IMGREL
 ; CXX-NEXT:   .long   2
 ; CXX-NEXT:   .long   .Ltmp2@IMGREL+1
 ; CXX-NEXT:   .long   3
 ; CXX-NEXT:   .long   .Ltmp3@IMGREL+1
 ; CXX-NEXT:   .long   2
 ; CXX-NEXT:   .long   "?catch$5@?0?test@4HA"@IMGREL
 ; CXX-NEXT:   .long   4
 
 ; SEH-LABEL: test:
 ; SEH-LABEL: .Llsda_begin0:
-; SEH-NEXT:    .long   .Ltmp0@IMGREL+1
+; SEH-NEXT:    .long   .Ltmp0@IMGREL
 ; SEH-NEXT:    .long   .Ltmp1@IMGREL+1
 ; SEH-NEXT:    .long   dummy_filter@IMGREL
 ; SEH-NEXT:    .long   .LBB0_3@IMGREL
-; SEH-NEXT:    .long   .Ltmp0@IMGREL+1
+; SEH-NEXT:    .long   .Ltmp0@IMGREL
 ; SEH-NEXT:    .long   .Ltmp1@IMGREL+1
 ; SEH-NEXT:    .long   dummy_filter@IMGREL
 ; SEH-NEXT:    .long   .LBB0_5@IMGREL
-; SEH-NEXT:    .long   .Ltmp2@IMGREL+1
+; SEH-NEXT:    .long   .Ltmp2@IMGREL
 ; SEH-NEXT:    .long   .Ltmp3@IMGREL+1
 ; SEH-NEXT:    .long   "?dtor$2@?0?test@4HA"@IMGREL
 ; SEH-NEXT:    .long   0
-; SEH-NEXT:    .long   .Ltmp2@IMGREL+1
+; SEH-NEXT:    .long   .Ltmp2@IMGREL
 ; SEH-NEXT:    .long   .Ltmp3@IMGREL+1
 ; SEH-NEXT:    .long   dummy_filter@IMGREL
 ; SEH-NEXT:    .long   .LBB0_5@IMGREL
 ; SEH-NEXT:  .Llsda_end0:

llvm/test/CodeGen/X86/catchret-empty-fallthrough.ll

 ; CHECK: callq bar
 ; CHECK: .Ltmp1:
 ; CHECK: .LBB0_[[catch:[0-9]+]]:
 
 ; CHECK: .seh_handlerdata
 ; CHECK-NEXT: .set .Lfoo$parent_frame_offset, 32
 ; CHECK-NEXT: .long   (.Llsda_end0-.Llsda_begin0)/16
 ; CHECK-NEXT: .Llsda_begin0:
-; CHECK-NEXT: .long   .Ltmp0@IMGREL+1
+; CHECK-NEXT: .long   .Ltmp0@IMGREL
 ; CHECK-NEXT: .long   .Ltmp1@IMGREL+1
 ; CHECK-NEXT: .long   1
 ; CHECK-NEXT: .long   .LBB0_[[catch]]@IMGREL

llvm/test/CodeGen/X86/seh-catchpad.ll

 ; CHECK:         leaq    "??_C@_06IBDBCMGJ@caught?$AA@"(%rip), %rcx
 ; CHECK:         callq   puts
 ; CHECK:         jmp     .LBB1_[[epilogue]]
 
 ; CHECK:         .seh_handlerdata
 ; CHECK-NEXT:         .set .Lmain$parent_frame_offset, 32
 ; CHECK-NEXT:         .long   (.Llsda_end0-.Llsda_begin0)/16
 ; CHECK-NEXT: .Llsda_begin0:
-; CHECK-NEXT:         .long   .Ltmp0@IMGREL+1
+; CHECK-NEXT:         .long   .Ltmp0@IMGREL
 ; CHECK-NEXT:         .long   .Ltmp1@IMGREL+1
 ; CHECK-NEXT:         .long   1
 ; CHECK-NEXT:         .long   .LBB1_[[except1bb]]@IMGREL
-; CHECK-NEXT:         .long   .Ltmp0@IMGREL+1
+; CHECK-NEXT:         .long   .Ltmp0@IMGREL
 ; CHECK-NEXT:         .long   .Ltmp1@IMGREL+1
 ; CHECK-NEXT:         .long   "?filt$0@0@main@@"@IMGREL
 ; CHECK-NEXT:         .long   .LBB1_[[except2bb]]@IMGREL
-; CHECK-NEXT:         .long   .Ltmp2@IMGREL+1
+; CHECK-NEXT:         .long   .Ltmp2@IMGREL
 ; CHECK-NEXT:         .long   .Ltmp3@IMGREL+1
 ; CHECK-NEXT:         .long   "?dtor$[[finbb:[0-9]+]]@?0?main@4HA"@IMGREL
 ; CHECK-NEXT:         .long   0
-; CHECK-NEXT:         .long   .Ltmp2@IMGREL+1
+; CHECK-NEXT:         .long   .Ltmp2@IMGREL
 ; CHECK-NEXT:         .long   .Ltmp3@IMGREL+1
 ; CHECK-NEXT:         .long   "?filt$0@0@main@@"@IMGREL
 ; CHECK-NEXT:         .long   .LBB1_3@IMGREL
-; CHECK-NEXT:         .long   .Ltmp6@IMGREL+1
+; CHECK-NEXT:         .long   .Ltmp6@IMGREL
 ; CHECK-NEXT:         .long   .Ltmp7@IMGREL+1
 ; CHECK-NEXT:         .long   "?filt$0@0@main@@"@IMGREL
 ; CHECK-NEXT:         .long   .LBB1_3@IMGREL
 ; CHECK-NEXT: .Llsda_end0:
 
 ; CHECK:         .text
 ; CHECK:         .seh_endproc
 

llvm/test/CodeGen/X86/seh-except-finally.ll

 ; CHECK: callq "?fin$0@0@use_both@@"
 ; CHECK: .Ltmp5
 ; CHECK: retq
 ;
 ; CHECK: .seh_handlerdata
 ; CHECK-NEXT: .Luse_both$parent_frame_offset
 ; CHECK-NEXT: .long (.Llsda_end0-.Llsda_begin0)/16
 ; CHECK-NEXT: .Llsda_begin0:
-; CHECK-NEXT: .long .Ltmp0@IMGREL+1
+; CHECK-NEXT: .long .Ltmp0@IMGREL
 ; CHECK-NEXT: .long .Ltmp1@IMGREL+1
 ; CHECK-NEXT: .long "?dtor$2@?0?use_both@4HA"@IMGREL
 ; CHECK-NEXT: .long 0
-; CHECK-NEXT: .long .Ltmp0@IMGREL+1
+; CHECK-NEXT: .long .Ltmp0@IMGREL
 ; CHECK-NEXT: .long .Ltmp1@IMGREL+1
 ; CHECK-NEXT: .long "?filt$0@0@use_both@@"@IMGREL
 ; CHECK-NEXT: .long .LBB0_{{[0-9]+}}@IMGREL
-; CHECK-NEXT: .long .Ltmp4@IMGREL+1
+; CHECK-NEXT: .long .Ltmp4@IMGREL
 ; CHECK-NEXT: .long .Ltmp5@IMGREL+1
 ; CHECK-NEXT: .long "?filt$0@0@use_both@@"@IMGREL
 ; CHECK-NEXT: .long .LBB0_{{[0-9]+}}@IMGREL
 ; CHECK-NEXT: .Llsda_end0:
 
 ; Function Attrs: noinline nounwind
 define internal i32 @"\01?filt$0@0@use_both@@"(i8* %exception_pointers, i8* %frame_pointer) #2 {
 entry:

llvm/test/CodeGen/X86/seh-finally.ll

 
 ; X64-LABEL: main:
 ; X64: retq
 
 ; X64: .seh_handlerdata
 ; X64-NEXT: .set .Lmain$parent_frame_offset, 32
 ; X64-NEXT: .long   (.Llsda_end0-.Llsda_begin0)/16 # Number of call sites
 ; X64-NEXT: .Llsda_begin0:
-; X64-NEXT: .long   .Ltmp0@IMGREL+1 # LabelStart
+; X64-NEXT: .long   .Ltmp0@IMGREL # LabelStart
 ; X64-NEXT: .long   .Ltmp1@IMGREL+1 # LabelEnd
 ; X64-NEXT: .long   "?dtor$2@?0?main@4HA"@IMGREL # FinallyFunclet
 ; X64-NEXT: .long   0               # Null
 ; X64-NEXT: .Llsda_end0:
 
 ; X64-LABEL: "?dtor$2@?0?main@4HA":
 ; X64: callq puts
 ; X64: retq

llvm/test/CodeGen/X86/seh-safe-div.ll

 ; CHECK: callq puts
 ; CHECK: movl $-1, [[rloc]]
 ; CHECK: jmp [[cont_bb]]
 
 ; CHECK: .seh_handlerdata
 ; CHECK-NEXT: .Lsafe_div$parent_frame_offset
 ; CHECK-NEXT: .long (.Llsda_end0-.Llsda_begin0)/16
 ; CHECK-NEXT: .Llsda_begin0:
-; CHECK-NEXT: .long .Ltmp0@IMGREL+1
+; CHECK-NEXT: .long .Ltmp0@IMGREL
 ; CHECK-NEXT: .long .Ltmp1@IMGREL+1
 ; CHECK-NEXT: .long safe_div_filt0@IMGREL
 ; CHECK-NEXT: .long [[handler0]]@IMGREL
-; CHECK-NEXT: .long .Ltmp0@IMGREL+1
+; CHECK-NEXT: .long .Ltmp0@IMGREL
 ; CHECK-NEXT: .long .Ltmp1@IMGREL+1
 ; CHECK-NEXT: .long safe_div_filt1@IMGREL
 ; CHECK-NEXT: .long [[handler1]]@IMGREL
 ; CHECK-NEXT: .Llsda_end0:
 ; CHECK: .text
 ; CHECK: .seh_endproc
 
 define void @try_body(i32* %r, i32* %n, i32* %d) {