Enhance runtime panic function to print backtrace before exiting about nballerina HOT 39 CLOSED

What we can do is specifying the name of the file in filename filed and the absolute path to the directory in directory filed. By looking at examples we can say that the convention is specifying the absolute paths.

@KavinduZoysa I think we need a bit more research to back this proposal. Please look at how other languages are storing paths. Please look if there is a spec relevant to this.

from nballerina.

To get this merged, I suggest we start by having runcheck.sh massage the output of the p test to remove the backtrace.

We can then work on better testing for this. The format should be something like

import ballerina/io;

public function main() {
    int INT_MAX = 9223372036854775807;
    io:println(add(INT_MAX, 1)); // @backtrace 1 main
}

function add(int x, int y) returns int {
    return x + y; // @panic add arithmetic overflow
}

The number in @backtrace is the frame index. The identifier in @panic/@backtrace is the function name. The line numbers come from the line on which @panic or @backtrace occur. We can do this as a separate issue.

from nballerina.

Can we extend this to deal with #8?

from nballerina.

@KavinduZoysa has done a PoC here: https://github.com/KavinduZoysa/test-GCs/tree/backtrace

from nballerina.

I don't understand why we don't need debug info on the call https://github.com/KavinduZoysa/test-GCs/blob/c42cecac3af8eef2e9a5819d08483a73961d4bd5/expr_binary_add.ll#L10

Edited: answer is there's debug info on the call to panic

from nballerina.

does path need to be normalized? is it relative?

from nballerina.

As discussed today, we should probably pass at least a line number to the panic function, eventually column number and filename. Rust does something similar. https://doc.rust-lang.org/core/panic/struct.Location.html

from nballerina.

I got following from C:

/Users/manu/sc/2021-06-07 ❮❮❮ ./a.out
dual_overflow.c:4:15: runtime error: signed integer overflow: 9223372036854775807 + 2 cannot be represented in type 'long'

from https://godbolt.org/z/E474TrhPf
Note that they are generating two calls to @__ubsan_handle_add_overflow one per addition.

from nballerina.

I think it should be possible to have just one call, if we use a phi node to distinguish and load different structs, but not sure it's worth the effort.

from nballerina.

does path need to be normalized? is it relative?

@manuranga, In DIFile node, whatever the string we add as directory will append to the file name and it will be printed out at the log.

!1 = !DIFile(filename: "expr_binary_add.bal", directory: "PATH_TO_BAL_SOURCE")

In my example, I have added PATH_TO_BAL_SOURCE manually.

from nballerina.

As discussed today, we should probably pass at least a line number to the panic function, eventually column number and filename. Rust does something similar. https://doc.rust-lang.org/core/panic/struct.Location.html

@jclark, Currently we are printing the line number, file name and path in the log. I will check how to add the column number as well.

    Abort 
        at foo(PATH_TO_BAL_SOURCE/expr_binary_add.bal:2)

from nballerina.

@jclark, Currently we are printing the line number, file name and path in the log. I will check how to add the column number as well.

@KavinduZoysa I think you misunderstood. Yes you are printing it now, but the information is coming from the backtrace lib. But if you look at above Godbolt information is passed as a struct from the code to trace printer.

from nballerina.

In my example, I have added PATH_TO_BAL_SOURCE manually.

I do understand what is happening.
The questions is, what should it be? What is the convention? Should it be relative to the source root? Should it be an absolute path? Ect.

from nballerina.

In my example, I have added PATH_TO_BAL_SOURCE manually.

I do understand what is happening.
The questions is, what should it be? What is the convention? Is it relative to the source root? Is it an absolute path? Ect.

What we can do is specifying the name of the file in filename filed and the absolute path to the directory in directory filed. By looking at examples we can say that the convention is specifying the absolute paths. Another thing that I noticed, if we specify the path of the file in filename filed, it will discard what we specify in directory filed.

from nballerina.

It looks to me that this comes from DWARF. Look at page 97 "file_names" in http://dwarfstd.org/doc/Dwarf3.pdf

from nballerina.

This needs to handle the mangling in #43.

from nballerina.

@KavinduZoysa If there's no debugging information, what info can we get in the backtrace? I guess we can at least get the names of the functions.

from nballerina.

Follow on from this is #46.

from nballerina.

@KavinduZoysa If there's no debugging information, what info can we get in the backtrace? I guess we can at least get the names of the functions.

@jclark, libbacktrace provides us pc, filename, line no and function name. Among them, we can get pc without debugging information.

from nballerina.

I am surprised function name is not available - the function name is in there to allow linking even if there is no debugging information.

from nballerina.

@jclark, Yes, it is available as you said. I couldn't figure it out earlier. Sorry about it.

In libbacktrace, we can get the function name when pc is given and the debugging information is not provided.

from nballerina.

We should also look at how this works:

https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#stack-traces-and-report-symbolization

from nballerina.

This should be done via a combination of #379 and #46

