Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add string parameter to WASM_UNREACHABLE #2499

Merged
merged 3 commits into from Dec 5, 2019
Merged

Add string parameter to WASM_UNREACHABLE #2499

merged 3 commits into from Dec 5, 2019

Conversation

sbc100
Copy link
Member

@sbc100 sbc100 commented Dec 4, 2019 •
edited

This works more like llvm's unreachable handler.

@sbc100 sbc100 requested a review from tlively December 4, 2019 20:34
sbc100 added a commit that referenced this pull request Dec 4, 2019
@sbc100
Re-enable use of NDEBUG in release builds
a25f7ee 
I'm not sure why this was disabled in the first place.  If we want
more information from WASM_UNREACHABLE to be preserved even in release
builds I could do that as a followup to #2499.
@sbc100 sbc100 mentioned this pull request Dec 4, 2019
Merged
tlively
tlively reviewed Dec 4, 2019
View reviewed changes
Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

What do you think about switching from the WASM_ prefix to a BYN_ prefix? Also cc @kripken for that question.

@sbc100
Copy link
Member Author

sbc100 commented Dec 4, 2019

Yeah, I was going to do that too.. maybe lets do that as a followup and we can do it for all the macros in binaryen?

tlively
tlively reviewed Dec 4, 2019
View reviewed changes
Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

There's a slight regression here in that the messages can no longer print any information about local variables, but that seems acceptable because these don't trigger unless more debugging is needed anyway.

@sbc100
Copy link
Member Author

sbc100 commented Dec 4, 2019

They can, but you have to jump through hoops a little bit (std::string("message: ") + var).c_str() I think will work, but its ugly. We could make it variadic if we want? Mostly I'm just copying llvm here.

kripken
kripken approved these changes Dec 4, 2019
View reviewed changes
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.

Another step in the LLVM-ification of Binaryen ;)

@kripken
Copy link
Member

kripken commented Dec 4, 2019

BYN_ prefix might make sense, yeah.

@sbc100
Add string parameter to WASM_UNREACHABLE
6a5e913 
This works more like llvm's unreachable handler in that is preserves
information even in release builds (where asserts are disabled).

Right now binaryen always builds with asserts on, even in release builds
mains so that WASM_UNREACHABLE can report useful information.  However
with this new method we can turn on asserts and still have
WASM_UNREACHABLE be useful.
@sbc100
fix
ad56d7f
@sbc100 sbc100 merged commit a28343a into master Dec 5, 2019
@sbc100 sbc100 deleted the unreachable branch December 5, 2019 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tlively tlively tlively left review comments

@kripken kripken kripken approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sbc100 @kripken @tlively