[wasm interpreter] Add trace in LoadMem/StoreMem builtin [v8/v8 : main]
王忠齐 (Gerrit)
王忠齐 added 1 comment![]( "Open in Gerrit")
Related details
- Paolo Severini
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paolo Severini (Gerrit)
Paolo Severini added 3 comments![]( "Open in Gerrit")
Thank you!
If there is a bug in `ShadowStack::Print` we should fix it, but I am not sure we should add push/pop tracing to the load/store builtins. Calling these C++ functions from precompiled builtins seems a little fragile to me, even though DrumBrake tracing is not enabled by default and even require an additional build flag.
__ pushq(rax);
__ pushq(rcx);
__ pushq(rdx);
__ pushq(r8);
__ pushq(r9);
__ pushq(r10);
__ pushq(r11);Is this working in all architectures?
On Linux x64 we need to save and restore RAX RCX RDX RSI RDI R8 R9 R10 R11
On Windows x64 we need to save only RAX RCX RDX R8 R9 R10 R11.
But when the instruction handlers deal with floating point we also need to save fp registers:
On Linux XMM0–XMM15
On Windows XMM0–XMM5
But there is also the arm64 version in interpreter-builtins-arm64.cc to consider.
EmitTracePush<0>(masm, wasm_runtime,We should pass true or false instead of 1 or 0, or even better, an enum value that describes the meaning of the template argument.
Related details
- 王忠齐
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
王忠齐 (Gerrit)
王忠齐 added 3 comments![]( "Open in Gerrit")
__ pushq(rax);
__ pushq(rcx);
__ pushq(rdx);
__ pushq(r8);
__ pushq(r9);
__ pushq(r10);
__ pushq(r11);Is this working in all architectures?
On Linux x64 we need to save and restore RAX RCX RDX RSI RDI R8 R9 R10 R11
On Windows x64 we need to save only RAX RCX RDX R8 R9 R10 R11.But when the instruction handlers deal with floating point we also need to save fp registers:
On Linux XMM0–XMM15
On Windows XMM0–XMM5But there is also the arm64 version in interpreter-builtins-arm64.cc to consider.
Thanks for your advise. I just consider the register currently in use. This will make the program more robust.
We should pass true or false instead of 1 or 0, or even better, an enum value that describes the meaning of the template argument.
- Paolo Severini
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
王忠齐 (Gerrit)
王忠齐 added 2 comments![]( "Open in Gerrit")
Thank you!
If there is a bug in `ShadowStack::Print` we should fix it, but I am not sure we should add push/pop tracing to the load/store builtins. Calling these C++ functions from precompiled builtins seems a little fragile to me, even though DrumBrake tracing is not enabled by default and even require an additional build flag.
The other plan is add the trace functions into to the instruction table like trace_PushConstI32Slot, but it the case we need to save extra stack_offset and kind in it. I think it may cause others error in the future, because we need to save twice stack_offset in the trace function and the LoadMem builtin. If we changed one and forget the other, it will get a wrong result. As a dev tool, I think only save one stack_offset in the LoadMem/StoreMem builtin will produce less incorrect results, so I add the trace function in the LoadMem/StoreMem builtin.
__ pushq(rax);
__ pushq(rcx);
__ pushq(rdx);
__ pushq(r8);
__ pushq(r9);
__ pushq(r10);
__ pushq(r11);王忠齐Is this working in all architectures?
On Linux x64 we need to save and restore RAX RCX RDX RSI RDI R8 R9 R10 R11
On Windows x64 we need to save only RAX RCX RDX R8 R9 R10 R11.But when the instruction handlers deal with floating point we also need to save fp registers:
On Linux XMM0–XMM15
On Windows XMM0–XMM5But there is also the arm64 version in interpreter-builtins-arm64.cc to consider.
Thanks for your advise. I just consider the register currently in use. This will make the program more robust.
In arm64, LoadMem/StoreMem builtins haven't been supported. Maybe I shouldn't add it currently, because I cann't test it now. I think it could be supported with the LoadMem/StoreMem builtin in arm64. I will add it to my plan, and I will do it when I am free.
Related details
- Paolo Severini
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paolo Severini (Gerrit)
Paolo Severini added 2 comments![]( "Open in Gerrit")
王忠齐Thank you!
If there is a bug in `ShadowStack::Print` we should fix it, but I am not sure we should add push/pop tracing to the load/store builtins. Calling these C++ functions from precompiled builtins seems a little fragile to me, even though DrumBrake tracing is not enabled by default and even require an additional build flag.
The other plan is add the trace functions into to the instruction table like trace_PushConstI32Slot, but it the case we need to save extra stack_offset and kind in it. I think it may cause others error in the future, because we need to save twice stack_offset in the trace function and the LoadMem builtin. If we changed one and forget the other, it will get a wrong result. As a dev tool, I think only save one stack_offset in the LoadMem/StoreMem builtin will produce less incorrect results, so I add the trace function in the LoadMem/StoreMem builtin.
A apologize for not having provided a better solution sooner.
I checked better and in V8 there is already a portable way to call C++ function from builtins, through the `CallRuntime` assembly macro.
Something like:
```
__ CallRuntime(Runtime::kWasmInterpreterTracePop, <args>);
```
We should reuse this mechanism, without recreating it.
__ pushq(rax);
__ pushq(rcx);
__ pushq(rdx);
__ pushq(r8);
__ pushq(r9);
__ pushq(r10);
__ pushq(r11);王忠齐Is this working in all architectures?
On Linux x64 we need to save and restore RAX RCX RDX RSI RDI R8 R9 R10 R11
On Windows x64 we need to save only RAX RCX RDX R8 R9 R10 R11.But when the instruction handlers deal with floating point we also need to save fp registers:
On Linux XMM0–XMM15
On Windows XMM0–XMM5But there is also the arm64 version in interpreter-builtins-arm64.cc to consider.
王忠齐Thanks for your advise. I just consider the register currently in use. This will make the program more robust.
In arm64, LoadMem/StoreMem builtins haven't been supported. Maybe I shouldn't add it currently, because I cann't test it now. I think it could be supported with the LoadMem/StoreMem builtin in arm64. I will add it to my plan, and I will do it when I am free.
You are totally right that these builtins are not supported on arm64.
Related details
- 王忠齐
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
王忠齐 (Gerrit)
王忠齐 added 1 comment![]( "Open in Gerrit")
王忠齐Thank you!
If there is a bug in `ShadowStack::Print` we should fix it, but I am not sure we should add push/pop tracing to the load/store builtins. Calling these C++ functions from precompiled builtins seems a little fragile to me, even though DrumBrake tracing is not enabled by default and even require an additional build flag.
Paolo SeveriniThe other plan is add the trace functions into to the instruction table like trace_PushConstI32Slot, but it the case we need to save extra stack_offset and kind in it. I think it may cause others error in the future, because we need to save twice stack_offset in the trace function and the LoadMem builtin. If we changed one and forget the other, it will get a wrong result. As a dev tool, I think only save one stack_offset in the LoadMem/StoreMem builtin will produce less incorrect results, so I add the trace function in the LoadMem/StoreMem builtin.
A apologize for not having provided a better solution sooner.
I checked better and in V8 there is already a portable way to call C++ function from builtins, through the `CallRuntime` assembly macro.
Something like:
```
__ CallRuntime(Runtime::kWasmInterpreterTracePop, <args>);
```
We should reuse this mechanism, without recreating it.
It only be used in a JS function currently, the rumtime function will get param as JS abi. It looks strange that passing a JS value from a c function to the other c function.
Related details
- Paolo Severini
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
王忠齐 (Gerrit)
王忠齐 added 1 comment![]( "Open in Gerrit")
王忠齐Thank you!
If there is a bug in `ShadowStack::Print` we should fix it, but I am not sure we should add push/pop tracing to the load/store builtins. Calling these C++ functions from precompiled builtins seems a little fragile to me, even though DrumBrake tracing is not enabled by default and even require an additional build flag.
Paolo SeveriniThe other plan is add the trace functions into to the instruction table like trace_PushConstI32Slot, but it the case we need to save extra stack_offset and kind in it. I think it may cause others error in the future, because we need to save twice stack_offset in the trace function and the LoadMem builtin. If we changed one and forget the other, it will get a wrong result. As a dev tool, I think only save one stack_offset in the LoadMem/StoreMem builtin will produce less incorrect results, so I add the trace function in the LoadMem/StoreMem builtin.
王忠齐A apologize for not having provided a better solution sooner.
I checked better and in V8 there is already a portable way to call C++ function from builtins, through the `CallRuntime` assembly macro.
Something like:
```
__ CallRuntime(Runtime::kWasmInterpreterTracePop, <args>);
```
We should reuse this mechanism, without recreating it.
It only be used in a JS function currently, the rumtime function will get param as JS abi. It looks strange that passing a JS value from a c function to the other c function.
maybe __ CallCFunction will be better.
Related details
- Paolo Severini
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
王忠齐 (Gerrit)
王忠齐 added 2 comments![]( "Open in Gerrit")
王忠齐Thank you!
If there is a bug in `ShadowStack::Print` we should fix it, but I am not sure we should add push/pop tracing to the load/store builtins. Calling these C++ functions from precompiled builtins seems a little fragile to me, even though DrumBrake tracing is not enabled by default and even require an additional build flag.
Paolo SeveriniThe other plan is add the trace functions into to the instruction table like trace_PushConstI32Slot, but it the case we need to save extra stack_offset and kind in it. I think it may cause others error in the future, because we need to save twice stack_offset in the trace function and the LoadMem builtin. If we changed one and forget the other, it will get a wrong result. As a dev tool, I think only save one stack_offset in the LoadMem/StoreMem builtin will produce less incorrect results, so I add the trace function in the LoadMem/StoreMem builtin.
王忠齐A apologize for not having provided a better solution sooner.
I checked better and in V8 there is already a portable way to call C++ function from builtins, through the `CallRuntime` assembly macro.
Something like:
```
__ CallRuntime(Runtime::kWasmInterpreterTracePop, <args>);
```
We should reuse this mechanism, without recreating it.
王忠齐It only be used in a JS function currently, the rumtime function will get param as JS abi. It looks strange that passing a JS value from a c function to the other c function.
maybe __ CallCFunction will be better.
Done
Related details
- Paolo Severini
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paolo Severini (Gerrit)
Paolo Severini added 1 comment![]( "Open in Gerrit")
__ movq(rdi, wasm_runtime);
__ movq(rsi, Immediate(kind));
__ movq(rdx, slot_offset);We overwrite the values of rdi, rsi rdx (or other registers on Windows) which might be used by the code later. Usually in these builtins rdx is the stack pointer, rcx points to the current bytecode and r8 point to the wasm_runtime. So the following handlers probably don't work.
You might try to build and run all mjsunit tests with the `jitless` variant locally, with something like this, in the /src/v8 folder:
```
python3 tools/run-tests.py --variants=jitless --outdir=..\out\debug_x64 mjsunit/wasm/*
```
Related details
- 王忠齐
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
王忠齐 (Gerrit)
王忠齐 added 1 comment![]( "Open in Gerrit")
__ movq(rdi, wasm_runtime);
__ movq(rsi, Immediate(kind));
__ movq(rdx, slot_offset);We overwrite the values of rdi, rsi rdx (or other registers on Windows) which might be used by the code later. Usually in these builtins rdx is the stack pointer, rcx points to the current bytecode and r8 point to the wasm_runtime. So the following handlers probably don't work.
You might try to build and run all mjsunit tests with the `jitless` variant locally, with something like this, in the /src/v8 folder:
```
python3 tools/run-tests.py --variants=jitless --outdir=..\out\debug_x64 mjsunit/wasm/*
```
Thanks, I find the bug too. I have check the CallCFunction, it didn't save the caller saved register for us. So I save the caller saved register as before in patch set 6.
Related details
- Paolo Severini
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paolo Severini (Gerrit)
Paolo Severini added 2 comments![]( "Open in Gerrit")
FrameScope scope(masm, StackFrame::MANUAL);What is the purpose of a `FrameScope` here?
SaveCallerSavedRegister(masm);We are saving registers at the beginning of the builtin and then restoring them at the end, but some registers, like rcx==code pointer, rdx==stack pointer, but also others, can be modified inside the builtin and we need to go to the next builtin with the modified value.
I think you should just save the registers you are using to pass values to `CallCFunction` before calling it and then restore them immediately after the call.
You should verify that everything works by running at least the wasm-interpreter.js tests with `--jitless --wasm-jitless --trace-drumbrake-execution --trace-drumbrake-execution-verbose` (in addition to all the other arguments).
Related details
- 王忠齐
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
王忠齐 (Gerrit)
王忠齐 added 2 comments![]( "Open in Gerrit")
FrameScope scope(masm, StackFrame::MANUAL);What is the purpose of a `FrameScope` here?
There is a DCHECK in CallCFunction, a FrameScope is needed by it.
SaveCallerSavedRegister(masm);We are saving registers at the beginning of the builtin and then restoring them at the end, but some registers, like rcx==code pointer, rdx==stack pointer, but also others, can be modified inside the builtin and we need to go to the next builtin with the modified value.
I think you should just save the registers you are using to pass values to `CallCFunction` before calling it and then restore them immediately after the call.
You should verify that everything works by running at least the wasm-interpreter.js tests with `--jitless --wasm-jitless --trace-drumbrake-execution --trace-drumbrake-execution-verbose` (in addition to all the other arguments).
Thanks for your advise, save register just before call instruction may cann't restore the register used as paramter. And I will test it locally.
Related details
- Paolo Severini
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
王忠齐 (Gerrit)
王忠齐 added 1 comment![]( "Open in Gerrit")
SaveCallerSavedRegister(masm);王忠齐We are saving registers at the beginning of the builtin and then restoring them at the end, but some registers, like rcx==code pointer, rdx==stack pointer, but also others, can be modified inside the builtin and we need to go to the next builtin with the modified value.
I think you should just save the registers you are using to pass values to `CallCFunction` before calling it and then restore them immediately after the call.
You should verify that everything works by running at least the wasm-interpreter.js tests with `--jitless --wasm-jitless --trace-drumbrake-execution --trace-drumbrake-execution-verbose` (in addition to all the other arguments).
Thanks for your advise, save register just before call instruction may cann't restore the register used as paramter. And I will test it locally.
I have pass the test case wasm-interpreter.js in Patchset 7, PTAL.
Related details
- Paolo Severini
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |