[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

511

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.

671

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

511

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

671

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

694

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

[mstorsjo]

llvm/lib/MC/MCWin64EH.cpp

694

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

509

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

509

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/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.
+    // This can happen if unwind info is forced to be generated at a point
+    // where the end of the function hasn't been determined yet, and where
+    // the current section at the time of generating the unwind info wasn't
+    // the text section, so we weren't able to automatically add a
+    // FuncletOrFuncEnd label.
     RawFuncLength = 0;
   } 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
streamer.emitValue(MCSymbolRefExpr::create(info->Symbol,
                                              MCSymbolRefExpr::VK_COFF_IMGREL32,
                                              context),
                      4);
 }
 
 void llvm::Win64EH::ARM64UnwindEmitter::Emit(MCStreamer &Streamer) const {
   // Emit the unwind info structs first.
   for (const auto &CFI : Streamer.getWinFrameInfos()) {
+    if (!CFI->FuncletOrFuncEnd &&
+        Streamer.getCurrentSectionOnly() == CFI->TextSection)
+      CFI->FuncletOrFuncEnd = Streamer.emitCFILabel();
     MCSection *XData = Streamer.getAssociatedXDataSection(CFI->TextSection);
     Streamer.SwitchSection(XData);
     ARM64EmitUnwindInfo(Streamer, CFI.get());
   }
 
   // Now emit RUNTIME_FUNCTION entries.
   for (const auto &CFI : Streamer.getWinFrameInfos()) {
     MCSection *PData = Streamer.getAssociatedPDataSection(CFI->TextSection);
     Streamer.SwitchSection(PData);
     ARM64EmitRuntimeFunction(Streamer, CFI.get());
   }
 }
 
 void llvm::Win64EH::ARM64UnwindEmitter::EmitUnwindInfo(
     MCStreamer &Streamer, WinEH::FrameInfo *info) const {
   // Switch sections (the static function above is meant to be called from
   // here and from Emit().
+  if (!info->FuncletOrFuncEnd &&
+      Streamer.getCurrentSectionOnly() == info->TextSection)
+    info->FuncletOrFuncEnd = Streamer.emitCFILabel();
   MCSection *XData = Streamer.getAssociatedXDataSection(info->TextSection);
   Streamer.SwitchSection(XData);
   ARM64EmitUnwindInfo(Streamer, info);
 }

llvm/test/MC/AArch64/seh.s

 // CHECK-NEXT:   Section (5) .pdata {
 // CHECK-NEXT:     0x0 IMAGE_REL_ARM64_ADDR32NB func
 // CHECK-NEXT:     0x4 IMAGE_REL_ARM64_ADDR32NB .xdata
 // CHECK-NEXT:     0x8 IMAGE_REL_ARM64_ADDR32NB smallFunc
 // CHECK-NEXT:     0xC 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
+// CHECK:        RuntimeFunction {
+// CHECK-NEXT:     Function: smallFunc
+// CHECK-NEXT:     ExceptionRecord: .xdata
+// CHECK-NEXT:     ExceptionData {
+// CHECK-NEXT:       FunctionLength: 4
 
     .text
     .globl func
     .def func
     .scl 2
     .type 32
     .endef
     .seh_proc func