Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

per-host function override of native stack requirement #3325

Open
yamt opened this issue Apr 17, 2024 · 7 comments
Open

per-host function override of native stack requirement #3325

yamt opened this issue Apr 17, 2024 · 7 comments

Comments

@yamt
Copy link
Collaborator

yamt commented Apr 17, 2024

consider that you have a few host functions which require huge native stack to run.
right now, you need to bump WASM_STACK_GUARD_SIZE enough for the largest possible consumer
even when you precisely know which instances/threads would use such functions.
it would be nicer to provide a way to keep WASM_STACK_GUARD_SIZE small while allowing a few host functions to consume more stack so that you can use large native stack only for instances/threads which actually need it.

even within our tree, some native functions require more native stack than others.
eg.

/* UVWASI requires larger native stack */

eg. #2332

possible implementation approaches:

a. extend NativeSymbol with necessary info. (eg. size_t native_stack_size)

make aot_call_function/aot_call_indirect etc detect overflows using the value.

pros:

  • declarative

cons:

  • NativeSymbol is not always handy when calling native function.
    maybe we need to copy it to eg. AOTImportFunc.
  • a straightforward implementation would break the abi.
    (it should be ok for 2.0 though)
  • direct call (what aot_compile_op_call does when signature is available)
    needs something different. maybe the aot compiler can emit the check.
    however, it would effectively make the stack requirement of the native
    function a part of the AOT ABI. i'm not sure if it's acceptable.

a'. wasm_runtime_set_native_api_stacks

use similar detection logic as (a), but a different api to tell stack size.

wasm_runtime_set_native_api_stacks(const char *module_name, NativeSymbolStack *native_stacks, uint32 n_symbol_stack)

cons:

  • wamrc --native-lib needs extra care

b. add an extra overflow detection logic to each possible stack-hogging functions

like this:

if (!wasm_runtime_check_native_stack(exec_env, 1000000)) {
    return; /* fail */
}

pros:

  • easy to implement
  • flexible (eg. you can make the estimation depend on function parameters)
  • consistent with aot-compiled modules. (check in callee)

cons:

  • a lot of boring code changes if you have a lot of functions
    (eg. libc-uvwasi)
  • unless we sprinkle noinline attributes, a compiler can inline functions
    and possibly break the check.
@wenyongh
Copy link
Contributor

Hi, it seems both two methods are not so straightforward, how about we just introduce a new API to set the native stack size required for the host native APIs? Some what like:

struct NativeSymbolStack {
    const char *name;
    uint32 stack_size;
}
wasm_runtime_set_native_api_stacks(NativeSymbolStack *native_stacks, uint32 n_symbol_stack);

And the info will be propagated into a field in WASMFunctionImport and AOTFuncImport, and runtime does native stack overflow check according to the field's value before calling the native API, and by default the field value is WASM_STACK_GUARD_SIZE if it isn't set by the API. Also runtime can set the field value of some native APIs after registering some built-in native APIs.

@yamt
Copy link
Collaborator Author

yamt commented Apr 18, 2024

Hi, it seems both two methods are not so straightforward, how about we just introduce a new API to set the native stack size required for the host native APIs? Some what like:

struct NativeSymbolStack {
    const char *name;
    uint32 stack_size;
}
wasm_runtime_set_native_api_stacks(NativeSymbolStack *native_stacks, uint32 n_symbol_stack);

And the info will be propagated into a field in WASMFunctionImport and AOTFuncImport, and runtime does native stack overflow check according to the field's value before calling the native API, and by default the field value is WASM_STACK_GUARD_SIZE if it isn't set by the API. Also runtime can set the field value of some native APIs after registering some built-in native APIs.

at this point, i don't have strong opinion on the api. (how to give the n_symbol_stack value.)
your suggested api would have almost same overflow detection implementation with the first approach, right?
how do you think about the last item in cons?
this one:

   "direct call (what aot_compile_op_call does when signature is available)
   needs something different. maybe the aot compiler can emit the check.
   however, it would effectively make the stack requirement of the native
   function a part of the AOT ABI. i'm not sure if it's acceptable.")

@wenyongh
Copy link
Contributor

Yes, it is very similar to the first approach, but no need to make changes for current runtime native libraries implementation and developer's native library registration if there is no extra requirement from him.

For the last item, yes, had better let aot compiler emit the check directly, not sure whether it is good to pass the native stack size info to wamrc like registering the empty native apis for it with wamrc --native-lib=xxx.so? We can add another option like wamrc --native-stack-lib=xxx.so?

@wenyongh
Copy link
Contributor

BTW, the api had better use module name as the first argument, like wasm_runtime_register_natives: wasm_runtime_set_native_api_stacks(const char *module_name, NativeSymbolStack *native_stacks, uint32 n_symbol_stack)

