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.

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

Finish updating testsuite #2001

Merged
merged 2 commits into from
Oct 3, 2022
Merged

Finish updating testsuite #2001

merged 2 commits into from
Oct 3, 2022

Conversation

keithw
Copy link
Member

@keithw keithw commented Sep 21, 2022

The testsuite was already updated in #2003, so this just runs update-spec-tests.py to bring in the new tokens.wast test. The Wast lexer changes are necessary because of WebAssembly/spec#1499 . There may be a more elegant way to make this change, though...

@keithw keithw mentioned this pull request Sep 22, 2022
@keithw keithw force-pushed the update-testsuite branch 2 times, most recently from 1c2c6af to bb0dc9c Compare October 1, 2022 05:26
@keithw keithw changed the title Update testsuite Finish updating testsuite (adding new tokens.wast test) Oct 3, 2022
@keithw keithw changed the title Finish updating testsuite (adding new tokens.wast test) Finish updating testsuite Oct 3, 2022
@keithw keithw requested a review from sbc100 October 3, 2022 06:42
@keithw keithw marked this pull request as ready for review October 3, 2022 06:42
#define ERROR(...) parser->Error(GetLocation(), __VA_ARGS__)
#define ERROR(...) \
if (parser) \
parser->Error(GetLocation(), __VA_ARGS__)
Copy link
Member

Choose a reason for hiding this comment

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

Why was this needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

With the change in tokenization, ReadReservedChars() now needs to be able to call GetStringToken() if it sees a " character, but GetStringToken takes a WastParser named parser so it can call the ERROR macro, and ReadReservedChars() is called in a whole bunch of places and takes no arguments. So... this makes it possible for ReadReservedChars() to call GetStringToken(nullptr) without causing a crash if that wants to log an error.

Copy link
Member

Choose a reason for hiding this comment

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

But won't this mean that such errors can/will be lost?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, now that strings can be part of a reserved token, we would lose the log message if an invalid string is found while running NoTrailingReservedChars(). I think the best/cleanest fix for this is #2013, which just makes the WastLexer work similar to a WastParser (it receives an Errors* on construction and stores it so it can log errors from any context).

@keithw keithw merged commit a2c0d17 into main Oct 3, 2022
@keithw keithw deleted the update-testsuite branch October 3, 2022 17:40
matthias-blume pushed a commit to matthias-blume/wabt that referenced this pull request Dec 16, 2022
…ly#2001)

* Update testsuite (adding new tokens.txt test)

* Adjust Wast lexing to match updated spec (WebAssembly/spec#1499)
keithw added a commit that referenced this pull request Feb 25, 2023
This had been crashing even with annotations disabled.
Adds a regression test.

This was missed in #2001 when updating the parser to match
the updated spec (WebAssembly/spec#1499).

Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=53935
keithw added a commit that referenced this pull request Feb 25, 2023
This had been crashing even with annotations disabled.
Adds a regression test.

This was missed in #2001 when updating the parser to match
the updated spec (WebAssembly/spec#1499).

Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=53935
keithw added a commit that referenced this pull request Feb 25, 2023
This had been crashing even with annotations disabled.
Adds a regression test.

This was missed in #2001 when updating the lexer to match
the updated spec (WebAssembly/spec#1499).

Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=53935
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