Conversation
Codecov Report
@@ Coverage Diff @@
## master #357 +/- ##
========================================
Coverage 98.84% 98.84%
========================================
Files 38 38
Lines 11468 11571 +103
========================================
+ Hits 11335 11437 +102
- Misses 133 134 +1 |
gumb0
force-pushed
the
validate-exports
branch
2 times, most recently
from
38a45eb to
05a3574
Compare
| throw validation_error{"invalid index of an exported function"}; | ||
| break; | ||
| case ExternalKind::Table: | ||
| if (export_.index != 0 || !module.has_table()) |
There was a problem hiding this comment.
Changed to using module class helpers
| module.imported_globals_mutability.size() + module.globalsec.size(); | ||
|
|
||
| // Validate exports. | ||
| std::unordered_set<std::string_view> export_names; |
There was a problem hiding this comment.
Is this much faster than vector or what is the reason?
There was a problem hiding this comment.
With set it is straight-forward to check if element exists.
lib/fizzy/parser.cpp
Outdated
| default: | ||
| assert(false); | ||
| } | ||
| if (const auto [_, inserted] = export_names.emplace(export_.name); !inserted) |
There was a problem hiding this comment.
I so much dislike this syntax of C++17 🤷
There was a problem hiding this comment.
if (!std::get<1>(export_names.emplace(export_.name)) isn't nicer.
But this entire thing is in a for loop, so why not just keep these as two statements (like the rest of codebase)?
There was a problem hiding this comment.
export_names.emplace(export_.name).second will work.
| EXPECT_EQ(module.exportsec[3].name, "xyz"); | ||
| EXPECT_EQ(module.exportsec[3].kind, ExternalKind::Global); | ||
| EXPECT_EQ(module.exportsec[3].index, 0x45); | ||
| EXPECT_EQ(module.exportsec[3].index, 0); |
There was a problem hiding this comment.
Too bad the old test was invalid, but nicer to see what we're testing.
test/unittests/validation_test.cpp
Outdated
| EXPECT_THROW_MESSAGE(parse(wasm), validation_error, "call_indirect without defined table"); | ||
| } | ||
|
|
||
| TEST(validation, export_invald_index) |
| (export "m" (memory 1)) | ||
| */ | ||
| const auto wasm_mem = from_hex("0061736d010000000503010001070501016d0201"); | ||
| EXPECT_THROW_MESSAGE(parse(wasm_mem), validation_error, "invalid index of an exported memory"); |
There was a problem hiding this comment.
Should we have different message whether memory/table section is present or not?
There was a problem hiding this comment.
I think this is fine - 0 is invalid index in some context.
axic
left a comment
axic
left a comment
There was a problem hiding this comment.
The change looks good in general, please fix the typo before merging. I'm not too strong on the if statement. Whether to have different message based on table/memory section being present, that is probably a useful question.
|
Cases with exporting table/memory 0 when none is defined are missing from the spectests, added to the "to upstream" list. |
3 participants
No description provided.