Remove assertion on CFI stack

[rafauler]

Summary

This assertion requires that we have an exact number of
remember/restore CFIs. That is unnecessarily strict. We should only
assert that we have more remember instructions than restore ones.

Fixes #61721

[yota9]

bolt/lib/Core/BinaryFunction.cpp

2415

Just wondering - we are not interested in operation types that might be in state stack? E.g. are we only expect to have OpRestoreState here or any kind?

[rafauler]

bolt/lib/Core/BinaryFunction.cpp

2415

What happens is that the CFI code may have RememberState at any given point in order to easily encode the CFI in another, future point of the function, but may decide to just never restore it if the compiler is not very careful. And the standard allows that. From the point of view of the unwinder, it doesn't care if some state was saved but never used anywhere, so should we.

[sinan]

Hi,

any progress on this patch? Those who try to bolt a program built by clang in AArch64 are likely encounters this unnecessary assertion failure.

1. CFIFixup is turned on in AArch64 and it could emit remember/restore cfi directives

2. AsmPrinter::emitCFIInstruction do not emit the restore cfi directive if the cfi directive is beyond the end of the function(e.g. this restore cfi is placed inside an empty basic block, which is put at the end of the function).

void AsmPrinter::emitCFIInstruction(const MachineInstr &MI) {
  ... ...
  // If there is no "real" instruction following this CFI instruction, skip
  // emitting it; it would be beyond the end of the function's FDE range.
  auto *MBB = MI.getParent();
  auto I = std::next(MI.getIterator());
  while (I != MBB->end() && I->isTransient())
    ++I;
  if (I == MBB->instr_end() &&
      MBB->getReverseIterator() == MBB->getParent()->rbegin())
    return;

#61971 is another issue encountered this problem.

bolt/lib/Core/BinaryFunction.cpp

for (const MCInst &Instr : *BB) {
       default:
         // Any other CFI updates the state.
         EffectiveState = State;
         break;
       }
     }
   }
 
-  assert(StateStack.empty() && "corrupt CFI stack");
+  // StateStack may not be empty here, it is OK to have items in the stack
+  // that were not popped at the end of CFIs.
 }
 
 namespace {
 
 /// Our full interpretation of a DWARF CFI machine state at a given point
 struct CFISnapshot {
   /// CFA register number and offset defining the canonical frame at this
   /// point, or the number of a rule (CFI state) that computes it with a

bolt/test/AArch64/Inputs/unbalanced_cfi.s

+  .text
+  .file "input.ll"
+  .globl  isKnownPredicate                // -- Begin function isKnownPredicate
+  .p2align  2
+  .type isKnownPredicate,@function
+isKnownPredicate:                       // @isKnownPredicate
+  .cfi_startproc
+// %bb.0:                               // %entry
+  stp  x29, x30, [sp, #-80]!           // 16-byte Folded Spill
+  .cfi_def_cfa_offset 80
+  str  x25, [sp, #16]                  // 8-byte Folded Spill
+  stp  x24, x23, [sp, #32]             // 16-byte Folded Spill
+  stp  x22, x21, [sp, #48]             // 16-byte Folded Spill
+  stp  x20, x19, [sp, #64]             // 16-byte Folded Spill
+  mov x29, sp
+  .cfi_def_cfa w29, 80
+  .cfi_offset w19, -8
+  .cfi_offset w20, -16
+  .cfi_offset w21, -24
+  .cfi_offset w22, -32
+  .cfi_offset w23, -40
+  .cfi_offset w24, -48
+  .cfi_offset w25, -64
+  .cfi_offset w30, -72
+  .cfi_offset w29, -80
+  .cfi_remember_state
+  mov x21, x3
+  mov x22, x2
+  mov w20, w1
+  mov x19, x0
+  and w8, w1, #0xfffffffe
+  cmp w8, #32
+  b.ne  .LBB0_2
+// %bb.1:                               // %if.then8
+  ldr x23, [x22, #8]
+  ldr x24, [x21, #8]
+  mov x0, x23
+  bl  getType
+  mov x25, x0
+  mov x0, x24
+  bl  getType
+  cmp x25, x0
+  csel  x22, x23, x22, eq
+  csel  x21, x24, x21, eq
+.LBB0_2:                                // %if.end18
+  ldr x0, [x19, #8]
+  mov w1, w20
+  mov x2, x22
+  mov x3, x21
+  bl  isKnownPredicate
+  tbz w0, #0, .LBB0_4
+// %bb.3:                               // %return
+  mov w0, #1                          // =0x1
+  .cfi_def_cfa wsp, 80
+  ldp  x20, x19, [sp, #64]             // 16-byte Folded Reload
+  ldp  x22, x21, [sp, #48]             // 16-byte Folded Reload
+  ldp  x24, x23, [sp, #32]             // 16-byte Folded Reload
+  ldr  x25, [sp, #16]                  // 8-byte Folded Reload
+  ldp  x29, x30, [sp], #80             // 16-byte Folded Reload
+  .cfi_def_cfa_offset 0
+  .cfi_restore w19
+  .cfi_restore w20
+  .cfi_restore w21
+  .cfi_restore w22
+  .cfi_restore w23
+  .cfi_restore w24
+  .cfi_restore w25
+  .cfi_restore w30
+  .cfi_restore w29
+  ret
+.LBB0_4:                                // %if.end21
+  .cfi_restore_state
+  .cfi_remember_state
+  ldr x0, [x19, #8]
+  mov x1, x22
+  mov x2, x21
+  mov w3, wzr
+  mov w4, wzr
+  bl  getMinusSCEVE
+  sub w8, w20, #32
+  adrp  x9, .LJTI0_0
+  add x9, x9, :lo12:.LJTI0_0
+  mov x1, x0
+  adr x10, .LBB0_5
+  ldrb  w11, [x9, x8]
+  add x10, x10, x11, lsl #2
+  br  x10
+.LBB0_5:                                // %sw.bb
+  mov x0, x1
+  .cfi_def_cfa wsp, 80
+  ldp  x20, x19, [sp, #64]             // 16-byte Folded Reload
+  ldp  x22, x21, [sp, #48]             // 16-byte Folded Reload
+  ldp  x24, x23, [sp, #32]             // 16-byte Folded Reload
+  ldr  x25, [sp, #16]                  // 8-byte Folded Reload
+  ldp  x29, x30, [sp], #80             // 16-byte Folded Reload
+  .cfi_def_cfa_offset 0
+  .cfi_restore w19
+  .cfi_restore w20
+  .cfi_restore w21
+  .cfi_restore w22
+  .cfi_restore w23
+  .cfi_restore w24
+  .cfi_restore w25
+  .cfi_restore w30
+  .cfi_restore w29
+  b isZero
+.LBB0_6:                                // %sw.bb28
+  .cfi_restore_state
+  .cfi_remember_state
+  ldr x0, [x19, #8]
+  .cfi_def_cfa wsp, 80
+  ldp  x20, x19, [sp, #64]             // 16-byte Folded Reload
+  ldp  x22, x21, [sp, #48]             // 16-byte Folded Reload
+  ldp  x24, x23, [sp, #32]             // 16-byte Folded Reload
+  ldr  x25, [sp, #16]                  // 8-byte Folded Reload
+  ldp  x29, x30, [sp], #80             // 16-byte Folded Reload
+  .cfi_def_cfa_offset 0
+  .cfi_restore w19
+  .cfi_restore w20
+  .cfi_restore w21
+  .cfi_restore w22
+  .cfi_restore w23
+  .cfi_restore w24
+  .cfi_restore w25
+  .cfi_restore w30
+  .cfi_restore w29
+  b isKnownNonNegative
+.LBB0_7:                                // %sw.bb25
+  .cfi_restore_state
+  .cfi_remember_state
+  ldr x0, [x19, #8]
+  .cfi_def_cfa wsp, 80
+  ldp  x20, x19, [sp, #64]             // 16-byte Folded Reload
+  ldp  x22, x21, [sp, #48]             // 16-byte Folded Reload
+  ldp  x24, x23, [sp, #32]             // 16-byte Folded Reload
+  ldr  x25, [sp, #16]                  // 8-byte Folded Reload
+  ldp  x29, x30, [sp], #80             // 16-byte Folded Reload
+  .cfi_def_cfa_offset 0
+  .cfi_restore w19
+  .cfi_restore w20
+  .cfi_restore w21
+  .cfi_restore w22
+  .cfi_restore w23
+  .cfi_restore w24
+  .cfi_restore w25
+  .cfi_restore w30
+  .cfi_restore w29
+  b isKnownNonZero
+.LBB0_8:                                // %sw.bb34
+  .cfi_restore_state
+  .cfi_remember_state
+  ldr x0, [x19, #8]
+  .cfi_def_cfa wsp, 80
+  ldp  x20, x19, [sp, #64]             // 16-byte Folded Reload
+  ldp  x22, x21, [sp, #48]             // 16-byte Folded Reload
+  ldp  x24, x23, [sp, #32]             // 16-byte Folded Reload
+  ldr  x25, [sp, #16]                  // 8-byte Folded Reload
+  ldp  x29, x30, [sp], #80             // 16-byte Folded Reload
+  .cfi_def_cfa_offset 0
+  .cfi_restore w19
+  .cfi_restore w20
+  .cfi_restore w21
+  .cfi_restore w22
+  .cfi_restore w23
+  .cfi_restore w24
+  .cfi_restore w25
+  .cfi_restore w30
+  .cfi_restore w29
+  b isKnownPositive
+.LBB0_9:                                // %sw.bb31
+  .cfi_restore_state
+  .cfi_remember_state
+  ldr x0, [x19, #8]
+  .cfi_def_cfa wsp, 80
+  ldp  x20, x19, [sp, #64]             // 16-byte Folded Reload
+  ldp  x22, x21, [sp, #48]             // 16-byte Folded Reload
+  ldp  x24, x23, [sp, #32]             // 16-byte Folded Reload
+  ldr  x25, [sp, #16]                  // 8-byte Folded Reload
+  ldp  x29, x30, [sp], #80             // 16-byte Folded Reload
+  .cfi_def_cfa_offset 0
+  .cfi_restore w19
+  .cfi_restore w20
+  .cfi_restore w21
+  .cfi_restore w22
+  .cfi_restore w23
+  .cfi_restore w24
+  .cfi_restore w25
+  .cfi_restore w30
+  .cfi_restore w29
+  b isKnownNonPositive
+.LBB0_10:                               // %sw.bb37
+  .cfi_restore_state
+  .cfi_remember_state
+  ldr x0, [x19, #8]
+  .cfi_def_cfa wsp, 80
+  ldp  x20, x19, [sp, #64]             // 16-byte Folded Reload
+  ldp  x22, x21, [sp, #48]             // 16-byte Folded Reload
+  ldp  x24, x23, [sp, #32]             // 16-byte Folded Reload
+  ldr  x25, [sp, #16]                  // 8-byte Folded Reload
+  ldp  x29, x30, [sp], #80             // 16-byte Folded Reload
+  .cfi_def_cfa_offset 0
+  .cfi_restore w19
+  .cfi_restore w20
+  .cfi_restore w21
+  .cfi_restore w22
+  .cfi_restore w23
+  .cfi_restore w24
+  .cfi_restore w25
+  .cfi_restore w30
+  .cfi_restore w29
+  b isKnownNegative
+.LBB0_11:                               // %sw.default
+.Lfunc_end0:
+  .size isKnownPredicate, .Lfunc_end0-isKnownPredicate
+  .cfi_endproc
+  .section  .rodata,"a",@progbits
+.LJTI0_0:
+  .byte (.LBB0_5-.LBB0_5)>>2
+  .byte (.LBB0_7-.LBB0_5)>>2
+  .byte (.LBB0_11-.LBB0_5)>>2
+  .byte (.LBB0_11-.LBB0_5)>>2
+  .byte (.LBB0_11-.LBB0_5)>>2
+  .byte (.LBB0_11-.LBB0_5)>>2
+  .byte (.LBB0_8-.LBB0_5)>>2
+  .byte (.LBB0_6-.LBB0_5)>>2
+  .byte (.LBB0_10-.LBB0_5)>>2
+  .byte (.LBB0_9-.LBB0_5)>>2
+                                        // -- End function
+  .section  ".note.GNU-stack","",@progbits
+  .text
+  .globl  getType                         // -- Begin function getType
+  .p2align  2
+  .type getType,@function
+getType:                                // @getType
+  .cfi_startproc
+// %bb.0:                               // %entry
+  ret
+.Lfunc_end00:
+  .size getType, .Lfunc_end00-getType
+  .cfi_endproc
+                                        // -- End function
+  .globl  getMinusSCEVE                   // -- Begin function getMinusSCEVE
+  .p2align  2
+  .type getMinusSCEVE,@function
+getMinusSCEVE:                          // @getMinusSCEVE
+  .cfi_startproc
+// %bb.0:                               // %entry
+  ret
+.Lfunc_end1:
+  .size getMinusSCEVE, .Lfunc_end1-getMinusSCEVE
+  .cfi_endproc
+                                        // -- End function
+  .globl  isZero                          // -- Begin function isZero
+  .p2align  2
+  .type isZero,@function
+isZero:                                 // @isZero
+  .cfi_startproc
+// %bb.0:                               // %entry
+  ret
+.Lfunc_end2:
+  .size isZero, .Lfunc_end2-isZero
+  .cfi_endproc
+                                        // -- End function
+  .globl  isKnownNonZero                  // -- Begin function isKnownNonZero
+  .p2align  2
+  .type isKnownNonZero,@function
+isKnownNonZero:                         // @isKnownNonZero
+  .cfi_startproc
+// %bb.0:                               // %entry
+  ret
+.Lfunc_end3:
+  .size isKnownNonZero, .Lfunc_end3-isKnownNonZero
+  .cfi_endproc
+                                        // -- End function
+  .globl  isKnownNonNegative              // -- Begin function isKnownNonNegative
+  .p2align  2
+  .type isKnownNonNegative,@function
+isKnownNonNegative:                     // @isKnownNonNegative
+  .cfi_startproc
+// %bb.0:                               // %entry
+  ret
+.Lfunc_end4:
+  .size isKnownNonNegative, .Lfunc_end4-isKnownNonNegative
+  .cfi_endproc
+                                        // -- End function
+  .globl  isKnownNonPositive              // -- Begin function isKnownNonPositive
+  .p2align  2
+  .type isKnownNonPositive,@function
+isKnownNonPositive:                     // @isKnownNonPositive
+  .cfi_startproc
+// %bb.0:                               // %entry
+  ret
+.Lfunc_end5:
+  .size isKnownNonPositive, .Lfunc_end5-isKnownNonPositive
+  .cfi_endproc
+                                        // -- End function
+  .globl  isKnownPositive                 // -- Begin function isKnownPositive
+  .p2align  2
+  .type isKnownPositive,@function
+isKnownPositive:                        // @isKnownPositive
+  .cfi_startproc
+// %bb.0:                               // %entry
+  ret
+.Lfunc_end6:
+  .size isKnownPositive, .Lfunc_end6-isKnownPositive
+  .cfi_endproc
+                                        // -- End function
+  .globl  isKnownNegative                 // -- Begin function isKnownNegative
+  .p2align  2
+  .type isKnownNegative,@function
+isKnownNegative:                        // @isKnownNegative
+  .cfi_startproc
+// %bb.0:                               // %entry
+  ret
+.Lfunc_end7:
+  .size isKnownNegative, .Lfunc_end7-isKnownNegative
+  .cfi_endproc
+                                        // -- End function
+  .globl  main                            // -- Begin function main
+  .p2align  2
+  .type main,@function
+main:                                   // @main
+  .cfi_startproc
+// %bb.0:                               // %entry
+  sub sp, sp, #16
+  .cfi_def_cfa_offset 16
+  mov w0, wzr
+  str wzr, [sp, #12]
+  add sp, sp, #16
+  .cfi_def_cfa_offset 0
+  ret
+.Lfunc_end8:
+  .size main, .Lfunc_end8-main
+  .cfi_endproc

bolt/test/AArch64/unbalanced_cfi.test

+This test checks that BOLT does not unnecessarily fails to process a
+binary that doesn't run cfi_restore_state for every
+cfi_remember_state. That is not a hard requirement and we should
+tolerate this unbalance. We should only fail when trying to restore a
+state when the stack is empty.
+
+REQUIRES: system-linux
+
+RUN: %clang %cflags -no-pie %p/Inputs/unbalanced_cfi.s -Wl,-q -fuse-ld=lld \
+RUN:   -nostdlib -o %t.exe
+RUN: llvm-bolt %t.exe -o %t.bolt.exe --lite=0 -reorder-blocks=reverse