[ARM SEH 5] [MC] [Win64EH] Check that the SEH unwind opcodes match the actual instructions

[mstorsjo]

Summary

It's a fairly common issue that the generating code incorrectly marks
instructions as narrow or wide; check that the instruction lengths
add up to the expected value, and abort if it doesn't. This allows
catching code generation bugs.

[efriedma]

llvm/lib/Target/ARM/MCTargetDesc/ARMWinCOFFStreamer.cpp

270

I don't like changing whether we emit a label based on NDEBUG; that will affect the actual assembler text. (The actual checking is also probably cheap enough we could just leave it on, but I'll leave that up to you.)

[mstorsjo]

llvm/lib/Target/ARM/MCTargetDesc/ARMWinCOFFStreamer.cpp

270

No, this isn't a regular label - the CFI labels are no-ops in assembly output:

MCSymbol *MCStreamer::emitCFILabel() {
  // Return a dummy non-null value so that label fields appear filled in when
  // generating textual assembly.
  return (MCSymbol *)1;
}

Yeah, the checking is cheap anyway though so it's no problem to leave it on.

[mstorsjo]

Updated and rebased. Made the instruction length checking always enabled, outside of NDEBUG, and changed the error into a proper user level error instead of a printout and std::abort(). Added a testcase for the error message too.

[mstorsjo]

Updated without .seh_endepilogue_nop.

[mstorsjo]

Added checks to make sure that prologs and epilogs are properly terminated, and epilog start/end are paired correctly. This gives clearer indication if there were issues in code generation (like epilogs being mangled by tail merging).

[efriedma]

LGTM

[mstorsjo]

Plain rebase, no functional changePlain rebase, no functional change.

llvm/include/llvm/MC/MCWinEH.h

struct FrameInfo {
   bool Fragment = false;
 
   int LastFrameInst = -1;
   const FrameInfo *ChainedParent = nullptr;
   std::vector<Instruction> Instructions;
   struct Epilog {
     std::vector<Instruction> Instructions;
     unsigned Condition;
+    MCSymbol *End;
   };
   MapVector<MCSymbol *, Epilog> EpilogMap;
 
   FrameInfo() = default;
   FrameInfo(const MCSymbol *Function, const MCSymbol *BeginFuncEHLabel)
       : Begin(BeginFuncEHLabel), Function(Function) {}
   FrameInfo(const MCSymbol *Function, const MCSymbol *BeginFuncEHLabel,
             const FrameInfo *ChainedParent)

llvm/lib/MC/MCWin64EH.cpp

case Win64EH::UOP_Custom: {
       Count += J + 1;
       break;
     }
     }
   }
   return Count;
 }
 
-static uint32_t ARMCountOfInstructionBytes(ArrayRef<WinEH::Instruction> Insns) {
+static uint32_t ARMCountOfInstructionBytes(ArrayRef<WinEH::Instruction> Insns,
+                                           bool *HasCustom = nullptr) {
   uint32_t Count = 0;
   for (const auto &I : Insns) {
     switch (static_cast<Win64EH::UnwindOpcodes>(I.Operation)) {
     default:
       llvm_unreachable("Unsupported ARM unwind code");
     case Win64EH::UOP_AllocSmall:
     case Win64EH::UOP_AllocLarge:
     case Win64EH::UOP_AllocHuge:
case Win64EH::UOP_WideEndNop:
       break;
     case Win64EH::UOP_End:
       // This doesn't map to any instruction
       break;
     case Win64EH::UOP_Custom:
       // We can't reason about what instructions this maps to; return a
       // phony number to make sure we don't accidentally do epilog packing.
       Count += 1000;
+      if (HasCustom)
+        *HasCustom = true;
       break;
     }
   }
   return Count;
 }
 
+static void checkARMInstructions(MCStreamer &Streamer,
+                                 ArrayRef<WinEH::Instruction> Insns,
+                                 const MCSymbol *Begin, const MCSymbol *End,
+                                 StringRef Name, StringRef Type) {
+  if (!End)
+    return;
+  Optional<int64_t> MaybeDistance =
+      GetOptionalAbsDifference(Streamer, End, Begin);
+  if (!MaybeDistance)
+    return;
+  uint32_t Distance = (uint32_t)*MaybeDistance;
+  bool HasCustom = false;
+  uint32_t InstructionBytes = ARMCountOfInstructionBytes(Insns, &HasCustom);
+  if (HasCustom)
+    return;
+  if (Distance != InstructionBytes) {
+    Streamer.getContext().reportError(
+        SMLoc(), "Incorrect size for " + Name + " " + Type + ": " +
+                     Twine(Distance) +
+                     " bytes of instructions in range, but .seh directives "
+                     "corresponding to " +
+                     Twine(InstructionBytes) + " bytes\n");
+  }
+}
+
+static bool isARMTerminator(const WinEH::Instruction &inst) {
+  switch (static_cast<Win64EH::UnwindOpcodes>(inst.Operation)) {
+  case Win64EH::UOP_End:
+  case Win64EH::UOP_EndNop:
+  case Win64EH::UOP_WideEndNop:
+    return true;
+  default:
+    return false;
+  }
+}
+
 // Unwind opcode encodings and restrictions are documented at
 // https://docs.microsoft.com/en-us/cpp/build/arm-exception-handling
 static void ARMEmitUnwindCode(MCStreamer &streamer,
                               const WinEH::Instruction &inst) {
   uint32_t w, lr;
   int i;
   switch (static_cast<Win64EH::UnwindOpcodes>(inst.Operation)) {
   default:
static void ARMEmitUnwindInfo(MCStreamer &streamer, WinEH::FrameInfo *info,
 
   MCContext &context = streamer.getContext();
   MCSymbol *Label = context.createTempSymbol();
 
   streamer.emitValueToAlignment(4);
   streamer.emitLabel(Label);
   info->Symbol = Label;
 
+  if (!info->PrologEnd)
+    streamer.getContext().reportError(SMLoc(), "Prologue in " +
+                                                   info->Function->getName() +
+                                                   " not correctly terminated");
+
+  if (info->PrologEnd && !info->Fragment)
+    checkARMInstructions(streamer, info->Instructions, info->Begin,
+                         info->PrologEnd, info->Function->getName(),
+                         "prologue");
+  for (auto &I : info->EpilogMap) {
+    MCSymbol *EpilogStart = I.first;
+    auto &Epilog = I.second;
+    checkARMInstructions(streamer, Epilog.Instructions, EpilogStart, Epilog.End,
+                         info->Function->getName(), "epilogue");
+    if (Epilog.Instructions.empty() ||
+        !isARMTerminator(Epilog.Instructions.back()))
+      streamer.getContext().reportError(
+          SMLoc(), "Epilogue in " + info->Function->getName() +
+                       " not correctly terminated");
+  }
+
   Optional<int64_t> RawFuncLength;
   const MCExpr *FuncLengthExpr = nullptr;
   if (!info->FuncletOrFuncEnd) {
     report_fatal_error("FuncletOrFuncEnd not set");
   } else {
     // As the size of many thumb2 instructions isn't known until later,
     // we can't always rely on being able to calculate the absolute
     // length of the function here. If we can't calculate it, defer it

llvm/lib/MC/WinCOFFObjectWriter.cpp

void WinCOFFObjectWriter::assignFileOffsets(MCAssembler &Asm,
   unsigned Offset = W.OS.tell();
 
   Offset += UseBigObj ? COFF::Header32Size : COFF::Header16Size;
   Offset += COFF::SectionSize * Header.NumberOfSections;
 
   for (const auto &Section : Asm) {
     COFFSection *Sec = SectionMap[&Section];
 
-    if (Sec->Number == -1)
+    if (!Sec || Sec->Number == -1)
       continue;
 
     Sec->Header.SizeOfRawData = Layout.getSectionAddressSize(&Section);
 
     if (IsPhysicalSection(Sec)) {
       Sec->Header.PointerToRawData = Offset;
       Offset += Sec->Header.SizeOfRawData;
     }

llvm/lib/Target/ARM/MCTargetDesc/ARMWinCOFFStreamer.cpp

 //===-- ARMWinCOFFStreamer.cpp - ARM Target WinCOFF Streamer ----*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "ARMMCTargetDesc.h"
 #include "Utils/ARMBaseInfo.h"
 #include "llvm/MC/MCAsmBackend.h"
 #include "llvm/MC/MCAssembler.h"
 #include "llvm/MC/MCCodeEmitter.h"
+#include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCObjectWriter.h"
 #include "llvm/MC/MCWin64EH.h"
 #include "llvm/MC/MCWinCOFFStreamer.h"
 
 using namespace llvm;
 
 namespace {
 class ARMWinCOFFStreamer : public MCWinCOFFStreamer {

 }
 
 void ARMTargetWinCOFFStreamer::emitARMWinCFIEpilogEnd() {
   auto &S = getStreamer();
   WinEH::FrameInfo *CurFrame = S.EnsureValidWinFrameInfo(SMLoc());
   if (!CurFrame)
     return;
 
+  if (!CurrentEpilog) {
+    S.getContext().reportError(SMLoc(), "Stray .seh_endepilogue in " +
+                                            CurFrame->Function->getName());
+    return;
+  }
+
   std::vector<WinEH::Instruction> &Epilog =
       CurFrame->EpilogMap[CurrentEpilog].Instructions;
 
   unsigned UnwindCode = Win64EH::UOP_End;
   if (!Epilog.empty()) {
     WinEH::Instruction EndInstr = Epilog.back();
     if (EndInstr.Operation == Win64EH::UOP_Nop) {
       UnwindCode = Win64EH::UOP_EndNop;
       Epilog.pop_back();
     } else if (EndInstr.Operation == Win64EH::UOP_WideNop) {
       UnwindCode = Win64EH::UOP_WideEndNop;
       Epilog.pop_back();
     }
   }
 
   InEpilogCFI = false;
   WinEH::Instruction Inst = WinEH::Instruction(UnwindCode, nullptr, -1, 0);
   CurFrame->EpilogMap[CurrentEpilog].Instructions.push_back(Inst);
+  MCSymbol *Label = S.emitCFILabel();
+  CurFrame->EpilogMap[CurrentEpilog].End = Label;
   CurrentEpilog = nullptr;
 }
 
 void ARMTargetWinCOFFStreamer::emitARMWinCFICustom(unsigned Opcode) {
   emitARMWinUnwindCode(Win64EH::UOP_Custom, 0, Opcode);
 }
 
 } // end anonymous namespace
 
 MCTargetStreamer *llvm::createARMObjectTargetWinCOFFStreamer(MCStreamer &S) {
   return new ARMTargetWinCOFFStreamer(S);
 }

llvm/test/MC/ARM/seh-checks.s

+// This test checks error reporting for mismatched prolog/epilog lengths
+
+// RUN: not llvm-mc -triple thumbv7-pc-win32 -filetype=obj -o /dev/null %s 2>&1 | FileCheck %s
+
+// CHECK-NOT: func1
+// CHECK: error: Incorrect size for func2 epilogue: 6 bytes of instructions in range, but .seh directives corresponding to 4 bytes
+// CHECK: error: Incorrect size for func3 prologue: 4 bytes of instructions in range, but .seh directives corresponding to 2 bytes
+
+        .text
+        .syntax unified
+
+        .seh_proc func1
+func1:
+        // Instruction with indeterminate length
+        b other
+        .seh_endprologue
+        nop
+        .seh_startepilogue
+        // The p2align causes the length of the epilogue to be unknown, so
+        // we can't report errors about the mismatch here.
+        .p2align 1
+        pop {r4-r7-lr}
+        .seh_save_regs {r4-r7,lr}
+        bx lr
+        .seh_nop
+        .seh_endepilogue
+        .seh_endproc
+
+        .seh_proc func2
+func2:
+        .seh_endprologue
+        nop
+        .seh_startepilogue
+        // As we're popping into lr instead of directly into pc, this pop
+        // becomes a wide instruction.
+        pop {r4-r7,lr}
+        // The directive things we're making a narrow instruction, which
+        // is wrong.
+        .seh_save_regs {r4-r7,lr}
+        bx lr
+        .seh_nop
+        .seh_endepilogue
+        .seh_endproc
+
+        .seh_proc func3
+func3:
+        nop.w
+        .seh_nop
+        .seh_endprologue
+        nop
+        .seh_endproc

llvm/test/MC/ARM/seh-checks2.s

+// This test checks error reporting for missing ending/starting of prologues/epilogues
+
+// RUN: not llvm-mc -triple thumbv7-pc-win32 -filetype=obj -o /dev/null %s 2>&1 | FileCheck %s
+
+// CHECK: error: Stray .seh_endepilogue in func1
+// CHECK: error: Prologue in func2 not correctly terminated
+// CHECK: error: Epilogue in func3 not correctly terminated
+// CHECK: error: Epilogue in func4 not correctly terminated
+
+        .text
+        .syntax unified
+
+        .seh_proc func1
+func1:
+        sub sp, #16
+        .seh_stackalloc 16
+        .seh_endprologue
+        nop
+        // Missing .seh_startepilogue
+        add sp, #16
+        .seh_stackalloc 16
+        bx lr
+        .seh_nop
+        .seh_endepilogue
+        .seh_endproc
+
+        .seh_proc func2
+func2:
+        sub sp, #16
+        .seh_stackalloc 16
+        // Missing .seh_endprologue
+        nop
+        .seh_startepilogue
+        add sp, #16
+        .seh_stackalloc 16
+        bx lr
+        .seh_nop
+        .seh_endepilogue
+        .seh_endproc
+
+        .seh_proc func3
+func3:
+        sub sp, #16
+        .seh_stackalloc 16
+        .seh_endprologue
+        nop
+        .seh_startepilogue
+        add sp, #16
+        .seh_stackalloc 16
+        bx lr
+        .seh_nop
+        // Missing .seh_endepilogue
+        .seh_endproc
+
+        .seh_proc func4
+func4:
+        sub sp, #16
+        .seh_stackalloc 16
+        .seh_endprologue
+        nop
+        .seh_startepilogue
+        add sp, #16
+        .seh_stackalloc 16
+        bx lr
+        .seh_nop
+        // Missing .seh_endepilogue
+        nop
+        .seh_startepilogue
+        add sp, #16
+        .seh_stackalloc 16
+        bx lr
+        .seh_nop
+        .seh_endepilogue
+        .seh_endproc