Skip to content

Conversation

@tlively
Copy link
Member

@tlively tlively commented Nov 15, 2018

This yields more useful information when something goes wrong.

This yields more useful information when something goes wrong.
@tlively tlively requested a review from kripken November 15, 2018 07:08
@kripken
Copy link
Member

kripken commented Nov 15, 2018

An assert would only happen if not NDEBUG, so this changes the behavior in a release build without asserts (note that our CI only builds release+asserts currently, so we are missing possible compiler failures from a change like this, due to not having a noreturn function there).

How about using an assert when it would have an effect, and __builtin_unreachable otherwise? Do you know what LLVM does for unreachable?

@tlively tlively changed the title [ci skip] Use assert instead of abort for WASM_UNREACHABLE Use assert in addition to abort for WASM_UNREACHABLE Nov 15, 2018
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm with that fix

# include <assert.h>
# include <stdlib.h>
# define WASM_UNREACHABLE() abort()
# define WASM_UNREACHABLE() assert(false); abort()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should enclose both in { } as otherwise bad stuff would happen with this:

if (..) WASM_UNREACHABLE();

@tlively tlively merged commit c0cf612 into WebAssembly:master Nov 16, 2018
@tlively tlively deleted the assert branch November 16, 2018 01:28
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

Successfully merging this pull request may close these issues.

2 participants