Improve exception handling in memory.grow #737

gumb0 · 2021-02-16T19:35:33Z

No description provided.

codecov · 2021-02-16T20:04:30Z

Codecov Report

Merging #737 (3864569) into master (25205ab) will decrease coverage by 0.09%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master     #737      +/-   ##
==========================================
- Coverage   99.35%   99.26%   -0.10%     
==========================================
  Files          73       73              
  Lines       11388    11398      +10     
==========================================
- Hits        11315    11314       -1     
- Misses         73       84      +11

Flag	Coverage Δ
rust	`99.85% <ø> (ø)`
spectests	`90.68% <50.00%> (-0.41%)`	⬇️
unittests	`99.22% <50.00%> (-0.11%)`	⬇️

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

Impacted Files	Coverage Δ
lib/fizzy/execute.cpp	`98.70% <50.00%> (-1.30%)`	⬇️

gumb0 · 2021-02-16T20:07:58Z

lib/fizzy/execute.cpp

+            {
+                ret = static_cast<uint32_t>(-1);
+            }
+            catch (const std::length_error&)


Doesn't look possible to cover it, for me std::string::max_size() returns 2^62-1 bytes (2^46 pages), we can't allocate more than that with wasm.

I don't think this is worth having considering max_size() is not very useful.
It may be just better to add a unit test that checks if wasm memory max size is not smaller than std::string::max_size().
Alternatively, you can catch std::exception here.

On 32-bit build the value is 2^31-1.

chfast · 2021-02-17T11:01:37Z

lib/fizzy/execute.cpp

@@ -840,7 +840,7 @@ ExecutionResult execute(Instance& instance, FuncIdx func_idx, const Value* args,
                    throw std::bad_alloc();
                memory->resize(new_pages * PageSize);
            }
-            catch (std::bad_alloc const&)
+            catch (const std::exception&)


Add a comment that std::bad_alloc and std::legth_error are expected.

The ... catches everything so it guarantees noexcept outside. The std::exception does not, so there will be another compiler generated check ending in std::terminate or something.

So my final call is to use ... and also add an assert that the exception is one of the two expected types.

Although, checking the exception type may be tricky. Probably a helper is needed as presented in https://en.cppreference.com/w/cpp/error/rethrow_exception

Well then in the helper catch std::length_error will still be not covered, it's simpler then to have here catch (bad_alloc) + catch (length_error) + catch (...) { assert(false); } .

Well then in the helper catch std::length_error will still be not covered, it's simpler then to have here catch (bad_alloc) + catch (length_error) + catch (...) { assert(false); } .

But yeah execute will be smaller if extra catches go into helper, and helper will should be omitted in release build...

axic · 2021-02-22T20:26:22Z

lib/fizzy/execute.cpp

@@ -499,6 +499,25 @@ inline bool invoke_function(const FuncType& func_type, uint32_t func_idx, Instan
    return true;
 }

+[[maybe_unused]] bool is_resize_exception(std::exception_ptr exception)


Nice! Perhaps you want to add a comment here explaining our use case.

Added a comment.

axic · 2021-02-22T20:26:43Z

lib/fizzy/execute.cpp

@@ -840,8 +859,9 @@ ExecutionResult execute(Instance& instance, FuncIdx func_idx, const Value* args,
                    throw std::bad_alloc();


Should this use std::bad_alloc or std::length_error ?

The bad_alloc is better but not perfect fit out of these two.

Actually, here I'd see another refactoring: do not use throw.

uint32_t ret = (new_pages <= instance.memory_pages_limit) ? static_cast<uint32_t>(cur_pages) : static_cast<uint32_t>(-1); try { memory->resize(new_pages * PageSize); } catch (...) { ... }

It shouldn't go to resize though, if it's over the limit.
I can make a version with if, but it looks somewhat uglier.

The bad_alloc is better but not perfect fit out of these two.

Why it it better and what's the "fit" here? I'd say requirement for memory.grow to fail in case requested size is out of bounds is similar to requirement for basic_string to throw length_error in case count > max_size().
bad_alloc represents failure of some lower level allocation mechanism.

But I think in practice it doesn't matter and I'm fine with either.

chfast

Not very beautiful, but I think this is fine.

It is rather visible that some Memory abstraction type would be nice. This can be done together with "guarded pages" later.

There is also an issue on 32-bit architectures where string/bytes can only allocate up to 2GB of memory.

axic · 2021-02-23T17:40:13Z

lib/fizzy/execute.cpp

-            }
-            catch (std::bad_alloc const&)
-            {
+            uint32_t ret;


Hmm, wondering if the following would be more readable:

uint32_t ret = static_cast<uint32_t>(-1); if (new_pages <= instance.memory_pages_limit) { try { memory->resize(new_pages * PageSize); ret = static_cast<uint32_t>(cur_pages); } catch (...) { assert(is_resize_exception(std::current_exception())); } }

It looks fine, I don't mind.

The best is to wrap this into uint32_t Memory::grow(something);. Then you eliminate ret housekeeping.

I can move it into a simple helper for now.

axic · 2021-02-23T18:20:05Z

lib/fizzy/execute.cpp

-                ret = static_cast<uint32_t>(-1);
-            }
-            stack.push(ret);
+            stack.push(grow_memory(*memory, delta, instance.memory_pages_limit));


