[PATCH] D42848: Correct dwarf unwind information in function epilogue

[Francis Visoiu Mistrih via Phabricator via llvm-commits]

thegameg added a comment.

Thanks for bringing this up again! Sorry for taking so long to take another look at it.



================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:53
+#ifndef NDEBUG
+    unsigned ErrorNum = verify(MF);
+    if (ErrorNum)
----------------
This can be:
```
if (unsigned ErrorNum = verify(MF))
```


================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:182
+        // TODO: Add support for handling cfi_remember_state.
+#ifndef NDEBUG
+        report_fatal_error(
----------------
Why are the `report_fatal_error`s guarded by `NDEBUG` here?


================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:204
+      case MCCFIInstruction::OpWindowSave:
+      case MCCFIInstruction::OpGnuArgsSize:
+        break;
----------------
I suppose these also need a TODO. I would also write somewhere that the CSRs are not yet handled by this pass.


================
Comment at: test/CodeGen/X86/avx512-regcall-Mask.ll:196
 ; LINUXOSX64-NEXT:    callq test_argv64i1
-; LINUXOSX64-NEXT:    addq $24, %rsp
+; LINUXOSX64-NEXT:    addq $16, %rsp
 ; LINUXOSX64-NEXT:    .cfi_adjust_cfa_offset -16
----------------
I'm guessing this was not intended. I can see that [[ https://github.com/llvm-mirror/llvm/blob/0cbbf04d21537d4a4bdc513725e1c6cf9345f09c/lib/Target/X86/X86FrameLowering.cpp#L412 | X86FrameLowering::mergeSPUpdate ]] doesn't support merging instructions with CFI.
I think you should add support for that to avoid regressions like this one.


================
Comment at: test/CodeGen/X86/frame-lowering-debug-intrinsic-2.ll:20
 
 ; CHECK-LABEL: noDebug
+; CHECK:        addq	$16, %rsp
----------------
Same here.


================
Comment at: test/CodeGen/X86/push-cfi-debug.ll:26
 ; CHECK: .cfi_adjust_cfa_offset -8
-; CHECK: addl $20, %esp
+; CHECK: addl $8, %esp
 ; CHECK: .cfi_adjust_cfa_offset -8
----------------
Same here.


================
Comment at: test/CodeGen/X86/push-cfi-obj.ll:15
 ; LINUX-NEXT:    Offset: 0x5C
-; LINUX-NEXT:    Size: 64
+; LINUX-NEXT:    Size: 72
 ; LINUX-NEXT:    Link: 0
----------------
As expected, this can prove that .eh_frame is going to get bigger. If you feel like doing it, it would be nice to see what the % size increase is for something like the test-suite.


================
Comment at: test/CodeGen/X86/push-cfi.ll:77
 ; LINUX-NEXT: call
-; LINUX-NEXT: addl $28, %esp
+; LINUX-NEXT: addl $16, %esp
 ; LINUX: .cfi_adjust_cfa_offset -16
----------------
Same here.


Repository:
  rL LLVM

https://reviews.llvm.org/D42848