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

Improve diagnostic when gstd without 'debug' #1857

Closed
gshep opened this issue Nov 17, 2022 · 3 comments · Fixed by #2174, #2200 or #4098
Closed

Improve diagnostic when gstd without 'debug' #1857

gshep opened this issue Nov 17, 2022 · 3 comments · Fixed by #2174, #2200 or #4098
Labels
C1-feature Feature request P4-consider Lowest priority Q3-involved

Comments

@gshep
Copy link
Member

gshep commented Nov 17, 2022

Problem to Solve

When an execution of a program built with gstd/nodebug hits expect/assert/panic/unreachable/etc we get in the logs just short message 'probably unreachable reached`. It doesn't point whether it is program logic fails and where it was or an error rose on executor side.

Possible Solution

The idea is to introduce new syscall (or two):

+
+        pub fn gr_panic(file: u64, line: u32, column: u32);
+        pub fn gr_panic2(location: &core::panic::Location<'_>) -> !;
     }
 }
 // ...
+pub fn panic(loc: &core::panic::Location<'_>) -> ! {
+    unsafe {
+        sys::gr_panic(loc.file().as_ptr() as u64, loc.line(), loc.column());
+        sys::gr_panic2(loc)
+    }
+}

Implementation of gr_panic is just to put data to the log and interrupt the execution. gr_panic2 forces compiler to put Location instance somewhere and to provide a pointer to it. Panic handler could possibly look then like this:

--- a/gstd/src/common/handlers.rs
+++ b/gstd/src/common/handlers.rs
@@ -48,8 +48,11 @@ pub fn oom(_: Layout) -> ! {
#[cfg(not(debug_assertions))]
#[cfg(target_arch = "wasm32")]
#[panic_handler]
-pub fn panic(_: &PanicInfo) -> ! {
-    wasm32::unreachable();
+pub fn panic(panic_info: &PanicInfo) -> ! {
+    match panic_info.location() {
+        Some(loc) => gcore::msg::panic(loc),
+        _ => wasm32::unreachable(),
+    }
}

demo-panicker WAT looks with these changes applied the following way:

(module
 (type (;0;) (func (param i32)))
 (type (;1;) (func))
 (type (;2;) (func (param i64 i32 i32)))
 (import "env" "gr_panic" (func (;0;) (type 2)))
 (import "env" "gr_panic2" (func (;1;) (type 0)))
 (import "env" "memory" (memory (;0;) 17))
 (func (;2;) (type 1)
   i32.const 1048620
   call 3
   unreachable)
 (func (;3;) (type 0) (param i32)
   (local i32 i32)
   local.get 0
   call 5
   unreachable)
 (func (;4;) (type 1)
   i32.const 1048652
   call 3
   unreachable)
 (func (;5;) (type 0) (param i32)
   local.get 0
   call 6
   unreachable)
 (func (;6;) (type 0) (param i32)
   local.get 0
   call 7
   unreachable)
 (func (;7;) (type 0) (param i32)
   local.get 0
   i64.load32_u
   local.get 0
   i32.load offset=8
   local.get 0
   i32.load offset=12
   call 0
   local.get 0
   call 1
   unreachable)
 (export "handle" (func 2))
 (export "init" (func 4))
 (data (;0;) (i32.const 1048576) "I just panic every timepanicker/src/lib.rs\00\00\17\00\10\00\13\00\00\00\08\00\00\00\05\00\00\00Panic in init!\00\00\17\00\10\00\13\00\00\00\0e\00\00\00\05"))

wasm-proc could then process the binary, extract source file paths to the new info file (so called 'debug info') and squeeze data element a bit:

@@ -3,10 +3,9 @@
 (type (;1;) (func))
 (type (;2;) (func (param i64 i32 i32)))
 (import "env" "gr_panic" (func (;0;) (type 2)))
-  (import "env" "gr_panic2" (func (;1;) (type 0)))
 (import "env" "memory" (memory (;0;) 17))
 (func (;2;) (type 1)
-    i32.const 1048620
+    i32.const 1048576
   call 3
   unreachable)
 (func (;3;) (type 0) (param i32)
@@ -15,7 +14,7 @@
   call 5
   unreachable)
 (func (;4;) (type 1)
-    i32.const 1048652
+    i32.const 1048588
   call 3
   unreachable)
 (func (;5;) (type 0) (param i32)
@@ -34,9 +33,7 @@
   local.get 0
   i32.load offset=12
   call 0
-    local.get 0
-    call 1
   unreachable)
 (export "handle" (func 2))
 (export "init" (func 4))
-  (data (;0;) (i32.const 1048576) "I just panic every timepanicker/src/lib.rs\00\00\17\00\10\00\13\00\00\00\08\00\00\00\05\00\00\00Panic in init!\00\00\17\00\10\00\13\00\00\00\0e\00\00\00\05"))
+  (data (;0;) (i32.const 1048576) "\17\00\10\00\13\00\00\00\08\00\00\00\17\00\10\00\13\00\00\00\0e\00\00\00"))

However for this to work it is required somehow to force compiler to put all Locations in the new standalone section and file paths to the different one.

Notes

Disadvantage: too complicated

@gshep gshep added C1-feature Feature request P4-consider Lowest priority labels Nov 17, 2022
@gshep
Copy link
Member Author

gshep commented Nov 17, 2022

inspired by #1821

@breathx
Copy link
Member

breathx commented Nov 23, 2022

This syscall will cost some real funds and we should "lock" them for possible panic case at the start.
One more solution from my side here is to provide instead of Error::Unknown on the backend side something like Error::Unreachable(Option<.. last ext error ..>) with message like "reason is unknown, unreachable instruction called, but it may be due to: {ext.last_error} "

@grishasobol
Copy link
Member

make also asserts in tests more precisely, see files:
pallets/gear/src/tests.rs
gtest/src/program.rs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C1-feature Feature request P4-consider Lowest priority Q3-involved
Projects
None yet
3 participants