Print error message #110

GorogPeter · 2023-06-26T10:21:15Z

print lexer initialisation error
print syntax error
print semantic error
print assert fail
print unequal call paramterer size in assert error
print not-defined function call in asser error

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:

(module
  (table 0 funcref)
  (elem (i32.const 0))
)

zherczeg

This improves the usage of shell a lot. Some comments though.

zherczeg · 2023-06-26T11:12:40Z

src/shell/Shell.cpp

+        fprintf(stderr, "Syntax error");
+        if (errors.size() > 1)
+            fprintf(stderr, "s");
+        fprintf(stderr, ":\n");


fprintf(stderr, "Syntax error(s)\n"); is enough

zherczeg · 2023-06-26T11:13:22Z

src/shell/Shell.cpp

+        fprintf(stderr, ":\n");
+        int i = 1;
+        for (auto e : errors)
+            fprintf(stderr, "%15d.) %s\n", i++, e.message.c_str());


It is rare to have many errors, and their index is not improtant, so I think just print the error messages.

zherczeg · 2023-06-26T11:14:21Z

src/shell/Shell.cpp

+            auto trapResult = executeWASM(store, filename, buf->data, functionTypes, &registeredInstanceMap);
+            if (trapResult.exception) {
+                std::string& errorMessage = trapResult.exception->message();
+                fprintf(stderr, "Semantic error: %s\n", errorMessage.c_str());


"Error: " is enough

zherczeg · 2023-06-26T11:15:32Z

src/shell/Shell.cpp

+                    printf(") expect value(");
+                    printConstVector(data->expectedResult);
+                    printf(") (line: %d) : FAILED\n", data->action->loc.line);
+                    abort();


I think printing i, result[i], and data->expectedResult[i] are the key thing here

At this point, printf is used to print messages but other case, fprintf is used instead.
For the code consistency, please use just one function for printing messages.
One more, we prefer RELEASE_ASSERT or ASSERT to abort function.
Please change abort function too.

Okay.
Thank you!

clover2123 · 2023-06-27T03:19:22Z

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?

clover2123 · 2023-06-27T03:28:53Z

src/runtime/Module.cpp

@@ -282,7 +282,7 @@ Instance* Module::instantiate(ExecutionState& state, const ExternVector& imports
                         &data);
            }