@wenyongh
Copy link
Contributor

Yes, it is very similar to the first approach, but no need to make changes for current runtime native libraries implementation and developer's native library registration if there is no extra requirement from him.

For the last item, yes, had better let aot compiler emit the check directly, not sure whether it is good to pass the native stack size info to wamrc like registering the empty native apis for it with wamrc --native-lib=xxx.so? We can add another option like wamrc --native-stack-lib=xxx.so?

Or just reuse the option wamrc --native-lib=xxx.so,but require the so to provide extra api to get the native stack info

@yamt
Copy link
Collaborator Author

yamt commented Apr 18, 2024

Yes, it is very similar to the first approach, but no need to make changes for current runtime native libraries implementation and developer's native library registration if there is no extra requirement from him.

even with the first approach, users who don't care can leave it default.
(similarly to what we do for NativeSymbol attachment today)

For the last item, yes, had better let aot compiler emit the check directly, not sure whether it is good to pass the native stack size info to wamrc like registering the empty native apis for it with wamrc --native-lib=xxx.so?

with the first approach, wamrc --native-lib would work as of today.

We can add another option like wamrc --native-stack-lib=xxx.so?

i'm not sure how it looks like.
does xxx.so for --native-stack-lib use an api different from the current get_native_lib/init_native_lib/deinit_native_lib?

Or just reuse the option wamrc --native-lib=xxx.so,but require the so to provide extra api to get the native stack info

do you mean to export an extra symbol?

anyway, my original concern was not really about --native-lib.
let me explain.
by making the aot compiler emit the caller-side check for a native function,
we effectively embed the corresponding n_symbol_stack value into the aot-compiled module.
if we later modify the implementation of the native function and it ends up with changing the n_symbol_stack value,
we need to re-aot-compile all modules which potentially use the native function.
for that reason, right now, i'm inclined toward the approach (b).

@wenyongh
Copy link
Contributor

Yes, it is very similar to the first approach, but no need to make changes for current runtime native libraries implementation and developer's native library registration if there is no extra requirement from him.

even with the first approach, users who don't care can leave it default. (similarly to what we do for NativeSymbol attachment today)

For the last item, yes, had better let aot compiler emit the check directly, not sure whether it is good to pass the native stack size info to wamrc like registering the empty native apis for it with wamrc --native-lib=xxx.so?

with the first approach, wamrc --native-lib would work as of today.

We can add another option like wamrc --native-stack-lib=xxx.so?

i'm not sure how it looks like. does xxx.so for --native-stack-lib use an api different from the current get_native_lib/init_native_lib/deinit_native_lib?

Or just reuse the option wamrc --native-lib=xxx.so,but require the so to provide extra api to get the native stack info

do you mean to export an extra symbol?

anyway, my original concern was not really about --native-lib. let me explain. by making the aot compiler emit the caller-side check for a native function, we effectively embed the corresponding n_symbol_stack value into the aot-compiled module. if we later modify the implementation of the native function and it ends up with changing the n_symbol_stack value, we need to re-aot-compile all modules which potentially use the native function. for that reason, right now, i'm inclined toward the approach (b).

OK, b is also good as long as it is not very trivial for you to add checks in the native apis.

yamt added a commit to yamt/wasm-micro-runtime that referenced this issue Apr 19, 2024
This is a test code to examine native stack overflow detection logic.

The current output on my environment (macOS amd64):

```shell
====== Interpreter
 stack size   | fail?  | leak?  | exception
---------------------------------------------------------------------------
    0 - 14704 | failed | leaked | Exception: native stack overflow
14704 - 17904 | failed |     ok | Exception: native stack overflow
17904 - 24576 |     ok |     ok |

====== AOT
 stack size   | fail?  | leak?  | exception
---------------------------------------------------------------------------
    0 - 18176 | failed | leaked | Exception: native stack overflow
18176 - 24576 |     ok |     ok |

====== AOT WAMR_DISABLE_HW_BOUND_CHECK=1
 stack size   | fail?  | leak?  | exception
---------------------------------------------------------------------------
    0 -  1968 | failed |     ok | Exception: native stack overflow
 1968 - 24576 |     ok |     ok |
```

This is a preparation to work on relevant issues, including:

bytecodealliance#3325
bytecodealliance#3320
bytecodealliance#3314
bytecodealliance#3297
wenyongh pushed a commit that referenced this issue Apr 19, 2024
This is a test code to examine native stack overflow detection logic.

The current output on my environment (macOS amd64):