from nballerina.

We need to set things up so that if a program is not compiled with -g, it does not link in the part of libbacktrace that access the debug symbols.

Edited: don't think this is possible

from nballerina.

This needs to check for errnum being -1 and use backtrace_syminfo instead

from nballerina.

ACK.

from nballerina.

public function main() {
    doPanic(error("help")); // @panic help
}

function doPanic(error e) {
    panic e;
}

@jclark, I generated the LLVM IR for above source with --debug flag. There was no debug info available for call instruction line for _bal_error_construct. Do we have to implement anything on the compiler side?

from nballerina.

Use testSuite\07-error\panic2-p.bal

When I use --debug, I get plenty of debug metadata. This is supposed to have everything you said was needed in #46 (comment)

If there's something missing, please tell me what it is.

from nballerina.

@heshanpadmasiri Please have a look and tell me if I am missing calls to anything. Maybe it needs "Dwarf Version"?

from nballerina.

@jclark, I have couple of concerns here.

1. When we run the bal unit tests with --debug flag, backtrace is printed for panic scenarios. So in order to pass the tests we have to change our scripts to compare the generated backtrace and provided backtrace. According to expect.sh we can compare only one line.
2. The filename is taken from the !DIFile node. In bal test files, this is resolved as a relative path (ex: ../../../compiler/testSuite/01-int/add2-p.bal). So is it fine to print this relative path in the backtrace?

from nballerina.

1. We have two options
   a) No --debug for tests. This means we should have a separate set of unit tests to test backtrace.
   b) Come up with a format for test suite, then change expect.sh Eg:

import ballerina/io;

public function main() {
    int INT_MAX = 9223372036854775807;
    io:println(add(INT_MAX, 1));
}

function add(int x, int y) returns int {
    return x + y; // @panic arithmetic overflow
                  //            add(add2-p.bal:9)
                  //            main(add2-p.bal:5)
}

2. Looks like Rust and Go puts absolute path. That looks weird to me. What happens when you run it in a production environment that is different from the place where you compiled it? I feel like the path relative to project root is the correct one (kind of like Java). At least Go gives a way to trim it using trimpath. Not sure how Rust deals with it. Can you please look into what others are doing and why?

from nballerina.

1. We have two options
   a) No --debug for tests. This means we should have a separate set of unit tests to test backtrace.

In this case also, we will have a backtrace(function name only) because we call backtrace_syminfo, when we get -1 errnum.

1. Looks like Rust and Go puts absolute path. That looks weird to me. What happens when you run it in a production environment that is different from the place where you compiled it?

Yes, I will check what others are doing

from nballerina.

@KavinduZoysa DIFile has a filename part and a directory part. Does libbacktrace combine the two or just use the filename part? We can control exactly what goes into the filename and directory (it comes from the corresponding fields in SourcePart).

from nballerina.

There are two patterns here.

1. If filename is a path, libbacktrace takes only the filename.
   ex: !1 = !DIFile(filename:"../../../compiler/testSuite/01-int/add2-p.bal", directory:"")
   It does not matter what is in the directory, it will only consider filename
2. If filename is not a path, but it is exactly the name of the file, libbacktrace combines it with the directory
   ex: !1 = !DIFile(filename:"add2-p.bal", directory:"../../../compiler/testSuite/01-int")
   The result is ../../../compiler/testSuite/01-int/add2-p.bal

from nballerina.

It's important that the output with --debug gets checked to ensure LLVM accepts it, but I think we should have separate tests to ensure the debug info we generate is producing the right result in conjunction with libbacktrace and our runtime.

Our regular p tests should just check that the compiler is passing the right line number to the panic constructor (which is independent of debug info/libbacktrace).

from nballerina.

@KavinduZoysa What about filename: "modules/foo/foo.bal", directory: "/usr/jjc/nballerina"? Or filename: "./foo.bal", directory: "/usr/jjc/nballerina/foo"?

Does it make any difference with the directory is absolute e.g. filename: "add2-p.bal", directory: "/usr/jjc/compiler/testSuite/01-int"?

from nballerina.

@KavinduZoysa What about filename: "modules/foo/foo.bal", directory: "/usr/jjc/nballerina"? Or filename: "./foo.bal", directory: "/usr/jjc/nballerina/foo"?

These two cases it does not consider what is available in the directory, because filename is a path. So the output is modules/foo/foo.bal and ./foo.bal respectively.

Does it make any difference with the directory is absolute e.g. filename: "add2-p.bal", directory: "/usr/jjc/compiler/testSuite/01-int"?

In this case, it combines the filename and directory, so output is /usr/jjc/compiler/testSuite/01-int/add2-p.bal

from nballerina.

In that case, the runtime should just print out whatever libbacktrace gives it. We can get whatever we want by passing appropriate filename/directory to the compiler.

from nballerina.

This should produce a line number for 07-array/push6-fp.bal

from nballerina.