Closed GorogPeter closed 10 months ago
In this patch, you replaced some RELEASE_ASSERT
statements by abort
along with printing error messages.
But IMO RELEASE_ASSERT
is used in place where we assume that the condition of RELEASE_ASSERT
should be met.
Are there any test cases that violate these RELEASE_ASSERT
?
https://github.com/Samsung/walrus/blob/interp/src/Walrus.h#L266
RELEASE_ASSERT is basically an abort()
, so we thought printing the error and aborting mimics its operation. How shall we change it? I am not sure I understand the question about the tests, the code called abort before, and it is still doing it.
I'm wondering that additionally printing error messages are really necessary for each RELEASE_ASSERT
.
We have used RELEASE_ASSERT
or just ASSERT
so far to check the certain conditions that need to be true, not like exceptions.
RELEASE_ASSERT
is quite simple and clear but this patch adds messages with abort
which makes the code more complex.
It is a developer friendly patch, since it is hard to tell (for me) the actual error from the error message provided by shell. So this is a QoL change not a functionality change. It only affects the shell, not the engine.
We could use a macro to make the code simpler if that would help.
Since this patch is updated now, could we land it?
May I alter something or is it good?
Sorry for late 😢
fix 2 typo (replace uint16_t with Walrus::ByteCodeStackOffset).
fix another typo. Because of it 3 tests failed (binary.wast, binary-leb128.wast, elem.wast), but it didn't throw assert failed and the ExecuteWASM() returned with an error that wasn't caught.
For example in elem.wast:174-177 where it tries to write nothing to a 0 sized table: