search

go-stack / stack

Package stack implements utilities to capture, manipulate, and format call stacks.
MIT License
395 stars 33 forks source link

Refactor checks for sigpanic. #12

Closed dkushner closed 7 years ago

dkushner commented 7 years ago

This commit changes the method by which the library checks for the presence of the runtime sigpanic function. Instead of forcing a sigpanic to then grab a reference to the function, we simply check for the function name. This should be just as reliable as the alternative and does not cause unwanted interactions with debuggers.

dkushner commented 7 years ago

Per this delve issue , #12 and #10

For reference, this issue is experienced when using delve in with the non-native backend. Essentially, it stems from the fact that LLVM is not able to properly intercept EXC_BAD_ACCESS and so cannot allow the running program to deal with the signal. Since go-stack uses an invalid dereference to trigger the sigpanic and grab a reference to the runtime function, delve will get stuck in a constant loop rendering it utterly useless.

This impacts a number of log frameworks that rely on go-stack for grabbing line/function references for log messages.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.9%) to 88.36% when pulling 6ed538dbca442803810d71dd22f06c4f275487a5 on dkushner:bugfix/sigpanic-check into 7a2f19628aabfe68f0766b59e74d6315f8347d22 on go-stack:master.

ChrisHines commented 7 years ago

The link to the delve issue above is broken. Please update your comment to fix the link. Also, can you post benchstat processed before/after benchmarks for this PR? Thanks.

dkushner commented 7 years ago

@ChrisHines, fixed! Benchstat looks like this:

name                   old time/op  new time/op   delta
CallVFmt-8              528ns ± 9%    502ns ± 1%     ~     (p=0.514 n=4+4)
CallPlusVFmt-8          676ns ± 5%    656ns ± 0%     ~     (p=0.343 n=4+4)
CallSharpVFmt-8         516ns ± 6%    499ns ± 2%     ~     (p=0.686 n=4+4)
CallSFmt-8              458ns ± 4%    449ns ± 1%     ~     (p=0.171 n=4+4)
CallPlusSFmt-8          612ns ± 8%    595ns ± 1%     ~     (p=0.943 n=4+4)
CallSharpSFmt-8         467ns ± 5%    442ns ± 2%     ~     (p=0.171 n=4+4)
CallDFmt-8              426ns ± 4%    401ns ± 0%   -5.76%  (p=0.029 n=4+4)
CallNFmt-8              393ns ± 5%    378ns ± 1%     ~     (p=0.200 n=4+4)
CallPlusNFmt-8          358ns ± 5%    350ns ± 1%     ~     (p=1.000 n=4+4)
Caller-8                386ns ± 4%    411ns ± 2%   +6.61%  (p=0.029 n=4+4)
Trace-8                 868ns ± 4%    949ns ± 1%   +9.39%  (p=0.029 n=4+4)
Trace10-8              2.81µs ± 1%   3.40µs ± 1%  +21.23%  (p=0.029 n=4+4)
Trace50-8              4.58µs ± 3%   6.42µs ± 1%  +40.18%  (p=0.029 n=4+4)
Trace100-8             8.12µs ± 4%  11.64µs ± 2%  +43.48%  (p=0.029 n=4+4)
CallerAndVFmt-8         941ns ± 4%    952ns ± 1%     ~     (p=0.286 n=4+4)
TraceAndVFmt-8         2.67µs ± 4%   2.76µs ± 1%     ~     (p=0.343 n=4+4)
Trace10AndVFmt-8       9.35µs ± 2%   9.84µs ± 1%   +5.18%  (p=0.029 n=4+4)
RuntimeCaller-8         400ns ± 5%    394ns ± 2%     ~     (p=0.886 n=4+4)
RuntimeCallerAndFmt-8   734ns ± 4%    716ns ± 2%     ~     (p=0.086 n=4+4)
FuncForPC-8            9.54ns ± 4%   9.38ns ± 1%     ~     (p=0.229 n=4+4)
FuncFileLine-8          127ns ± 6%    124ns ± 1%     ~     (p=0.486 n=4+4)

Moderate increase due to the performance of the fn.Name() call and string VS pointer comparison, I assume.

ChrisHines commented 7 years ago

The changes in this PR were rebased on develop in 3bd81bdde4686ea4aee961921944d1d012253997 and released in https://github.com/go-stack/stack/releases/tag/v1.5.4