[AArch64][Windows] Compute function length correctly in unwind tables.

[efriedma]

Summary

The primary fix here is to WinException.cpp: we need to exclude jump tables when computing the length of a function, or else we fail to correctly compute the length. (We can only compute the number of bytes consumed by certain assembler directives after the entire file is parsed. ".p2align" is one of those directives, and is used by jump table generation.)

The secondary fix, to MCWin64EH, is to make sure we don't silently miscompile if we hit a similar situation in the future.

It's possible we could extend ARM64EmitUnwindInfo to emit the length of the function using a relocation that would be resolved later in the assembler, instead of trying to resolve it immediately; this would make alignment directives work correctly. I haven't pursued that approach because I'm not sure how complicated it would be. (We would need to estimate the length somehow to implement unwind data splitting).

Hopefully fixes https://bugs.llvm.org/show_bug.cgi?id=41581 .

[rnk]

It sounds like we're encoding the unwind info too early for AArch64. When we were implementing codeview inline line table stuff, we needed to be able to reliably calculate absolute label differences, so we added MCCVInlineLineTableFragment to delay the table encoding until assembly time. We can certainly do this for now, but perhaps a FIXME to delay things would be good.

[efriedma]

Well, I guess there are sort of two potential approaches to emitting it "later". One, we could emit something like ".word (endfunc-beginfunc)/4+0x10800000", as long as we have some conservative estimate we could use to prove that we don't need to split the unwind data. The other would be to use a dedicated, relaxable fragment, which could grow to accommodate splitting the unwind data if necessary. The dedicated fragment is probably a bit more bulletproof, and more consistent with the way we handle relaxation in other contexts, but it would require a lot of refactoring to implement.

I can put a FIXME into ARM64EmitUnwindInfo with the above paragraph, sure. (I don't think the WinException changes need a FIXME; even if we do fix the assembler side of this issue, there isn't any reason for the unwind table to provide unwind information for jump tables.)

[rnk]

In D61095#1477913, @efriedma wrote:

Well, I guess there are sort of two potential approaches to emitting it "later". One, we could emit something like ".word (endfunc-beginfunc)/4+0x10800000", as long as we have some conservative estimate we could use to prove that we don't need to split the unwind data. The other would be to use a dedicated, relaxable fragment, which could grow to accommodate splitting the unwind data if necessary. The dedicated fragment is probably a bit more bulletproof, and more consistent with the way we handle relaxation in other contexts, but it would require a lot of refactoring to implement.

Oh, yes, that would be the usual way of doing things, build a symbolic MCExpr for it. With the inline line table encoding, we couldn't do that because the label differences get encoded with a variable length encoding. Anyway, a FIXME is fine.

[dmajor]

I've confirmed that this fixes the Firefox repro, thanks!

[mstorsjo]

@efriedma - what's the status on this one?

[efriedma]

I've been distracted by some other work; I'll try to address the review comments today or tomorrow.

[efriedma]

Added FIXME to MCWin64EH.cpp.

[rnk]

lgtm, thanks!

llvm/trunk/lib/CodeGen/AsmPrinter/WinException.h

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.
   int getFrameIndexOffset(int FrameIndex, const WinEHFuncInfo &FuncInfo);
 
+  void endFuncletImpl();
 public:
   //===--------------------------------------------------------------------===//
   // Main entry points.
   //
   WinException(AsmPrinter *A);
   ~WinException() override;
 
   /// Emit all exception information that should come after the content.
   void endModule() override;
 
   /// Gather pre-function exception information.  Assumes being emitted
   /// immediately after the function entry point.
   void beginFunction(const MachineFunction *MF) override;
 
+  void markFunctionEnd() override;
+
   /// Gather and emit post-function exception information.
   void endFunction(const MachineFunction *) override;
 
   /// Emit target-specific EH funclet machinery.
   void beginFunclet(const MachineBasicBlock &MBB, MCSymbol *Sym) override;
   void endFunclet() override;
 };
 }
 
 #endif
 

llvm/trunk/lib/CodeGen/AsmPrinter/WinException.cpp

