[riot-notifications] [RIOT-OS/RIOT] WIP: pkg/wamr: add WAMR to provide WASM support in RIOT (#15329)

Marian Buschsieweke notifications at github.com
Wed Sep 22 09:52:32 CEST 2021


@maribu commented on this pull request.

A bunch of style nits

> +#include <stdio.h>
+#include <stdint.h>
+
+#include <string.h>
+
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wstrict-prototypes"
+#include "wasm_export.h"
+#pragma GCC diagnostic pop
+
+#include <thread.h>
+
+/*provide some test program*/
+#include "blob/main.wasm.h"
+
+

```suggestion
```

> +static int app_argc;
+static char **app_argv;
+
+static void*
+app_instance_main(wasm_module_inst_t module_inst)
+{
+    const char *exception;
+
+    wasm_application_execute_main(module_inst, app_argc, app_argv);
+    if ((exception = wasm_runtime_get_exception(module_inst))){
+        puts(exception);
+    }
+    return NULL;
+}
+
+

```suggestion
```

> +static void*
+app_instance_main(wasm_module_inst_t module_inst)

```suggestion
static void *app_instance_main(wasm_module_inst_t module_inst)
```

> +void *
+iwasm_main(void *arg1)

```suggestion
void *iwasm_main(void *arg1)
```

> +#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wstrict-prototypes"
+#include "wasm_export.h"
+#pragma GCC diagnostic pop

This warning is generated if the forward declaration of a function looks like this `void foo()` instead of this `void foo(void)`.

Can't we just fix `wasm_export.h`, since this is a relatively trivial fix?

> +    return NULL;
+}
+
+
+void *
+iwasm_t(void *arg1)
+{
+    wasm_module_t wasm_module = (wasm_module_t) arg1;
+    wasm_module_inst_t wasm_module_inst = NULL;
+    char error_buf[128];
+
+    /* instantiate the module */
+    if (!(wasm_module_inst = wasm_runtime_instantiate(wasm_module, 8 * 1024,
+        8 * 1024, error_buf, sizeof(error_buf)))) {
+        puts(error_buf);
+    }else{

```suggestion
    }
    else {
```

> +void *
+iwasm_t(void *arg1)

```suggestion
void *iwasm_t(void *arg1)
```

> +    if (!(wasm_module_inst = wasm_runtime_instantiate(wasm_module, 8 * 1024,
+        8 * 1024, error_buf, sizeof(error_buf)))) {

```suggestion
    wasm_module_inst = wasm_runtime_instantiate(wasm_module, 8 * 1024, 8 * 1024,
                                                error_buf, sizeof(error_buf));
    if (!wasm_module_inst) {
```

Style is subjective, but still IMO this is easier to read.

> +    /* instantiate the module */
+    if (!(wasm_module_inst = wasm_runtime_instantiate(wasm_module, 8 * 1024,
+        8 * 1024, error_buf, sizeof(error_buf)))) {
+        puts(error_buf);
+    }else{
+        app_instance_main(wasm_module_inst);
+        /* destroy the module instance */
+        wasm_runtime_deinstantiate(wasm_module_inst);
+    }
+    return NULL;
+}
+
+void *
+iwasm_main(void *arg1)
+{
+    (void) arg1; /*unused*/

```suggestion
    (void) arg1;/*unused*/
```

> +app_instance_main(wasm_module_inst_t module_inst)
+{
+    const char *exception;
+
+    wasm_application_execute_main(module_inst, app_argc, app_argv);
+    if ((exception = wasm_runtime_get_exception(module_inst))){
+        puts(exception);
+    }
+    return NULL;
+}
+
+
+void *
+iwasm_t(void *arg1)
+{
+    wasm_module_t wasm_module = (wasm_module_t) arg1;

```suggestion
    wasm_module_t wasm_module = (wasm_module_t)arg1;
```

> +#include "blob/main.wasm.h"
+
+
+#define DEFAULT_THREAD_STACKSIZE (6 * 1024)
+#define DEFAULT_THREAD_PRIORITY 50
+
+static int app_argc;
+static char **app_argv;
+
+static void*
+app_instance_main(wasm_module_inst_t module_inst)
+{
+    const char *exception;
+
+    wasm_application_execute_main(module_inst, app_argc, app_argv);
+    if ((exception = wasm_runtime_get_exception(module_inst))){

```suggestion
    if ((exception = wasm_runtime_get_exception(module_inst))) {
```

> @@ -0,0 +1,157 @@
+/*
+ * Copyright (C) 2019 Intel Corporation.  All rights reserved.
+ * Copyright (C) 2020 TU Bergakademie Freiberg Karl Fessel
+ * SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+ */
+
+//

```suggestion
```

> +    }
+    return NULL;
+}
+
+void *
+iwasm_main(void *arg1)
+{
+    (void) arg1; /*unused*/
+    uint8_t *wasm_file_buf = NULL;
+    unsigned wasm_file_buf_size = 0;
+    wasm_module_t wasm_module = NULL;
+    char error_buf[128];
+
+    RuntimeInitArgs init_args;
+
+//chose allocator non defaults to system allocator

```suggestion
/* chose allocator non defaults to system allocator */
```

> +#pragma GCC diagnostic ignored "-Wpedantic"
+    init_args.mem_alloc_option.allocator.malloc_func = malloc;
+    init_args.mem_alloc_option.allocator.realloc_func = realloc;
+    init_args.mem_alloc_option.allocator.free_func = free;
+#pragma GCC diagnostic pop

```suggestion
    init_args.mem_alloc_option.allocator.malloc_func = (void *)malloc;
    init_args.mem_alloc_option.allocator.realloc_func = (void *)realloc;
    init_args.mem_alloc_option.allocator.free_func = (void *)free;
```

> +    static char global_heap_buf[256 * 1024] = { 0 };//(256 kB)
+
+    init_args.mem_alloc_type = Alloc_With_Pool;
+    init_args.mem_alloc_option.pool.heap_buf = global_heap_buf;
+    init_args.mem_alloc_option.pool.heap_size = sizeof(global_heap_buf);
+#elif defined(FUNC_ALLOC)
+    init_args.mem_alloc_type = Alloc_With_Allocator;
+/*deactivate Wpedantic for some lines*/
+#pragma GCC diagnostic ignored "-Wpedantic"
+    init_args.mem_alloc_option.allocator.malloc_func = malloc;
+    init_args.mem_alloc_option.allocator.realloc_func = realloc;
+    init_args.mem_alloc_option.allocator.free_func = free;
+#pragma GCC diagnostic pop
+#else
+    init_args.mem_alloc_type = Alloc_With_System_Allocator;
+#endif

IMO you could just use the variant using `malloc()` and friends here. In the end, the RIOT examples for packages are not tutorial in how to use the external packages, but in how to use the package with RIOT. Setting up an alternative allocation scheme is something that is (hopefully) covered by the upstream documentation of the package and requires no RIOT specific special handling.

For that reason, I think it is better to keep the example as simple and minimal as possible, except when showing important RIOT specifics.

> +    }else{
+        iwasm_t(wasm_module);

```suggestion
    }
    else {
        iwasm_t(wasm_module);
```

> +bool
+iwasm_init(void)

```suggestion
bool iwasm_init(void)
```

> +                                          error_buf, sizeof(error_buf)))) {
+        puts(error_buf);
+
+    }else{
+        iwasm_t(wasm_module);
+        wasm_runtime_unload(wasm_module);
+    }
+
+    wasm_runtime_destroy();
+    return NULL;
+}
+
+bool
+iwasm_init(void)
+{
+    struct{

```suggestion
    struct {
```

> +        char *  stack;
+        int  stacksize;
+        uint8_t  priority;
+        int  flags;
+        thread_task_func_t task_func;
+        void *  arg;
+        const char * name;

```suggestion
        char *stack;
        int stacksize;
        uint8_t priority;
        int flags;
        thread_task_func_t task_func;
        void *arg;
        const char *name;
```

> +        int  stacksize;
+        uint8_t  priority;
+        int  flags;
+        thread_task_func_t task_func;
+        void *  arg;
+        const char * name;
+    } b = {
+        .stacksize = DEFAULT_THREAD_STACKSIZE,
+        .priority = 8,
+        .flags = 0,
+        .task_func = iwasm_main,
+        .arg = NULL,
+        .name = "simple_wamr"
+    };
+
+    b.stack=malloc(b.stacksize);

```suggestion
    b.stack = malloc(b.stacksize);
```

> +        uint8_t  priority;
+        int  flags;
+        thread_task_func_t task_func;
+        void *  arg;
+        const char * name;
+    } b = {
+        .stacksize = DEFAULT_THREAD_STACKSIZE,
+        .priority = 8,
+        .flags = 0,
+        .task_func = iwasm_main,
+        .arg = NULL,
+        .name = "simple_wamr"
+    };
+
+    b.stack=malloc(b.stacksize);
+    kernel_pid_t tpid = thread_create (b.stack, b.stacksize, b.priority, b.flags, b.task_func, b.arg, b.name);

```suggestion
    kernel_pid_t tpid = thread_create(b.stack, b.stacksize, b.priority, b.flags,
                                      b.task_func, b.arg, b.name);
```

> +int
+main(void)

```suggestion
int main(void)
```

> +
+#define telltruth(X) ((X) ? "true" : "false")

Since this is used exactly once, why the effort of a macro?

> +        .task_func = iwasm_main,
+        .arg = NULL,
+        .name = "simple_wamr"
+    };
+
+    b.stack=malloc(b.stacksize);
+    kernel_pid_t tpid = thread_create (b.stack, b.stacksize, b.priority, b.flags, b.task_func, b.arg, b.name);
+
+    return tpid != 0 ? true : false;;
+}
+
+#define telltruth(X) ((X) ? "true" : "false")
+int
+main(void)
+{
+    printf("iwasm_initilised: %s\n",telltruth(iwasm_init()));

```suggestion
    printf("iwasm_initilised: %s\n", iwasm_init() ? "true" : "false");
```

> @@ -0,0 +1,5 @@
+USEMODULE += sema
+
+#WAMR supports "X86_32/64", "AARCH64", "ARM", "THUMB", "MIPS" and "XTENSA"
+#no 8/16 Bit TODO: might need to blacklist more
+FEATURES_BLACKLIST += arch_8bit arch_16bit

```suggestion
FEATURES_REQUIRED_ANY += arch_native|arch_arm|arch_mips32r2|arch_esp
```

IMO it is easier to maintain a list of supported architectures than a list of unsupported. Also, this would give a more helpful error message (as it will tell the user which of her board she could try instead, rather than just "meh, I don't like this one").

No support for RISC-V yet, even when not using JIT?

> +
+

```suggestion

```

> @@ -0,0 +1,61 @@
+PKG_NAME=wamr
+PKG_URL=https://github.com/bytecodealliance/wasm-micro-runtime.git
+PKG_VERSION=main

This has to be fixed to a given commit, otherwise this will be a nightmare to maintain.

The completely outdated `libcoap` package isn't because the version was fixed to a specific commit, but because nobody cared to maintain the package. If the package would have followed `master`, it would have caused breaking CI all the time. This in turn would have pissed people of to either nail down the package version to a specific commit (same result) or just drop the package.

The only way to prevent code from bit-rotting is to get people maintaining it.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/15329#pullrequestreview-760527826
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210922/d594b3ae/attachment-0001.htm>


More information about the notifications mailing list