api: find_exported_* improvements #776

gumb0 · 2021-03-31T12:11:35Z

No description provided.

codecov · 2021-03-31T12:37:56Z

Codecov Report

Merging #776 (4ccc254) into master (faddd97) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #776   +/-   ##
=======================================
  Coverage   99.23%   99.23%           
=======================================
  Files          77       77           
  Lines       11886    11887    +1     
=======================================
+ Hits        11795    11796    +1     
  Misses         91       91

Flag	Coverage Δ
rust	`99.86% <ø> (ø)`
spectests	`90.54% <100.00%> (+<0.01%)`	⬆️
unittests	`99.19% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
lib/fizzy/capi.cpp	`97.83% <100.00%> (ø)`
lib/fizzy/instantiate.cpp	`100.00% <100.00%> (ø)`
lib/fizzy/instantiate.hpp	`100.00% <100.00%> (ø)`
test/spectests/spectests.cpp	`98.42% <100.00%> (ø)`
test/unittests/api_test.cpp	`100.00% <100.00%> (ø)`
test/unittests/end_to_end_test.cpp	`100.00% <100.00%> (ø)`
test/unittests/execute_call_test.cpp	`100.00% <100.00%> (ø)`
test/utils/fizzy_engine.cpp	`100.00% <100.00%> (ø)`
tools/wasi/wasi.cpp	`59.57% <100.00%> (ø)`

axic · 2021-03-31T16:42:25Z

lib/fizzy/instantiate.cpp

@@ -305,7 +305,8 @@ ExternalGlobal find_imported_global(const std::string& module, const std::string
    return {it->value, module_global_type};
 }

-std::optional<uint32_t> find_export(const Module& module, ExternalKind kind, std::string_view name)
+std::optional<uint32_t> find_export(


Are we sure that none of the constructors used would throw through std::make_optional? Can clang-tidy verify that at all?

Perhaps we could add a test marked noexcept constructing all those, just to have an early indication if we brake something that clang-tidy can't detect.

I've added noexcept to some constructors more, and I'm pretty sure, all External* objects use either explicitly noexcept or default move-constructors, which are noexcept implicitly.

Perhaps we could add a test marked noexcept constructing all those, just to have an early indication if we brake something that clang-tidy can't detect.

How would we detect it in test, if clang-tidy doesn't detect it? ~~Well setting terminate_handler might work...~~ not really, it would work only if something is really thrown.

clang-tidy may detect it if the constructors are used directly in a test marked noexcept, while it may not detect it via make_optional.

I guess it makes sense to use manual loop instead of find_if then.

Also apparently std::find_if can throw: https://en.cppreference.com/w/cpp/algorithm/find

Actually, it's only overloads with ExecutionPolicy can throw.

I think regular find_if is not noexcept, because the predicate function is allowed to throw.

I think it's safe to leave noexcept with find_if here, but I made labmda noexcept, too.

Also apparently std::find_if can throw: https://en.cppreference.com/w/cpp/algorithm/find

Actually, it's only overloads with ExecutionPolicy can throw.

I am not sure, that description is kind of confusing. Makes me think it can throw bad_alloc in any case.

First of all noexcept is not like const that all internal function calls must be noexcept. Usage of find_if looks fine to me.

Makes me think it can throw bad_alloc in any case.

Only overloads with ExecutionPolicy which we don't use.

clang-tidy only reports obvious mistakes (as direct usage of throw inside noexcept). We can dig deeper if you know the check name that fails.

lib/fizzy/instantiate.cpp

axic · 2021-03-31T18:33:00Z

lib/fizzy/instantiate.cpp

@@ -504,7 +507,8 @@ std::optional<ExternalFunction> find_exported_function(Instance& instance, std::
        ExecuteFunction(instance, idx), instance.module->get_function_type(idx)};


Here we use *opt_index. It is weird that operator* is not marked noexcept, but docs say "throws nothing". Compared to that ->value() explicitly checks and throws:

https://en.cppreference.com/w/cpp/utility/optional/operator*

https://en.cppreference.com/w/cpp/utility/optional/value

I found some explanation, but it's not clear to me https://stackoverflow.com/a/63913833

C++ is weird.

axic · 2021-03-31T18:36:17Z

lib/fizzy/instantiate.cpp

@@ -504,7 +507,8 @@ std::optional<ExternalFunction> find_exported_function(Instance& instance, std::
        ExecuteFunction(instance, idx), instance.module->get_function_type(idx)};
 }

-std::optional<ExternalGlobal> find_exported_global(Instance& instance, std::string_view name)
+std::optional<ExternalGlobal> find_exported_global(


This function also uses operator*.

chfast · 2021-04-08T12:51:03Z

lib/fizzy/instantiate.cpp

@@ -305,7 +305,8 @@ ExternalGlobal find_imported_global(const std::string& module, const std::string
    return {it->value, module_global_type};
 }

-std::optional<uint32_t> find_export(const Module& module, ExternalKind kind, std::string_view name)
+std::optional<uint32_t> find_export(


First of all noexcept is not like const that all internal function calls must be noexcept. Usage of find_if looks fine to me.

Makes me think it can throw bad_alloc in any case.

Only overloads with ExecutionPolicy which we don't use.

clang-tidy only reports obvious mistakes (as direct usage of throw inside noexcept). We can dig deeper if you know the check name that fails.

gumb0 · 2021-04-08T14:47:21Z

clang-tidy only reports obvious mistakes (as direct usage of throw inside noexcept). We can dig deeper if you know the check name that fails.

Weird that it does detect a call to bytes::resize() in grow_memory (bugprone-exception-escape check), but doesn't detect make_unique there https://github.com/wasmx/fizzy/pull/775/files#r603979857
(But it's actually a warning, not clang-tidy check)

gumb0 force-pushed the find-exported-improvements branch from 79890f2 to 62e3de7 Compare March 31, 2021 12:59

Rename find_exported_function returning an index

2535461

gumb0 force-pushed the find-exported-improvements branch from 62e3de7 to bd70d73 Compare March 31, 2021 14:49

gumb0 marked this pull request as ready for review March 31, 2021 15:18

gumb0 requested review from chfast and axic March 31, 2021 16:09

axic reviewed Mar 31, 2021

View reviewed changes

lib/fizzy/instantiate.cpp Show resolved Hide resolved

chfast approved these changes Mar 31, 2021

View reviewed changes

axic reviewed Mar 31, 2021

View reviewed changes

axic mentioned this pull request Mar 31, 2021

capi: mark all external functions noexcept #775

Merged

gumb0 added 2 commits April 7, 2021 12:21

Make find_exported_* functions noexcept

a08bd5b

Add missing noexcept to ExternalFunction/ExecuteFunction constructors

4ccc254

gumb0 force-pushed the find-exported-improvements branch from 6798328 to 4ccc254 Compare April 7, 2021 10:22

axic approved these changes Apr 7, 2021

View reviewed changes

chfast approved these changes Apr 8, 2021

View reviewed changes

gumb0 merged commit dbdef17 into master Apr 8, 2021

gumb0 deleted the find-exported-improvements branch April 8, 2021 13:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: find_exported_* improvements #776

api: find_exported_* improvements #776

gumb0 commented Mar 31, 2021

codecov bot commented Mar 31, 2021 •

edited

Loading

axic Mar 31, 2021

axic Mar 31, 2021

gumb0 Mar 31, 2021

gumb0 Mar 31, 2021 •

edited

Loading

axic Mar 31, 2021

gumb0 Apr 6, 2021

gumb0 Apr 7, 2021

gumb0 Apr 7, 2021

axic Apr 7, 2021

chfast Apr 8, 2021

axic Mar 31, 2021

gumb0 Apr 6, 2021

axic Apr 7, 2021

axic Mar 31, 2021

chfast Apr 8, 2021

gumb0 commented Apr 8, 2021 •

edited

Loading

		@@ -504,7 +507,8 @@ std::optional<ExternalFunction> find_exported_function(Instance& instance, std::
		ExecuteFunction(instance, idx), instance.module->get_function_type(idx)};

api: find_exported_* improvements #776

api: find_exported_* improvements #776

Conversation

gumb0 commented Mar 31, 2021

codecov bot commented Mar 31, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 Mar 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 commented Apr 8, 2021 • edited Loading

codecov bot commented Mar 31, 2021 •

edited

Loading

gumb0 Mar 31, 2021 •

edited

Loading

gumb0 commented Apr 8, 2021 •

edited

Loading