generic stack trace overriding mechanism

[stefantalpalaru]

Lightweight stack traces using an external libbacktrace wrapper: https://github.com/status-im/nim-libbacktrace

This addresses #12702 and will be helped by #12735 (on macOS)

These stack traces require --debuger:native for debugging symbols and look the same as Nim's default ones: https://travis-ci.org/status-im/nim-libbacktrace/jobs/626489399

This small PR adds support for libbacktrace both in the compiler itself (try it with ./koch boot -d:libbacktrace --debugger:native -d:release) and in external programs that import libbacktrace.

This last requirement is unfortunate. Is there some stdlib module, besides "system", where I can plug this instead of asking users to import it?

Also, any elegant way of showing an error message if the external Nim package is not installed on the system?

[Araq]

Have you considered to add a var stackTraceHook: proc () {.nimcall.} to excpt.nim instead?

[timotheecour]

IMO every new defined symbol in nim repository sources should be prefixed with nim => nimlibbacktrace; this prevents clashes with user defined symbols (but what'd be even better is supporting namespace, eg defined(nim.libbacktrace))

[timotheecour]

exportc names are global so, again, should be prefixed to prevent potential clashes with user exportc code (including C code in this case!) => nimGetBacktrace()

[timotheecour]

@stefantalpalaru I'm trying to test this PR, please help and let me know which steps / flags are superfluous/wrong

on OSX here:

test.nim:

import pkg/libbacktrace
proc main() = doAssert false
main()

git clone https://github.com/status-im/nim-libbacktrace && cd nim-libbacktrace && nimble develop && nimble install

cd $nim_prs_D
git fetch origin pull/12922/head
git checkout FETCH_HEAD
./koch boot -d:libbacktrace --debugger:native -d:release
./bin/nim c --skipUserCfg --skipParentCfg -d:release -d:libbacktrace --debugger:native -g $timn_D/tests/nim/all/t0842.nim
dsymutil $timn_D/tests/nim/all/t0842
$timn_D/tests/nim/all/t0842

Traceback (most recent call last)
Error: unhandled exception: t0842.nim(7, 24) `false`  [AssertionError]

=> no stacktrace...

[stefantalpalaru]

Have you considered to add a var stackTraceHook: proc () {.nimcall.} to excpt.nim instead?

Yes, @Araq, but I didn't know how to implement such a hook. I see some in "lib/system/gc_hooks.nim". Anything else I can look at?

Remember that I need this to also run inside the compiler, so I still have to enable the conditional importing of the external module. It would be nice to do that with a single custom define, say -d:stacktraceHook:mymodule and then I hardcode the proc's name inside that external module.

Can you think of any nicer way?

@timotheecour, you probably have to also dsymutil the compiler binary, since the exception stack trace starts inside it. Also, --debugger:native and -g are the same thing and you can use koch temp instead of koch boot for a quicker iteration.

[Araq]

Well these hooks have all pretty much the same problems:

• You still need to import the overriding modules explicitly. (Though I consider this superior to yet more command line options or configuration setttings.)
• Overhead of an indirect function call that optimizers struggle with.
• Until the import statement that sets the hook to something is in effect, the old behaviour remains. (!) And that's the real nasty one...

[timotheecour]

@stefantalpalaru

you probably have to also dsymutil the compiler binary, since the exception stack trace starts inside it

I tried but didn't change, and I don't think the exception stack trace starts inside it makes sense, eg as you can see from lldb backtrace the only binary involved is t0842, not nim

after investigating, the problem for me was -d:release which, in this example, due to aggressive inlining, was stripping ALL stackframes (the remaining unskipped ones were filtered out by libstacktrace as it's the boilerplate stack beginning NimMain etc and ending raiseExceptionAux etc which don't contain useful debug info)

it did work without -d:release, or if we change clang.options.speed = "-O3" to the rather useless clang.options.speed = "-O0" in nim.cfg

but here's a working example demoing stacktrace WITH -d:release:

  import std/json
  type
    Foo* = ref object
      z1*: string
    Baz* = ref object
      foo2*: Foo
  proc testAll*()=
    let s = """{"foo": {"z1": "hi"}}"""
    let j = parseJson(s)
    discard j.to(Baz)
  testAll()

./bin/nim c -d:release --skipUserCfg --skipParentCfg -d:libbacktrace --debugger:native --stacktrace:off -g $timn_D/tests/nim/all/t0842.nim
dsymutil $timn_D/tests/nim/all/t0842
$timn_D/tests/nim/all/t0842

Traceback (most recent call last)
lib/pure/json.nim(1229) to
lib/pure/json.nim(1063) initFromJson
lib/pure/json.nim(1122) initFromJson
lib/pure/json.nim(944) initFromJson
Error: unhandled exception: key not found: .foo2 [KeyError]

more frames would show without -d:release:

Traceback (most recent call last v2)
/Users/timothee/git_clone/nim/timn/tests/nim/all/t0842.nim(61) t0842
/Users/timothee/git_clone/nim/timn/tests/nim/all/t0842.nim(59) testAll
lib/pure/json.nim(1229) to
lib/pure/json.nim(1063) initFromJson
lib/pure/json.nim(1122) initFromJson
lib/pure/json.nim(944) initFromJson
Error: unhandled exception: key not found: .foo2 [KeyError]

[timotheecour]

@Araq @stefantalpalaru

This last requirement is unfortunate. Is there some stdlib module, besides "system", where I can plug this instead of asking users to import it?
Well these hooks have all pretty much the same problems:

here's a better approach: the following works, I tried:
step 1: in nimble install libbacktrace, also do: nim c --app:lib -o:install/usr/lib/libbacktrace3.dylib ./libbacktrace.nim (1 time)
step 2: add -d:libbacktrace:/Users/timothee/git_clone/nim/nim-libbacktrace/install/usr/lib/libbacktrace3.dylib to your config.nims or nim.cfg etc or to cmd line and update ur PR as follows:

when defined(libbacktrace):
  const libbacktrace {.strdefine.} = ""
  {.passL: libbacktrace.}
  proc getBacktrace*(): string {.importc.}

./bin/nim c -d:release --skipUserCfg --skipParentCfg -d:libbacktrace:/Users/timothee/git_clone/nim/nim-libbacktrace/install/usr/lib/libbacktrace3.dylib --debugger:native --stacktrace:off -g $timn_D/tests/nim/all/t0842.nim

proc getBacktrace*(): string {.importc, dynlib: "/Users/timothee/git_clone/nim/nim-libbacktrace/install/usr/lib/libbacktrace3.dylib2".}

note: as another benefit, it is more lightweight (no implicit compile step inside libbacktrace.nim when u import since u don't need to import that anymore)

Also, any elegant way of showing an error message if the external Nim package is not installed on the system?

if provided /Users/timothee/git_clone/nim/nim-libbacktrace/install/usr/lib/libbacktrace3.dylib doesn't exist, it'll give a reasonable err msg at runtime (as intended, instead of compile time; since we need that dylib at RT)

note

this did NOT work, and seems like a new nim compiler bug: EDIT: => #12929

when defined(libbacktrace):
  const libbacktrace {.strdefine.} = ""
  proc getBacktrace*(): string {.importc, dynlib: libbacktrace.}  # this acts as if libbacktrace = "", which is a BUG

EDIT: but this workaround does work:

when defined(libbacktrace):
  const libbacktrace {.strdefine.} = ""
  const libbacktrace2 = libbacktrace
  proc getBacktrace*(): string {.importc, dynlib: libbacktrace2.}

[stefantalpalaru]

I think I found a better way of doing it. When "nimStackTraceOverride" is defined, a new registration mechanism appears in the system module: registerStackTraceOverride().

Now all that's left to do is for an imported module to use it. With the latest modifications in nim-libbacktrace, we have this:

when defined(nimStackTraceOverride):
  registerStackTraceOverride(getBacktrace)

Applying the override to the compiler itself: ./koch boot -d:release --debugger:native -d:nimStackTraceOverride --import:libbacktrace (throw in a -d:debug to see it dump a stack trace in compiler.msgs.quit() when you nim c -r ... something with a non-zero exit)

Applying it to an external program: ./bin/nim c -r -f --stacktrace:off --debugger:native -d:nimStackTraceOverride --import:libbacktrace foo.nim

Stack trace header changed to "Traceback (most recent call last, using override)" to avoid confusion.

As you can see, this is a generic overriding system that's no longer tied to nim-libbacktrace. Let the competition begin :-)

[timotheecour]

I think I found a better way of doing it

I don't think that's a better way:

• --import:libbacktrace is much more intrusive than what I suggested above, it creates an implicit import in each module; now each module implicitly depends on libbacktrace as well as libbacktrace's own (recursive) dependencies, which includes libbacktrace_wrapper, os, system/ansi_c; so even the smallest hello world would now drag a bunch of dependencies. This affects compile times and has other unwanted effects.

• because of lack of cyclic imports, this would make backtraces unavailable in modules that are in dependency list of libbacktrace.nim, eg try this:

  import std/os
  proc main*()=
    setCurrentDir("/nonexistant")
  main()

with your approach you'd get:

Traceback (most recent call last, using override)
Stack trace override procedure not registered.
Error: unhandled exception: No such file or directory [OSError]

with the approach I suggested above in #12922 (comment) I get:

Traceback (most recent call last, using override)
/Users/timothee/git_clone/nim/timn/tests/nim/all/t0842.nim(78) t0842
/Users/timothee/git_clone/nim/timn/tests/nim/all/t0842.nim(77) testAll
lib/pure/os.nim(1304) setCurrentDir
lib/pure/includes/oserr.nim(94) raiseOSError
Error: unhandled exception: No such file or directory [OSError]

likewise with other modules eg those in system/*.nim etc
(in both cases, without -d:release to avoid inlining frames)

The approach from #12922 (comment) has none of those drawbacks, is non-intrusive, and doesn't require anything in user code apart from compilining with a compilation flag (which could be in user config.nims); all it boils down to at the end of the day is some dlopen/(or its windows equivalent) call that gets automatically called during nim's module initialization

[stefantalpalaru]

--import:libbacktrace is much more intrusive than what I suggested above, it creates an implicit import in each module

No, it doesn't. It's only imported once per project. Verify it by adding static: echo "importing libbacktrace" to "nim-libbacktrace/libbacktrace.nim".

Also, how can you write that with a straight face after proposing -d:libbacktrace:/Users/timothee/git_clone/nim/nim-libbacktrace/install/usr/lib/libbacktrace3.dylib? That looks good to you?

try this:

  import std/os
  proc main*()=
    setCurrentDir("/nonexistant")
  main()

with your approach you'd get:

Traceback (most recent call last, using override)
Stack trace override procedure not registered.
Error: unhandled exception: No such file or directory [OSError]

You're holding it wrong.

./bin/nim c -r -f --verbosity:0 --hints:off --stacktrace:off --debugger:native -d:nimStackTraceOverride --import:libbacktrace ../../nim/test/t0843.nim:

Traceback (most recent call last, using override)
/mnt/sda3/storage/CODE/nim/test/t0843.nim(4) t0843
/mnt/sda3/storage/CODE/nim/test/t0843.nim(3) main
/mnt/sda3/storage/CODE/00_github/Nim/lib/pure/os.nim(1304) setCurrentDir
/mnt/sda3/storage/CODE/00_github/Nim/lib/pure/includes/oserr.nim(94) raiseOSError
Error: unhandled exception: No such file or directory [OSError]
Error: execution of an external program failed: '/mnt/sda3/storage/CODE/nim/test/t0843 '
Traceback (most recent call last, using override)
/mnt/sda3/storage/CODE/00_github/Nim/compiler/nim.nim(106) nim
/mnt/sda3/storage/CODE/00_github/Nim/compiler/nim.nim(95) handleCmdLine
/mnt/sda3/storage/CODE/00_github/Nim/compiler/msgs.nim(443) rawMessage
/mnt/sda3/storage/CODE/00_github/Nim/compiler/msgs.nim(440) rawMessage
/mnt/sda3/storage/CODE/00_github/Nim/compiler/msgs.nim(361) handleError
/mnt/sda3/storage/CODE/00_github/Nim/compiler/msgs.nim(346) quit

The approach from #12922 (comment) has none of those drawbacks, is non-intrusive, and doesn't require anything in user code apart from compilining with a compilation flag (which could be in user config.nims)

You want the user to point you to a dynamic library so you can dlopen a hardcoded symbol from it and you think this is easier and more flexible than importing a Nim module. Am I reading this right?

[stefantalpalaru]

BTW, the proposed overriding mechanism is flexible enough for you to implement your preferred dlopen() strategy (or even direct linking to a shared/static library) outside the compiler. Put it into a "libbacktrace_dlopen.nim" file and import that.

[timotheecour]

--import:libbacktrace is much more intrusive than what I suggested above, it creates an implicit import in each module

No, it doesn't. It's only imported once per project. Verify it by adding static: echo "importing libbacktrace" to "nim-libbacktrace/libbacktrace.nim".

it does create an implicit import in each module, check the nim compiler source code, as well as this example:
nim c -r --import:os main.nim

# main.nim: implicitly imports os
import ./main2
echo ExtSep
# main2.nim: implicitly imports os
echo ExtSep

adding static: echo "importing libbacktrace" doesn't disprove this: imported modules are processed once, even if imported multiple times from different modules.

You're holding it wrong.

my bad, I had forgotten to git pull your other repo https://github.com/status-im/nim-libbacktrace since your last commit; indeed the example I had does work; a contrived example where my point would still hold would be to add a top-level expression that would fail at runtime (eg adding doAssert falseyCondition in std/os or any module recursively imported from libbacktrace.nim); I'm suspecting there could be real cases where this could be an issue but can't think of one right now.

Also, how can you write that with a straight face after proposing -d:libbacktrace:/Users/timothee/git_clone/nim/nim-libbacktrace/install/usr/lib/libbacktrace3.dylib? That looks good to you?

yes, that approach looks good: that explicit path was just an example to make it work; if that lib is installed in a OS-specific standard location (say via nimble install), that path doesn't need to be given explicitly and can be guessed, so that -d:libbacktrace would be sufficient for auto-guessing, and easily over-rideable with -d:libbacktrace:explicitpath (which can be put in ~/.config/nim/config.nims once and then forgotten).

Nim compiler/standard library relies heavily on this feature to load symbols, see all the dynlib: instances in nim sources (eg pcre.nim)

All you need is: proc getBacktrace*(): string {.importc, dynlib: guessLibbacktrace().}

You want the user to point you to a dynamic library so you can dlopen a hardcoded symbol from it and you think this is easier and more flexible than importing a Nim module. Am I reading this right?

yes this is better, for same reasons as already mentioned. nim compiler / stdlib heavily relies on dlopen-ing symbols already (see all the {.dynlib.}).

hopefully convincing example

main.nim:

echo "ok1"

with the approach i suggested:

Hint: system [Processing]
Hint: widestrs [Processing]
Hint: io [Processing]
Hint: t0842 [Processing]
CC: stdlib_io.nim
CC: stdlib_system.nim
CC: t0842.nim
Hint:  [Link]
Hint: operation successful (21970 lines compiled; 0.342 sec total; 19.871MiB peakmem; Debug Build) [SuccessX]
ls t0842 => 91K

with your approach --import:libbacktrace:

Hint: system [Processing]
Hint: widestrs [Processing]
Hint: io [Processing]
Hint: t0842 [Processing]
Hint: libbacktrace [Processing]
Hint: libbacktrace_wrapper [Processing]
Hint: os [Processing]
Hint: strutils [Processing]
Hint: parseutils [Processing]
Hint: math [Processing]
Hint: bitops [Processing]
Hint: macros [Processing]
Hint: algorithm [Processing]
Hint: unicode [Processing]
Hint: pathnorm [Processing]
Hint: osseps [Processing]
Hint: posix [Processing]
Hint: times [Processing]
Hint: options [Processing]
Hint: typetraits [Processing]
CC: libbacktrace_wrapper
CC: stdlib_io.nim
CC: stdlib_system.nim
CC: stdlib_posix.nim
CC: stdlib_times.nim
CC: stdlib_os.nim
CC: ../../../../nim-libbacktrace/libbacktrace.nim
CC: t0842.nim
Hint:  [Link]
Hint: operation successful (43783 lines compiled; 0.745 sec total; 58.344MiB peakmem; Debug Build) [SuccessX]
ls t0842 => 155K

as you can see, this is worse in all dimensions: binary size, compile time, number of depdencies involved, memory consumed.

("small files" do occur in practice eg for inim (interactivity matters), for creating shared libraries etc; there's no reason to un-necessarily add un-needed module dependencies )

btw: just wanted to mention your PR is much appreciated

[Araq]

We might need a better override mechanism for system.nim related functionality.

[stefantalpalaru]

btw: just wanted to mention your PR is much appreciated

Thanks! I'm sure it will become even better as a result of our technical arguments.

it does create an implicit import in each module

OK, but I can't measure the overhead in practice. I added an ugly macro to "compiler/nim.nim" that imports the module defined with -d:nimStackTraceOverride=libbacktrace so I could drop --import:libbacktrace from the ./koch boot ... command line. No difference in compilation time or peak memory usage, which convinces me that this importing overhead is negligible.

Yes, importing a module that imports "os" in a "hello world" program increases compilation time and memory usage. Should we really rewrite currentSourcePath and parentDir() just to have cheap stack traces in toy programs?

Now, I can save you from having to compile a C or C++ file by giving you static libraries instead of those {.compile.} pragmas in "libbacktrace.nim", but don't expect to shave more than a few milliseconds from that.

What you're asking for, with your dlopen and dynamic lib strategy is incredibly ugly from a system administrator's perspective. You want the language-specific package manager abomination to install libraries in "/usr/local/lib" or something, without the ability to uninstall them (or deal with file collisions with other packages) and create hidden dependencies for all Nim programs built this way, even the statically linked ones that some unsuspecting user might think are ready for shipping. Alternatively, you might want to hardcode library search paths with -rpath which is just as bad.

We can do better than resort to these ugly hacks borne out of the lack of sysadmin experience on OSs without official package managers. Think Linux, think discipline, think package hygiene. Dynamic libraries should be linked to, by default, not dlopen-ed, while static libraries are what you use when you don't want to compile the same files more than once, but you want those objects in your binary.

[stefantalpalaru]

I've replaced the "compile" pragmas with static libs in nim-libbacktrace and, more importantly, I got it to work on 64-bit Windows, after figuring out that the problem was the libunwind shipped by the Mingw-w64 version of GCC. Getting LLVM's libunwind to compile in that environment was not trivial.

We might need a better override mechanism for system.nim related functionality.

How are other projects doing it?

[stefantalpalaru]

I've added some debugging support: you see all trace lines when you export NIM_LIBBACKTRACE_DEBUG=1.

Since returning an empty stack trace when NimMainModule() is inlined due to -d:release is ugly, I'm now showing a couple of useless lines. What I would really need is __attribute__((noinline)) or the MSVC equivalent on that function.

t0843.nim:

import os
proc main*()=
  setCurrentDir("/nonexistent")
main()

./bin/nim c -r -f --verbosity:0 --hints:off -d:release --debugger:native -d:nimStackTraceOverride --import:libbacktrace ../../nim/test/t0843.nim:

Traceback (most recent call last, using override)
/mnt/sda3/storage/CODE/00_github/Nim/lib/system.nim(3636) main
/mnt/sda3/storage/CODE/00_github/Nim/lib/system.nim(3629) NimMain
Error: unhandled exception: No such file or directory [OSError]
Error: execution of an external program failed: '/mnt/sda3/storage/CODE/nim/test/t0843 '

[timotheecour]

I've added some debugging support: you see all trace lines when you export NIM_LIBBACKTRACE_DEBUG=1.

depending on environment variable is not very nim-esque, eg, it has global effects; such customizations would be best replaced by, for example, an api call

ISince returning an empty stack trace when NimMainModule() is inlined due to -d:release is ugly, I'm now showing a couple of useless lines.

instead of adding useless lines, you can make it clear with:

Traceback (most recent call last) [empty due to inlining]

What I would really need is attribute((noinline)) or the MSVC equivalent on that function.

this works for me: nim c -r -d:release --passC:-fno-inline ... (on OSX but probably same on linux/gcc) and shows full stacktrace

[stefantalpalaru]

depending on environment variable is not very nim-esque, eg, it has global effects; such customizations would be best replaced by, for example, an api call

Good thing it's not language dependent, then. Environment variables for debugging are an established practice: https://github.com/jemalloc/jemalloc/wiki/Use-Case%3A-Heap-Profiling

Traceback (most recent call last) [empty due to inlining]

That lacks the minimal info that it happened in the main function.

--passC:-fno-inline

Does not work on my system, with -d:release and GCC. I reduced it down to --passC:"-O1 -fno-inline -foptimize-sibling-calls" (with -Og removed from gcc.options.debug, so it doesn't override -O1). This "sibling calls" optimisation makes NimMainInner(), NimMainModule() and main__ggjB1fzBf7f45c3gvw3ZbA() disappear in here:

N_LIB_PRIVATE N_NIMCALL(void, main__ggjB1fzBf7f45c3gvw3ZbA)(void) {
	setCurrentDir__gH49bWY7pApxazoTIU6NIpwos(((NimStringDesc*) &TM__G9ciTsqXW32ZKpy9cO3Nf6ag_2));
}

N_LIB_PRIVATE N_NIMCALL(void, NimMainModule)(void) {
{
	main__ggjB1fzBf7f45c3gvw3ZbA();
}
}

N_CDECL(void, NimMainInner)(void) {
	NimMainModule();
}

[stefantalpalaru]

This works with -d:release: --passC:"-fno-inline -fno-optimize-sibling-calls"

[Araq]

Good thing it's not language dependent, then. Environment variables for debugging are an established practice: https://github.com/jemalloc/jemalloc/wiki/Use-Case%3A-Heap-Profiling

Not related to this PR but env vars are quite in conflict with "reproducible" builds, established practice or not is irrelevant.

[stefantalpalaru]

env vars are quite in conflict with "reproducible" builds

You control the environment in which the build is reproduced and that includes environment variables: https://www.debian.org/doc/debian-policy/ch-source.html#reproducibility

[Araq]

Fair enough I guess... Still terrible to rely on interprocess global state for anything.