[Regression] Stack pointer corruption on ARMv7

[vit9696]

There is a stack pointer corruption regression in LLVM 13 causing stack leak (at least) with ASAN enabled when targeting cortex-a9.

The issue is reproducible with the code below:

void puts(const char *format);

void func(int a1, int a2, int* a3, int* a4, int* a5)
{
  (void)a1;
  (void)a2;
  (void)a3;
  (void)a4;
  (void)a5;
  puts("Hello");
}

int func2(void) {
  return 0;
}

void test(void) {
  static int a;
  int e;
  int c;
  int d;
  int b = 0;

  for (;;)
  {
    if (func2() == 0)
    {
      func(a, b, &c, &d, &e);
    }
  }
}

It can also be viewed online. The arguments to compile are as follows:

clang -o error.o -c -target arm-gnu-linux-eabi -mcpu=cortex-a9 -fsanitize=address error.c

The problematic part is the loop in the test function. When func is called, LLVM does not adjust the stack back, causing fast stack overflow:

.LBB2_5:                                @ %for.cond
       ...
       sub     sp, sp, #8
       str     r12, [sp]
       bl      func
.LBB2_7:                                @ %if.end
       b       .LBB2_5

LLVM 12.0.1 does not have this issue. The issue is present in main and is caused by d88f96d. Differential: https://reviews.llvm.org/D102613.

Reverting the patch resolves the problem, but I am not well familiar with that part of LLVM to drive the change.

CC @tstellar, this should be a blocker for 13.0.1 as it is a regression.
CC @TNorthover, @ornata, could you provide any information on a proper fix?

0001-Revert-ARM-support-mandatory-tail-calls-for-tailcc-s.patch.zip

[vit9696]

Referencing #51489.

[tstellar]

@TNorthover Should we revert d88f96d this on the release/13.x branch?

[TNorthover]

Oh dear. If it reverts cleanly that's probably the sensible choice. Swift is the only project that really cares about the feature and it has its own branching.

I'll hang onto this issue to sort out a proper fix when I get back to work.

[vit9696]

Thanks Tim! The good side of it is that we have reverted it for a about a month now (with the patch above) and so far spotted no issues. We only use C and very little C++ though.

[TNorthover]

The arguments for ADJCALLSTACKUP had changed but FastISel (& GISel) hadn't got the message. Patch up for review now: https://reviews.llvm.org/D116805

[tstellar]

The arguments for ADJCALLSTACKUP had changed but FastISel (& GISel) hadn't got the message. Patch up for review now: https://reviews.llvm.org/D116805

Some test cases fail when I try to build with the attached patch to revert. Would it be better to backport https://reviews.llvm.org/D116805 instead?

[vit9696]

We will test https://reviews.llvm.org/D116805 on 13.0.0 branch against this issue tomorrow or so.

[tstellar]

@vit9696 OK, I want to tag the release on Wednesday at the latest, so the sooner you can test and ACK the patch, the better.

[vit9696]

Done, as long as the tests pass, I believe it is a much better approach than reverting.

[TNorthover]

Should be fixed on main now by 0b5b35f.

[tstellar]

Merged: eaeb7dc