[MSVC][EH]clang-cl results in uncaught exception

[Naville]

Case 1

#include <malloc.h>
#include <stdio.h>

struct Exn {};

int main() {
  try {
    static bool StaticVar = false;
    if (!StaticVar) {
      StaticVar = true;
    }

    volatile void* used = alloca(4);

    try {
      throw Exn();
    } catch (Exn &e) {
      throw;
    }
  } catch (Exn &) {
  }

  printf("Good\n");
  return 0;
}

Case 2

#include <malloc.h>
#include <stdio.h>

struct Exn {};

void Foo()
{
    static bool b;
    if (!b)
    {
        printf("Foo 1st time\n");
        b = true;
    }
    printf("Alloca: %p\n", alloca(4));
    try
    {
        throw Exn();
    }
    catch (Exn& e)
    {
        throw e;
    }
}

int main()
{
    try
    {
        Foo();
        printf("FOO\n");
    }
    catch (Exn&)
    {
        printf("EXN\n");
    }
}

We're currently using 16.0.0-RC2

[Naville]

executables.zip

[Naville]

CODE_OWNERS.txt told me to @asl Clang-cl history seems to point to @majnemer Sorry for the ping if unrelated

[EugeneZelenko]

@Naville: You could use https://godbolt.org to demonstrate problems.

[Naville]

@Naville: You could use https://godbolt.org to demonstrate problems.

@EugeneZelenko godbolt doesn't have clang-cl IIRC

[smeenai]

@rnk used to be the best POC for Windows exception handling in LLVM, though I'm not sure if he's still active in the area. @compnerd is very experienced with it as well.

[Naville]

This is probably also X86 backend issue as the output assembly shows that the framepointer used by SEH is polluted.

[phoebewang]

This looks like the same problem I noticed days before. Confirmed it will failed with the assert in my patch The root cause is WinEH doesn't support dynamic alloca in current implement. Maybe we can hoist the dynamic alloca out of SEH blocks. cc @clin111

[Naville]

Ping, I understand dynalloc hoisting would be difficult, but can we at least merge D143739 first?

[phoebewang]

Unfortunately, I found 4 test cases failed with D143739. Guess these tests may have runtime problems too, but need time to investigate.

Failed Tests (4):
  LLVM :: CodeGen/AArch64/funclet-match-add-sub-stack.ll
  LLVM :: CodeGen/AArch64/wineh-try-catch-vla.ll
  LLVM :: CodeGen/X86/catchpad-dynamic-alloca.ll
  LLVM :: CodeGen/X86/cleanuppad-inalloca.ll

[phoebewang]

@clin111 once put a hoist patch D145275 but dropped for some reason. I'm not sure if simple hoisting can solve all the problem.

[Naville]

In our use-case, merely hoisting some of the "safe" dynamic allocas would help a lot already.

Not sure how to implement this though 🤔

[phoebewang]

Maybe you can cherry-pick D145275 downstream?

[Naville]

I'm too afraid to merge that downstream given the reviewers mentioned it's unsafe :( I'd rather crash in backend rather than introducing unsafe code that crash at runtime

[clin111]

Patch was dropped for 2 reasons: LLVM architects do not like transformations that change the behavior of the program. And, hoisting alloca is generally unsafe. I believe that this specific case, where we hoist alloca used only in catchpad, is safe. The memory is not freed, and it is not large, so it should not matter where we allocate it. But, it is not a strong enough argument, as it does change the program behavior.

The fix only covers catchpad cases. There are many cases such as https://github.com/llvm/llvm-project/issues/60914 which are not covered.

So, this patch does not have high enough standards for permanent commit to open source. But, as a temporary workaround, I think it is OK.

[Naville]

ty, given the size of our project it's probably wise to wait for a proper fix 👍 Any plan on that?

[Naville]

Gentle ping

[rnk]

Sorry, I was on leave until May and didn't see this. My recollection is that LLVM may not support WinEH with allocas. I think we never implemented the logic to save the current stack pointer and restore it after catching an exception.

I don't think we should resort to heroics to make this stuff work. If MSVC hoists these allocas, that's mostly easy enough and we try to implement that, but I'm hesitant to take on the fully complexity of emitting stacksave / stackrestore everywhere. Doing it right requires lots of careful testing with a long tail of issues that the LLVM project doesn't really have the bandwidth to support. That said, we should emit a clear error or warning that catching an exception will effectively "free" allocas and VLAs, crashing like this is a bug and a bad user experience.

[Naville]

I agree, however none of the IR check reviews are commited yet

( https://reviews.llvm.org/D143739 https://reviews.llvm.org/D151209 )