At this point could just use the stack pointer and no pop/push is needed 🙊

axic · 2021-02-23T18:29:34Z

lib/fizzy/execute.cpp

+    }
+}
+
+inline uint32_t grow_memory(


If we are going with this helper, please document it. What is important to document that it returns page count prior to expansion.

axic

I wonder how this affects benchmarks as it could mean quite a different binary layout 🙊

gumb0 commented Feb 16, 2021

View reviewed changes

gumb0 requested review from chfast and axic February 16, 2021 20:08

gumb0 force-pushed the memory-grow-exceptions branch from d5e66c6 to 3951f10 Compare February 17, 2021 10:49

chfast approved these changes Feb 17, 2021

View reviewed changes

chfast reviewed Feb 17, 2021

View reviewed changes

gumb0 force-pushed the memory-grow-exceptions branch from 3951f10 to 71e4ac2 Compare February 17, 2021 11:06

chfast approved these changes Feb 17, 2021

View reviewed changes

gumb0 force-pushed the memory-grow-exceptions branch from 71e4ac2 to ce2bca6 Compare February 22, 2021 20:21

axic reviewed Feb 22, 2021

View reviewed changes

gumb0 force-pushed the memory-grow-exceptions branch from ce2bca6 to 7d5cd5a Compare February 23, 2021 10:50

chfast approved these changes Feb 23, 2021

View reviewed changes

axic reviewed Feb 23, 2021

View reviewed changes

gumb0 force-pushed the memory-grow-exceptions branch from 6af194f to 9d92654 Compare February 23, 2021 18:15

axic reviewed Feb 23, 2021

View reviewed changes

gumb0 force-pushed the memory-grow-exceptions branch from 9d92654 to c43e129 Compare February 23, 2021 18:27

axic reviewed Feb 23, 2021

View reviewed changes

Improve exception handling in memory.grow instruction

3864569

gumb0 force-pushed the memory-grow-exceptions branch from c43e129 to 3864569 Compare February 23, 2021 18:37

axic approved these changes Feb 23, 2021

View reviewed changes

gumb0 requested a review from chfast February 23, 2021 18:39

chfast approved these changes Feb 23, 2021

View reviewed changes

gumb0 merged commit 47401da into master Feb 23, 2021

gumb0 deleted the memory-grow-exceptions branch February 23, 2021 19:35

gumb0 mentioned this pull request Feb 24, 2021

Update changelog #734

Merged

axic mentioned this pull request Mar 1, 2021

refactor: ExecuteFunction without std::function #716

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve exception handling in memory.grow #737

Improve exception handling in memory.grow #737

gumb0 commented Feb 16, 2021

codecov bot commented Feb 16, 2021 •

edited

Loading

gumb0 Feb 16, 2021 •

edited

Loading

chfast Feb 17, 2021

chfast Feb 17, 2021

chfast Feb 17, 2021

chfast Feb 22, 2021

gumb0 Feb 22, 2021

gumb0 Feb 22, 2021

axic Feb 22, 2021

gumb0 Feb 23, 2021

axic Feb 22, 2021

chfast Feb 23, 2021

chfast Feb 23, 2021

gumb0 Feb 23, 2021

gumb0 Feb 23, 2021

chfast left a comment

axic Feb 23, 2021 •

edited

Loading

chfast Feb 23, 2021

gumb0 Feb 23, 2021

axic Feb 23, 2021

axic Feb 23, 2021

axic left a comment

		@@ -840,8 +859,9 @@ ExecutionResult execute(Instance& instance, FuncIdx func_idx, const Value* args,
		throw std::bad_alloc();

Improve exception handling in memory.grow #737

Improve exception handling in memory.grow #737

Conversation

gumb0 commented Feb 16, 2021

codecov bot commented Feb 16, 2021 • edited Loading

Codecov Report

gumb0 Feb 16, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast left a comment

Choose a reason for hiding this comment

axic Feb 23, 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

axic left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 16, 2021 •

edited

Loading

gumb0 Feb 16, 2021 •

edited

Loading

axic Feb 23, 2021 •

edited

Loading