Skip to content

Wasm EH: "Branch destination should be in scope" error #13515

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kripken opened this issue Feb 16, 2021 · 6 comments
Closed

Wasm EH: "Branch destination should be in scope" error #13515

kripken opened this issue Feb 16, 2021 · 6 comments

Comments

@kripken
Copy link
Member

kripken commented Feb 16, 2021

clang++: /b/s/w/ir/cache/builder/emscripten-releases/llvm-project/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:1518: unsigned int (anonymous namespace)::WebAssemblyCFGStackify::getBranchDepth(const SmallVectorImpl<(anonymous namespace)::WebAssemblyCFGStackify::EndMarkerInfo> &, const llvm::MachineBasicBlock *): Assertion `Depth < Stack.size() && "Branch destination should be in scope"' failed.
#include <stdio.h> // avoid iostream C++ code, just test libc++abi, not libc++
#include <stdint.h>

extern void refuel();
extern void checkRecursion();
extern bool getBoolean();

void func_0() {
  if (getBoolean()) {
    try {
      throw 0;
    } catch(...) {
    }
  }
  try {
    if (getBoolean()) {
      try {
        puts("log(1)");
      } catch(...) {
      }
    }
  } catch(...) {
  }
}

int main() {
  // func_0
  puts("calling func_0");
  refuel();
  try {
    func_0();
  } catch(...) {
    puts("main caught from func_0");
  }
  return 0;
}

STR: emcc a.cpp -c -fwasm-exceptions

Found by #13485

cc @aheejin @dschuff

@kripken
Copy link
Member Author

kripken commented Feb 17, 2021

A smaller testcase:

extern bool getBoolean();

struct Class {
  ~Class();
};

int main() {
  if (getBoolean()) {
    try {
      throw 0;
    } catch(int) {
    }
  }
  Class instance1;
  if (getBoolean()) {
  }
  while (getBoolean()) {
  }
}

@dschuff
Copy link
Member

dschuff commented Feb 19, 2021

@aheejin I know you are currently working on #13514 (which I agree is best since that's the one we saw in real code). Do you have a sense for whether this is likely to be an independent bug from that? To me it looks like both of these result from control flow destinations not being in the stack, and a change to the grouping algorithm is likely to affect both of those (so in other words, my guess would be that it's not worth trying to investigate fixing this separately from a fix to #13514. does that sound right to you?)

@aheejin
Copy link
Member

aheejin commented Feb 19, 2021

I had to investigate this in order to find out whether it is independent or not, and it is independent. But also while investigating this I think I found out the cause for this bug too. So not sure if it is advisable to ask you to fix this separately.

@dschuff
Copy link
Member

dschuff commented Feb 20, 2021

OK yeah, if your tentative fix for #13514 works out, then it probably makes sense for you to pick this one up too.

@dschuff
Copy link
Member

dschuff commented Feb 20, 2021

btw If you want to upload something to phabricator I'd be happy to throw some testing at it too.

@kripken
Copy link
Member Author

kripken commented Feb 24, 2021

This appears to still happen after the fix landed, on other testcases. A reduced one:

extern bool getBoolean();

struct Class {
  Class();
  ~Class();
};

int main() {
  if (getBoolean()) {
    try {
      throw 0;
    } catch (int) {
    }
    try {
      if (getBoolean()) {
      }
      throw 0;
    } catch (int) {
    }
  }
  Class instance1;
  try {
    getBoolean();
  } catch (int) {
  }
  getBoolean();
}

STR: ./em++ a.cpp -c -fwasm-exceptions on #13485 (tot: 0fcee855ada6358c4fe6e8c2a7f291c54cde4876).

@kripken kripken reopened this Feb 24, 2021
morehouse pushed a commit to morehouse/llvm-project that referenced this issue Mar 4, 2021
Fixing catch unwind mismatches can sometimes invalidate existing branch
destinations. This CL remaps those destinations after placing
try-delegates.

Fixes emscripten-core/emscripten#13515.

Reviewed By: dschuff

Differential Revision: https://reviews.llvm.org/D97178
morehouse pushed a commit to morehouse/llvm-project that referenced this issue Mar 4, 2021
This is a case D97178 tried to solve but missed. D97178 could not handle
the case when
multiple consecutive delegates are generated:
- Before:
```
block
  br (a)
  try
  catch
  end_try
end_block
          <- (a)
```

- After
```
block
  br (a)
  try
    ...
    try
      try
      catch
      end_try
            <- (a)
    delegate
  delegate
end_block
          <- (b)
```
(The `br` should point to (b) now)

D97178 assumed `end_block` exists two BBs later than `end_try`, because
it assumed the order as `end_try` BB -> `delegate` BB -> `end_block` BB.
But it turned out there can be multiple `delegate`s in between. This
patch changes the logic so we just search from `end_try` BB until we
find `end_block`.

Fixes emscripten-core/emscripten#13515.
(More precisely, fixes
emscripten-core/emscripten#13515 (comment).)

Reviewed By: dschuff, tlively

Differential Revision: https://reviews.llvm.org/D97569
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this issue Oct 7, 2022
Fixing catch unwind mismatches can sometimes invalidate existing branch
destinations. This CL remaps those destinations after placing
try-delegates.

Fixes emscripten-core/emscripten#13515.

Reviewed By: dschuff

Differential Revision: https://reviews.llvm.org/D97178
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this issue Oct 7, 2022
This is a case D97178 tried to solve but missed. D97178 could not handle
the case when
multiple consecutive delegates are generated:
- Before:
```
block
  br (a)
  try
  catch
  end_try
end_block
          <- (a)
```

- After
```
block
  br (a)
  try
    ...
    try
      try
      catch
      end_try
            <- (a)
    delegate
  delegate
end_block
          <- (b)
```
(The `br` should point to (b) now)

D97178 assumed `end_block` exists two BBs later than `end_try`, because
it assumed the order as `end_try` BB -> `delegate` BB -> `end_block` BB.
But it turned out there can be multiple `delegate`s in between. This
patch changes the logic so we just search from `end_try` BB until we
find `end_block`.

Fixes emscripten-core/emscripten#13515.
(More precisely, fixes
emscripten-core/emscripten#13515 (comment).)

Reviewed By: dschuff, tlively

Differential Revision: https://reviews.llvm.org/D97569
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants