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

Raise validation error if referring to unknown global or setting immutable global #351

Merged
merged 4 commits into from
May 30, 2020

Conversation

axic
Copy link
Member

@axic axic commented May 27, 2020

No description provided.

lib/fizzy/parser_expr.cpp Outdated Show resolved Hide resolved
@gumb0 gumb0 force-pushed the validate-globals branch 3 times, most recently from d253fcd to b1ec2cf Compare May 28, 2020 18:20
@@ -51,6 +51,9 @@ using TableIdx = uint32_t;
// https://webassembly.github.io/spec/core/syntax/modules.html#syntax-memidx
Copy link
Collaborator

Choose a reason for hiding this comment

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

BTW these links now go to 1.1 spec

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you want to open an issue/PR for us to direct-link to 1.0?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@gumb0
Copy link
Collaborator

gumb0 commented May 29, 2020

Init expressions of globals need to be validated, too, but I would do that in another PR, as it seems a bit more complicated.

@codecov
Copy link

codecov bot commented May 29, 2020

Codecov Report

Merging #351 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #351      +/-   ##
==========================================
+ Coverage   98.84%   98.86%   +0.01%     
==========================================
  Files          40       40              
  Lines       11643    11779     +136     
==========================================
+ Hits        11509    11645     +136     
  Misses        134      134              

@gumb0 gumb0 marked this pull request as ready for review May 29, 2020 13:57
@gumb0 gumb0 requested a review from chfast May 29, 2020 13:58
@gumb0
Copy link
Collaborator

gumb0 commented May 29, 2020

@axic This is ready, please review

@@ -74,5 +74,13 @@ struct Module
{
return !memorysec.empty() || !imported_memory_types.empty();
}

bool is_global_mutable(GlobalIdx idx) const noexcept
Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, shouldn't we have a unit test for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

For the sake of progress I'm merging this, but perhaps we should add unit tests for each module helper in a new PR>

@axic axic merged commit e6187e9 into master May 30, 2020
@axic axic deleted the validate-globals branch May 30, 2020 15:31
@gumb0
Copy link
Collaborator

gumb0 commented Jun 2, 2020

Globals validation is poorly covered by spec tests, I added some cases to "to upstream" list

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.

3 participants