[x86] Tail merging two variadic calls produces incorrect CFI

[rnk]

Bugzilla Link	49130
Version	trunk
OS	All
CC	@bjope,@topperc,@jmorse,@RKSimon,@nickdesaulniers,@pogo59,@phoebewang,@rotateright,@stephenhines

Extended Description

This comes up in the Linux kernel, as reported here:
ClangBuiltLinux/linux#612
https://lkml.org/lkml/2019/7/16/759

Consider:

$ cat t.c

int a, b;
void mypanic(int, ...) __attribute__((noreturn));
void imbalanced() {
  if (a)
    mypanic(0, 0, 0, 0, 0, 0, a);
  if (b)
    mypanic(0, 0, 0, 0, 0, 0, a, 0);
}

$ clang -Os t.c -S -o -

        .text
        .file   "t.c"
        .globl  imbalanced                      # -- Begin function imbalanced
        .type   imbalanced,@function
imbalanced:                             # @​imbalanced
        .cfi_startproc
# %bb.0:                                # %entry
        pushq   %rax
        .cfi_def_cfa_offset 16
        movl    a(%rip), %r10d
        testl   %r10d, %r10d
        jne     .LBB0_3
# %bb.1:                                # %if.end
        cmpl    $0, b(%rip)
        jne     .LBB0_5
# %bb.2:                                # %if.end3
        popq    %rax
        .cfi_def_cfa_offset 8
        retq
.LBB0_3:                                # %if.then
        .cfi_def_cfa_offset 16
        subq    $8, %rsp
        .cfi_adjust_cfa_offset 8
        xorl    %edi, %edi
        xorl    %esi, %esi
        xorl    %edx, %edx
        xorl    %ecx, %ecx
        xorl    %r8d, %r8d
        xorl    %r9d, %r9d
        xorl    %eax, %eax
        jmp     .LBB0_4
.LBB0_5:                                # %if.then2
        .cfi_def_cfa_offset 16
        xorl    %r10d, %r10d
        xorl    %edi, %edi
        xorl    %esi, %esi
        xorl    %edx, %edx
        xorl    %ecx, %ecx
        xorl    %r8d, %r8d
        xorl    %r9d, %r9d
        xorl    %eax, %eax
        pushq   %r10
        .cfi_adjust_cfa_offset 8
.LBB0_4:                                # %if.then
        pushq   %r10
        .cfi_adjust_cfa_offset 8
        callq   mypanic
.Lfunc_end0:
        .size   imbalanced, .Lfunc_end0-imbalanced
        .cfi_endproc
                                        # -- End function

What we have here is two calls to mypanic, one with 7 args, one with 8. One call does two pushes, and the other does one. If you interpret the .cfi_* directives as written, they are only correct if the program executes the second 'if' branch, not the first. If the program executes the first 'if' and attempts to unwind the stack, the wrong return address will be used.

Perhaps the CFI inserter pass could fix this by adding the full cfa offset to each .cfi_adjust_cfa_offset instruction, or by adding some kind of no-op marker instruction to the end of the MBB that encodes the cfa. This would prevent tail merging from considering these blocks to be identical.

[nickdesaulniers]

Thank you for a concise description of the problem; I agree with the description (though I don't know that the noreturn attribute is required to reproduce).

Perhaps the CFI inserter pass could fix this by adding the full cfa offset
to each .cfi_adjust_cfa_offset instruction, or by adding some kind of no-op
marker instruction to the end of the MBB that encodes the cfa. This would
prevent tail merging from considering these blocks to be identical.

ComputeCommonTailLength() in llvm/lib/CodeGen/BranchFolding.cpp calls skipBackwardPastNonInstructions() calls countsAsInstruction() to skip over debug instructions and CFI instructions. Perhaps it should not?

[rnk]

I thought it was supposed to look at CFI instructions to avoid these types of issues.

[nickdesaulniers]

If you interpret the .cfi_* directives as written, they are only correct if the program executes the second 'if' branch, not the first. If the program executes the first 'if' and attempts to unwind the stack, the wrong return address will be used.

llvm/lib/Target/X86/X86FrameLowering.cpp seems to be doing a lot of .cfi_* directive emission. Perhaps a bug in there?

[nickdesaulniers]

cc @phoebewang

(side note: looking at the generated asm from this case, I also noticed that it seems that in the parameter setup for the call, by pushing the stack args last, we pessimize our ability to zip common tails together. If we pushed %r10 first, then we could zip more of the common tails)

[llvmbot]

@llvm/issue-subscribers-debuginfo