-            if (UNLIKELY(elem->tableIndex() >= numberOfTableTypes() || index >= instance->m_tables[elem->tableIndex()]->size() || index + elem->functionIndex().size() > instance->m_tables[elem->tableIndex()]->size())) {
+            if (UNLIKELY(elem->tableIndex() >= numberOfTableTypes() || index > instance->m_tables[elem->tableIndex()]->size() || index + elem->functionIndex().size() > instance->m_tables[elem->tableIndex()]->size())) {


It seems that index >= instance->m_tables[elem->tableIndex()]->size() can be totally removed

Maybe adding an (uint64_t) casting could be used to avoid overflow situations.

Btw, isn't the wabt validator should be able to catch these issues? We could check it.

Have you checked this?
It seems that wabt validator cannot check this case in advance.. but what do you think about it?

zherczeg · 2023-06-27T04:58:48Z

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.

clover2123 · 2023-06-27T05:56:08Z

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.

zherczeg · 2023-06-27T06:00:30Z

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.

zherczeg · 2023-06-27T06:01:34Z

We could use a macro to make the code simpler if that would help.

clover2123 · 2023-06-28T07:18:44Z

src/shell/Shell.cpp

            RELEASE_ASSERT(data->fn->functionType()->result().size() == data->expectedResult.size());
            // compare result
            for (size_t i = 0; i < result.size(); i++) {
-                RELEASE_ASSERT(equals(result[i], data->expectedResult[i]));
+                if (!equals(result[i], data->expectedResult[i])) {
+                    printf("Assert FAILED (line: %d) ", data->action->loc.line);


Are you going to replace all RELEASE_ASSERT statements by this approach?
If so, please consider all RELEASE_ASSERT cases in this patch

Good point!

zherczeg

Close to done

zherczeg · 2023-08-02T02:56:30Z

src/shell/Shell.cpp

-    if (!wabt::Succeeded(result)) {
-        RELEASE_ASSERT_NOT_REACHED();
-        return;
+    if (wabt::Succeeded(result) == false) {


!wabt::Succeeded(result) is ok

zherczeg · 2023-08-02T02:56:59Z

src/shell/Shell.cpp

            if (assertReturn->action->type() == wabt::ActionType::Invoke) {
                auto action = static_cast<wabt::InvokeAction*>(assertReturn->action.get());
                auto fn = fetchInstance(action->module_var, instanceMap, registeredInstanceMap)->resolveExportFunction(action->name);
                executeInvokeAction(action, fn, assertReturn->expected, nullptr);
            } else if (assertReturn->action->type() == wabt::ActionType::Get) {
                auto action = static_cast<wabt::GetAction*>(assertReturn->action.get());
                auto v = fetchInstance(action->module_var, instanceMap, registeredInstanceMap)->resolveExportGlobal(action->name)->value();
-                RELEASE_ASSERT(equals(v, assertReturn->expected[0]))
+                if (equals(v, assertReturn->expected[0]) == false) {


zherczeg · 2023-08-02T02:59:19Z

src/shell/Shell.cpp

            std::vector<uint8_t> buf;
            if (tsm) {
                buf = readModuleData(&tsm->module)->data;
            } else {
                buf = dsm->data;
            }
            auto trapResult = executeWASM(store, filename, buf, functionTypes);
-            RELEASE_ASSERT(trapResult.exception);
+            if (trapResult.exception == nullptr) {
+                printf("Execute WASM returned nullptr (in wabt::CommandType::AssertInvalid case)\n");


Expected exception: assertModuleInvalid->text.data()

zherczeg · 2023-08-02T02:59:42Z

src/shell/Shell.cpp

            std::vector<uint8_t> buf;
            if (tsm) {
                buf = readModuleData(&tsm->module)->data;
            } else {
                buf = dsm->data;
            }
            auto trapResult = executeWASM(store, filename, buf, functionTypes);
-            RELEASE_ASSERT(trapResult.exception);
+            if (trapResult.exception == nullptr) {
+                printf("Execute WASM returned nullptr (in wabt::CommandType::AssertUnlinkable case)\n");


Expected exception

zherczeg · 2023-08-09T06:43:16Z

src/shell/Shell.cpp

@@ -427,7 +438,9 @@ static Walrus::Value toWalrusValue(wabt::Const& c)
        return Walrus::Value(Walrus::Value::ExternRef, c.ref_bits() + 1, Walrus::Value::Force);
    }
    default:
-        RELEASE_ASSERT_NOT_REACHED();
+        printf("Error: unknown value type during converting wabt::Const to wabt::Value\n");
+        ASSERT_NOT_REACHED();


keep RELEASE_ASSERT_NOT_REACHED

zherczeg · 2023-08-09T06:44:12Z

src/shell/Shell.cpp

+        }
+        default: {
+            printf("Error: unkown wabt::Const type\n");
+            ASSERT_NOT_REACHED();


keep RELEASE_ASSERT_NOT_REACHED

zherczeg · 2023-08-09T06:45:00Z

src/shell/Shell.cpp

-    RELEASE_ASSERT(fn->functionType()->param().size() == action->args.size());
+    if (fn->functionType()->param().size() != action->args.size()) {
+        printf("Error: expected %zu parameter(s) but got %zu.\n", fn->functionType()->param().size(), action->args.size());
+        ASSERT_NOT_REACHED();


this assert is weaker than RELEASE_ASSERT_NOT_REACHED, keep the latter everywhere

zherczeg

LGTM

clover2123

LGTM

- fix 2 typos (uint16_t to Walrus::ByteCodeStackOffset) - fix 1 typo (because of it 3 tests failed, altough it was unnoticable hence the error wasn't caught) - refactor nested if...else to switch...case - print error message before exit - make string() operator for Value class

zherczeg · 2023-09-11T10:36:41Z

Since this patch is updated now, could we land it?

GorogPeter · 2023-09-20T11:24:43Z

May I alter something or is it good?

clover2123 · 2023-09-21T07:05:02Z

Sorry for late 😢

GorogPeter requested review from ksh8281 and clover2123 as code owners June 26, 2023 10:21

zherczeg requested changes Jun 26, 2023

View reviewed changes

clover2123 reviewed Jun 27, 2023

View reviewed changes

GorogPeter force-pushed the error_msg branch from 82bd2ec to 93d627e Compare June 27, 2023 10:52

clover2123 reviewed Jun 28, 2023

View reviewed changes

GorogPeter force-pushed the error_msg branch 3 times, most recently from d537e06 to 41207f9 Compare July 26, 2023 11:29

zherczeg requested changes Aug 2, 2023

View reviewed changes

GorogPeter force-pushed the error_msg branch from 41207f9 to 4480f6a Compare August 6, 2023 09:59

zherczeg requested changes Aug 9, 2023

View reviewed changes

GorogPeter force-pushed the error_msg branch from 4480f6a to 2a33809 Compare August 9, 2023 12:17

zherczeg approved these changes Aug 16, 2023

View reviewed changes

clover2123 approved these changes Aug 17, 2023

View reviewed changes

GorogPeter force-pushed the error_msg branch from 2a33809 to 099a77a Compare August 30, 2023 15:01

GorogPeter force-pushed the error_msg branch from 099a77a to 71ca82d Compare September 11, 2023 06:59

clover2123 merged commit 708e231 into Samsung:interp Sep 21, 2023
12 checks passed

GorogPeter deleted the error_msg branch November 2, 2023 14:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Print error message #110

Print error message #110

GorogPeter commented Jun 26, 2023

zherczeg left a comment

zherczeg Jun 26, 2023

GorogPeter Jun 26, 2023

zherczeg Jun 26, 2023

GorogPeter Jun 26, 2023

zherczeg Jun 26, 2023

GorogPeter Jun 26, 2023

zherczeg Jun 26, 2023

GorogPeter Jun 26, 2023

clover2123 Jun 28, 2023

GorogPeter Jun 28, 2023

clover2123 commented Jun 27, 2023

clover2123 Jun 27, 2023

zherczeg Jun 27, 2023

clover2123 Jun 28, 2023

zherczeg commented Jun 27, 2023

clover2123 commented Jun 27, 2023

zherczeg commented Jun 27, 2023

zherczeg commented Jun 27, 2023

clover2123 Jun 28, 2023

zherczeg Jun 28, 2023

zherczeg left a comment

zherczeg Aug 2, 2023

zherczeg Aug 2, 2023

zherczeg Aug 2, 2023

zherczeg Aug 2, 2023

zherczeg Aug 9, 2023

zherczeg Aug 9, 2023

zherczeg Aug 9, 2023

zherczeg left a comment

clover2123 left a comment

zherczeg commented Sep 11, 2023

GorogPeter commented Sep 20, 2023

clover2123 commented Sep 21, 2023

Print error message #110

Print error message #110

Conversation

GorogPeter commented Jun 26, 2023

zherczeg left a 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

clover2123 commented Jun 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zherczeg commented Jun 27, 2023

clover2123 commented Jun 27, 2023

zherczeg commented Jun 27, 2023

zherczeg commented Jun 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zherczeg left a 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

zherczeg left a comment

Choose a reason for hiding this comment

clover2123 left a comment

Choose a reason for hiding this comment

zherczeg commented Sep 11, 2023

GorogPeter commented Sep 20, 2023

clover2123 commented Sep 21, 2023