if (!Asm->MAI->usesWindowsCFI()) {
     shouldEmitLSDA = hasEHFunclets;
     shouldEmitPersonality = false;
     return;
   }
 
   beginFunclet(MF->front(), Asm->CurrentFnSym);
 }
 
+void WinException::markFunctionEnd() {
+  if (isAArch64 && CurrentFuncletEntry &&
+      (shouldEmitMoves || shouldEmitPersonality))
+    Asm->OutStreamer->EmitWinCFIFuncletOrFuncEnd();
+}
+
 /// endFunction - Gather and emit post-function exception information.
 ///
 void WinException::endFunction(const MachineFunction *MF) {
   if (!shouldEmitPersonality && !shouldEmitMoves && !shouldEmitLSDA)
     return;
 
   const Function &F = MF->getFunction();
   EHPersonality Per = EHPersonality::Unknown;
   if (F.hasPersonalityFn())
     Per = classifyEHPersonality(F.getPersonalityFn()->stripPointerCasts());
 
   // Get rid of any dead landing pads if we're not using funclets. In funclet
   // schemes, the landing pad is not actually reachable. It only exists so
   // that we can emit the right table data.
   if (!isFuncletEHPersonality(Per)) {
     MachineFunction *NonConstMF = const_cast<MachineFunction*>(MF);
     NonConstMF->tidyLandingPads();
   }
 
-  endFunclet();
+  endFuncletImpl();
 
   // endFunclet will emit the necessary .xdata tables for x64 SEH.
   if (Per == EHPersonality::MSVC_Win64SEH && MF->hasEHFunclets())
     return;
 
   if (shouldEmitPersonality || shouldEmitLSDA) {
     Asm->OutStreamer->PushSection();
 
if (shouldEmitPersonality) {
     // inliner doesn't allow inlining them, this isn't a major problem in
     // practice.
     if (!CurrentFuncletEntry->isCleanupFuncletEntry())
       Asm->OutStreamer->EmitWinEHHandler(PersHandlerSym, true, true);
   }
 }
 
 void WinException::endFunclet() {
+  if (isAArch64 && CurrentFuncletEntry &&
+      (shouldEmitMoves || shouldEmitPersonality)) {
+    Asm->OutStreamer->SwitchSection(CurrentFuncletTextSection);
+    Asm->OutStreamer->EmitWinCFIFuncletOrFuncEnd();
+  }
+  endFuncletImpl();
+}
+
+void WinException::endFuncletImpl() {
   // No funclet to process?  Great, we have nothing to do.
   if (!CurrentFuncletEntry)
     return;
 
   const MachineFunction *MF = Asm->MF;
   if (shouldEmitMoves || shouldEmitPersonality) {
     const Function &F = MF->getFunction();
     EHPersonality Per = EHPersonality::Unknown;
     if (F.hasPersonalityFn())
       Per = classifyEHPersonality(F.getPersonalityFn()->stripPointerCasts());
 
     // On funclet exit, we emit a fake "function" end marker, so that the call
     // to EmitWinEHHandlerData below can calculate the size of the funclet or
     // function.
     if (isAArch64) {
-      Asm->OutStreamer->SwitchSection(CurrentFuncletTextSection);
-      Asm->OutStreamer->EmitWinCFIFuncletOrFuncEnd();
       MCSection *XData = Asm->OutStreamer->getAssociatedXDataSection(
           Asm->OutStreamer->getCurrentSectionOnly());
       Asm->OutStreamer->SwitchSection(XData);
     }
 
     // Emit an UNWIND_INFO struct describing the prologue.
     Asm->OutStreamer->EmitWinEHHandlerData();
 

llvm/trunk/lib/MC/MCWin64EH.cpp

 
 static int64_t GetAbsDifference(MCStreamer &Streamer, const MCSymbol *LHS,
                                 const MCSymbol *RHS) {
   MCContext &Context = Streamer.getContext();
   const MCExpr *Diff =
       MCBinaryExpr::createSub(MCSymbolRefExpr::create(LHS, Context),
                               MCSymbolRefExpr::create(RHS, Context), Context);
   MCObjectStreamer *OS = (MCObjectStreamer *)(&Streamer);
+  // It should normally be possible to calculate the length of a function
+  // at this point, but it might not be possible in the presence of certain
+  // unusual constructs, like an inline asm with an alignment directive.
   int64_t value;
-  Diff->evaluateAsAbsolute(value, OS->getAssembler());
+  if (!Diff->evaluateAsAbsolute(value, OS->getAssembler()))
+    report_fatal_error("Failed to evaluate function length in SEH unwind info");
   return value;
 }
 
 static uint32_t
 ARM64CountOfUnwindCodes(const std::vector<WinEH::Instruction> &Insns) {
   uint32_t Count = 0;
   for (const auto &I : Insns) {
     switch (static_cast<Win64EH::UnwindOpcodes>(I.Operation)) {
static void ARM64EmitUnwindInfo(MCStreamer &streamer, WinEH::FrameInfo *info) {
 
   MCContext &context = streamer.getContext();
   MCSymbol *Label = context.createTempSymbol();
 
   streamer.EmitValueToAlignment(4);
   streamer.EmitLabel(Label);
   info->Symbol = Label;
 
-  uint32_t FuncLength = 0x0;
-  if (info->FuncletOrFuncEnd)
-    FuncLength = (uint32_t)GetAbsDifference(streamer, info->FuncletOrFuncEnd,
-                                            info->Begin);
-  FuncLength /= 4;
+  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;
+  } 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,
+    // we could emit something like ".word (endfunc-beginfunc)/4+0x10800000",
+    // as long as we have some conservative estimate we could use to prove
+    // that we don't need to split the unwind data. Emitting the constant
+    // is straightforward, but there's no existing code for estimating the
+    // size of the function.
+    //
+    // The other approach would be to use a dedicated, relaxable fragment,
+    // which could grow to accommodate splitting the unwind data if
+    // necessary. This is more straightforward, since it automatically works
+    // without any new infrastructure, and it's consistent with how we handle
+    // relaxation in other contexts.  But it would require some refactoring
+    // to move parts of the pdata/xdata emission into the implementation of
+    // a fragment. We could probably continue to encode the unwind codes
+    // here, but we'd have to emit the pdata, the xdata header, and the
+    // epilogue scopes later, since they depend on whether the we need to
+    // split the unwind data.
+    RawFuncLength = GetAbsDifference(streamer, info->FuncletOrFuncEnd,
+  }
+  if (RawFuncLength > 0xFFFFF)
+    report_fatal_error("SEH unwind data splitting not yet implemented");
+  uint32_t FuncLength = (uint32_t)RawFuncLength / 4;
   uint32_t PrologCodeBytes = ARM64CountOfUnwindCodes(info->Instructions);
   uint32_t TotalCodeBytes = PrologCodeBytes;
 
   // Process epilogs.
   MapVector<MCSymbol *, uint32_t> EpilogInfo;
   // Epilogs processed so far.
   std::vector<MCSymbol *> AddedEpilogs;
 

llvm/trunk/test/CodeGen/AArch64/win64-jumptable.ll

 ; RUN: llc -o - %s -mtriple=aarch64-windows -aarch64-enable-compress-jump-tables=0 | FileCheck %s
+; RUN: llc -o - %s -mtriple=aarch64-windows -aarch64-enable-compress-jump-tables=0 -filetype=obj | llvm-readobj -unwind | FileCheck %s -check-prefix=UNWIND
 
 define void @f(i32 %x) {
 entry:
   switch i32 %x, label %sw.epilog [
     i32 0, label %sw.bb
     i32 1, label %sw.bb1
     i32 2, label %sw.bb2
     i32 3, label %sw.bb3

 ; CHECK:    .word .LBB0_2-.LJTI0_0
 ; CHECK:    .word .LBB0_3-.LJTI0_0
 ; CHECK:    .word .LBB0_4-.LJTI0_0
 ; CHECK:    .word .LBB0_5-.LJTI0_0
 ; CHECK:    .section  .xdata,"dr"
 ; CHECK:    .seh_handlerdata
 ; CHECK:    .text
 ; CHECK:    .seh_endproc
+
+; Check that we can emit an object file with correct unwind info.
+; UNWIND: FunctionLength: {{[1-9][0-9]*}}