```shell
====== Interpreter
 stack size   | fail?  | leak?  | exception
---------------------------------------------------------------------------
    0 - 14704 | failed | leaked | Exception: native stack overflow
14704 - 17904 | failed |     ok | Exception: native stack overflow
17904 - 24576 |     ok |     ok |

====== AOT
 stack size   | fail?  | leak?  | exception
---------------------------------------------------------------------------
    0 - 18176 | failed | leaked | Exception: native stack overflow
18176 - 24576 |     ok |     ok |

====== AOT WAMR_DISABLE_HW_BOUND_CHECK=1
 stack size   | fail?  | leak?  | exception
---------------------------------------------------------------------------
    0 -  1968 | failed |     ok | Exception: native stack overflow
 1968 - 24576 |     ok |     ok |
```

This is a preparation to work on relevant issues, including:

#3325
#3320
#3314
#3297
yamt added a commit to yamt/wasm-micro-runtime that referenced this issue Apr 25, 2024
This allows each native function to require more native stack.

cf. bytecodealliance#3325
yamt added a commit to yamt/wasm-micro-runtime that referenced this issue Apr 25, 2024
This allows each native function to require more native stack.

cf. bytecodealliance#3325
yamt added a commit to yamt/wasm-micro-runtime that referenced this issue Apr 26, 2024
This allows each native function to require more native stack.

cf. bytecodealliance#3325
wenyongh pushed a commit that referenced this issue Apr 26, 2024
- Add a few API (#3325)
   ```c
   wasm_runtime_detect_native_stack_overflow_size
   wasm_runtime_detect_native_stack_overflow
   ```
- Adapt the runtime to use them
- Adapt samples/native-stack-overflow to use them
- Add a few missing overflow checks in the interpreters
- Build and run the sample on the CI
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this issue May 1, 2024
This is a test code to examine native stack overflow detection logic.

The current output on my environment (macOS amd64):

```shell
====== Interpreter
 stack size   | fail?  | leak?  | exception
---------------------------------------------------------------------------
    0 - 14704 | failed | leaked | Exception: native stack overflow
14704 - 17904 | failed |     ok | Exception: native stack overflow
17904 - 24576 |     ok |     ok |

====== AOT
 stack size   | fail?  | leak?  | exception
---------------------------------------------------------------------------
    0 - 18176 | failed | leaked | Exception: native stack overflow
18176 - 24576 |     ok |     ok |

====== AOT WAMR_DISABLE_HW_BOUND_CHECK=1
 stack size   | fail?  | leak?  | exception
---------------------------------------------------------------------------
    0 -  1968 | failed |     ok | Exception: native stack overflow
 1968 - 24576 |     ok |     ok |
```

This is a preparation to work on relevant issues, including:

bytecodealliance#3325
bytecodealliance#3320
bytecodealliance#3314
bytecodealliance#3297
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this issue May 1, 2024
…3355)

- Add a few API (bytecodealliance#3325)
   ```c
   wasm_runtime_detect_native_stack_overflow_size
   wasm_runtime_detect_native_stack_overflow
   ```
- Adapt the runtime to use them
- Adapt samples/native-stack-overflow to use them
- Add a few missing overflow checks in the interpreters
- Build and run the sample on the CI
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this issue May 2, 2024
This is a test code to examine native stack overflow detection logic.

The current output on my environment (macOS amd64):

```shell
====== Interpreter
 stack size   | fail?  | leak?  | exception
---------------------------------------------------------------------------
    0 - 14704 | failed | leaked | Exception: native stack overflow
14704 - 17904 | failed |     ok | Exception: native stack overflow
17904 - 24576 |     ok |     ok |

====== AOT
 stack size   | fail?  | leak?  | exception
---------------------------------------------------------------------------
    0 - 18176 | failed | leaked | Exception: native stack overflow
18176 - 24576 |     ok |     ok |

====== AOT WAMR_DISABLE_HW_BOUND_CHECK=1
 stack size   | fail?  | leak?  | exception
---------------------------------------------------------------------------
    0 -  1968 | failed |     ok | Exception: native stack overflow
 1968 - 24576 |     ok |     ok |
```

This is a preparation to work on relevant issues, including:

bytecodealliance#3325
bytecodealliance#3320
bytecodealliance#3314
bytecodealliance#3297
Signed-off-by: victoryang00 <victoryang00@ucsc.edu>
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this issue May 2, 2024
…3355)

- Add a few API (bytecodealliance#3325)
   ```c
   wasm_runtime_detect_native_stack_overflow_size
   wasm_runtime_detect_native_stack_overflow
   ```
- Adapt the runtime to use them
- Adapt samples/native-stack-overflow to use them
- Add a few missing overflow checks in the interpreters
- Build and run the sample on the CI

Signed-off-by: victoryang00 <victoryang00@ucsc.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants