[4/5] [MC] [Win64EH] Fill in FuncletOrFuncEnd if missing

[mstorsjo]

Summary

This can happen e.g. for code that declare .seh_proc/.seh_endproc in assembly, or for code that use .seh_handlerdata (which triggers the unwind info to be emitted before the end of the function).

The TextSection field must be made non-const to be able to use it with Streamer.SwitchSection().

[efriedma]

llvm/lib/MC/MCWin64EH.cpp

531

I don't understand why you're dropping the FIXME; clearly, if we're emitting unwind info that doesn't cover anything, something has gone very wrong.

681

It seems like you're setting FuncletOrFuncEnd in too many places; there should be exactly one directive that corresponds to the end of the function, I think.

[mstorsjo]

llvm/lib/MC/MCWin64EH.cpp

531

Right, the FIXME indeed should stay, if we still hit that case, even though it's much less probable after this change.

681

There's actually two different codepaths; either in MCStreamer::EmitWinCFIEndProc when running into .seh_endproc`, or in ARM64UnwindEmitter::EmitUnwindInfo when .seh_handlerdata forces emitting it while halfway through the function. But this one can be left out, as far as I see with the cases I have in mind.

The case with .seh_handlerdata is tricky, because it forces writing the xdata entry possibly before the full function has been generated. On x86 this is not an issue, because the xdata entry doesn't contain the full function length but that's referenced in the .pdata via relocations to the function start/end. But on arm, the xdata entry needs to know the full length. So for cases with .seh_handlerdata, we can place FuncletOrFuncEnd at the place where we are, and hope that it covers the parts of the function that unwinding will occur in.

[mstorsjo]

Wrote a bit longer comment explaining the issue with potentially truncated function length on .seh_handlerdata, left out the redundant place for filling in FuncletOrFuncEnd, and kept the fixme comment.

[efriedma]

llvm/lib/MC/MCWin64EH.cpp

710

Instead of checking Streamer.getCurrentSectionOnly() == info->TextSection, can we just Streamer.SwitchSection(info->TextSection)?

[mstorsjo]

llvm/lib/MC/MCWin64EH.cpp

710

Oh, yeah I guess that'd work just as well.

[mstorsjo]

Using SwitchSection() to always emit the label if missing. Had to make TextSection non-const to be able to use it with SwitchSection().

[mstorsjo]

Updated the testcase after the new version of 3/5.

[efriedma]

llvm/lib/MC/MCStreamer.cpp

696

Instead of setting FuncletOrFuncEnd here, should we be calling EmitWinCFIFuncletOrFuncEnd somewhere?

llvm/lib/MC/MCWin64EH.cpp

529

With the other changes in this patch, is it still possible for FuncletOrFuncEnd to be null?

[mstorsjo]

llvm/lib/MC/MCStreamer.cpp

696

I don't think so. EmitWinCFIEndProc is essentially called from two places; either from lib/CodeGen/AsmPrinter/WinException.cpp, which also does call EmitWinCFIFuncletOrFuncEnd properly, so there it shouldn't be possible to reach EndProc without FuncletOrFuncEnd being set. The only other case is MC/MCParser/COFFAsmParser.cpp, which has a 1:1 mapping between directives and calling these methods.

llvm/lib/MC/MCWin64EH.cpp

529

Ah, right, yes, it shouldn't be possible. Even if we'd trigger .seh_handlerdata immediately, we would have a FuncletOrFuncEnd symbol (with a zero distance from the start), and we'd skip writing this due to it being empty anyway. So we should be able to make this a report_fatal_error, as it shouldn't be triggerable even with specially crafted input.

[mstorsjo]

Made a missing FuncletOrFuncEnd a hard error.

[efriedma]

LGTM

llvm/include/llvm/MC/MCWinEH.h

 struct FrameInfo {
   const MCSymbol *Begin = nullptr;
   const MCSymbol *End = nullptr;
   const MCSymbol *FuncletOrFuncEnd = nullptr;
   const MCSymbol *ExceptionHandler = nullptr;
   const MCSymbol *Function = nullptr;
   const MCSymbol *PrologEnd = nullptr;
   const MCSymbol *Symbol = nullptr;
-  const MCSection *TextSection = nullptr;
+  MCSection *TextSection = nullptr;
 
   bool HandlesUnwind = false;
   bool HandlesExceptions = false;
   bool EmitAttempted = false;
 
   int LastFrameInst = -1;
   const FrameInfo *ChainedParent = nullptr;
   std::vector<Instruction> Instructions;

llvm/lib/MC/MCStreamer.cpp

void MCStreamer::EmitWinCFIEndProc(SMLoc Loc) {
   WinEH::FrameInfo *CurFrame = EnsureValidWinFrameInfo(Loc);
   if (!CurFrame)
     return;
   if (CurFrame->ChainedParent)
     getContext().reportError(Loc, "Not all chained regions terminated!");
 
   MCSymbol *Label = emitCFILabel();
   CurFrame->End = Label;
+  if (!CurFrame->FuncletOrFuncEnd)
+    CurFrame->FuncletOrFuncEnd = CurFrame->End;
 }
 
 void MCStreamer::EmitWinCFIFuncletOrFuncEnd(SMLoc Loc) {
   WinEH::FrameInfo *CurFrame = EnsureValidWinFrameInfo(Loc);
   if (!CurFrame)
     return;
   if (CurFrame->ChainedParent)
     getContext().reportError(Loc, "Not all chained regions terminated!");

llvm/lib/MC/MCWin64EH.cpp

static void ARM64EmitUnwindInfo(MCStreamer &streamer, WinEH::FrameInfo *info) {
   MCSymbol *Label = context.createTempSymbol();
 
   streamer.emitValueToAlignment(4);
   streamer.emitLabel(Label);
   info->Symbol = Label;
 
   int64_t RawFuncLength;
   if (!info->FuncletOrFuncEnd) {
-    // FIXME: This is very wrong; we emit SEH data which covers zero bytes
-    // of code. But otherwise test/MC/AArch64/seh.s crashes.
-    RawFuncLength = 0;
+    report_fatal_error("FuncletOrFuncEnd not set");
   } else {
     // FIXME: GetAbsDifference tries to compute the length of the function
     // immediately, before the whole file is emitted, but in general
     // that's impossible: the size in bytes of certain assembler directives
     // like .align and .fill is not known until the whole file is parsed and
     // relaxations are applied. Currently, GetAbsDifference fails with a fatal
     // error in that case. (We mostly don't hit this because inline assembly
     // specifying those directives is rare, and we don't normally try to
     // align loops on AArch64.)
     //
     // There are two potential approaches to delaying the computation. One,
streamer.emitValue(MCSymbolRefExpr::create(info->Symbol,
                      4);
 }
 
 void llvm::Win64EH::ARM64UnwindEmitter::Emit(MCStreamer &Streamer) const {
   // Emit the unwind info structs first.
   for (const auto &CFI : Streamer.getWinFrameInfos()) {
     WinEH::FrameInfo *Info = CFI.get();
     if (Info->empty())
       continue;
     MCSection *XData = Streamer.getAssociatedXDataSection(CFI->TextSection);
     Streamer.SwitchSection(XData);
     ARM64EmitUnwindInfo(Streamer, Info);
   }
 
   // Now emit RUNTIME_FUNCTION entries.
   for (const auto &CFI : Streamer.getWinFrameInfos()) {
     WinEH::FrameInfo *Info = CFI.get();
     // ARM64EmitUnwindInfo above clears the info struct, so we can't check
     // empty here. But if a Symbol is set, we should create the corresponding
     // pdata entry.
     if (!Info->Symbol)
       continue;
     MCSection *PData = Streamer.getAssociatedPDataSection(CFI->TextSection);
     Streamer.SwitchSection(PData);
     ARM64EmitRuntimeFunction(Streamer, Info);
   }
 }
 
 void llvm::Win64EH::ARM64UnwindEmitter::EmitUnwindInfo(
     MCStreamer &Streamer, WinEH::FrameInfo *info) const {
+  // Called if there's an .seh_handlerdata directive before the end of the
+  // function. This forces writing the xdata record already here - and
+  // in this case, the function isn't actually ended already, but the xdata
+  // record needs to know the function length. In these cases, if the funclet
+  // end hasn't been marked yet, the xdata function length won't cover the
+  // whole function, only up to this point.
+  if (!info->FuncletOrFuncEnd) {
+    Streamer.SwitchSection(info->TextSection);
+    info->FuncletOrFuncEnd = Streamer.emitCFILabel();
+  }
   // Switch sections (the static function above is meant to be called from
   // here and from Emit().
   MCSection *XData = Streamer.getAssociatedXDataSection(info->TextSection);
   Streamer.SwitchSection(XData);
   ARM64EmitUnwindInfo(Streamer, info);
 }

llvm/test/MC/AArch64/seh.s

 // CHECK-NEXT:     0x8 IMAGE_REL_ARM64_ADDR32NB __C_specific_handler
 // CHECK-NEXT:   }
 // CHECK-NEXT:   Section (5) .pdata {
 // CHECK-NEXT:     0x0 IMAGE_REL_ARM64_ADDR32NB func
 // CHECK-NEXT:     0x4 IMAGE_REL_ARM64_ADDR32NB .xdata
 // CHECK-NEXT:   }
 // CHECK-NEXT: ]
 
+// CHECK-NEXT: UnwindInformation [
+// CHECK-NEXT:   RuntimeFunction {
+// CHECK-NEXT:     Function: func
+// CHECK-NEXT:     ExceptionRecord: .xdata
+// CHECK-NEXT:     ExceptionData {
+// CHECK-NEXT:       FunctionLength: 8
 
     .text
     .globl func
     .def func
     .scl 2
     .type 32
     .endef
     .seh_proc func