Implement segfault handler in Crystal #10463

maxfierke · 2021-03-02T05:19:32Z

Resolves #3839

Required adding some C bindings for various signal structures,
based on rust's fairly comprehensive libc bindings.

This removes the need to build and link a separate C library and
reduces the complexity of cross-compiling (at build-time), with
some added complexity when porting to a new platform.

siginfo_t can be fairly complex, involving large context-dependent
unions that vary across architecture and platform. In Crystal's
case, we only care about the segfault version of the structure,
so we omit the others.

Opening as draft first for feedback before more thoroughly verifying on all supported platforms. Tier 1 platforms verified so far. will take some time to get to the BSDs, etc.

Tier I
- All verified (CI)
Tier 2 (manual)
- DragonFly BSD
  - No available ports. Ran into issues cross-compiling (different LLVM versions available, libressl instead of openssl), but validated that siginfo_t bindings look correct
- FreeBSD (12.0)
  - std_spec hangs on io_spec but completes the segfault handler tests fine
- NetBSD
  - Ports has too old a version to compile Crystal master, but based on reading their libc source, siginfo_t bindings look correct.
- OpenBSD
  - Ports only has Crystal 0.30.1 which is too old to compile Crystal master, and ran into issues cross-compiling. However, can verify the siginfo_t bindings seem correct (at least on releases from summer 2017 forward)

Required adding some C bindings for various signal structures, based on rust's fairly comprehensive libc bindings. This removes the need to build and link a separate C library and reduces the complexity of cross-compiling (at build-time), with some added complexity when porting to a new platform. `siginfo_t` can be fairly complex, involving large context-dependent unions that vary across architecture and platform. In Crystal's case, we only care about the segfault version of the structure, so we omit the others.

si_addr was erroneously char* on OpenBSD < 2017 but has since been fixed (openbsd/src@91587a9)

straight-shoota

Thanks, this looks really great and should surely help avoid some friction with distribution packages and cross-compiling.

src/signal.cr

straight-shoota · 2021-03-05T20:49:57Z

This requires an update to https://github.com/crystal-lang/distribution-scripts/ after merging.

maxfierke · 2021-04-07T15:38:20Z

@straight-shoota is this something that could be slotted in for 1.1.0? I'd be happy to create a PR for the distribution-scripts work to support it, if that's something that's possible for someone outside the core team to do.

straight-shoota · 2021-04-07T15:52:16Z

We still need a second core team member to review and approve this PR.

Contributions to distribution-scripts are welcome 👍

asterite · 2021-04-07T16:03:36Z

src/signal.cr

@@ -56,6 +56,7 @@ enum Signal : Int32
  USR1   = LibC::SIGUSR1
  USR2   = LibC::SIGUSR2
  WINCH  = LibC::SIGWINCH
+  STKSZ  = LibC::SIGSTKSZ


Does this need to be exposed in Singal?

I would guess probably not, unless there's a reason to expose stack size to users, but it's not clear to me whether we consider LibC bindings to be considered public API, and not clear whether sigaltstack and other functions for working with stacks would be relevant to most, if any, users. So I'll probably update this to avoid exposing STKSZ on Signal

LibC bindings are not public API. They are there to be used by the standard library. Users can use LibC, but in most cases they shouldn't need to.

So yes, for now let's not add it to Signal's public API. We could always add it later if needed.

great! addressed in 786c7d9

asterite

This is GREAT! I just left a small question.

Fryguy · 2021-04-07T16:20:34Z

src/signal.cr

+    if is_stack_overflow
+      Crystal::System.print_error "Stack overflow (e.g., infinite or very deep recursion)\n"
+    else
+      Crystal::System.print_error "Invalid memory access (signal %d) at address 0x%lx\n", sig, addr


Not for this PR, but would it be helpful to print the name of the signal? In my experience the number is hard to understand. Perhaps something like:

Suggested change

Crystal::System.print_error "Invalid memory access (signal %d) at address 0x%lx\n", sig, addr

Crystal::System.print_error "Invalid memory access (signal %d: SIG%s) at address 0x%lx\n", sig, Signal.new(sig), addr

I noticed that CI failed because it couldn't find 1.0.0: https://github.com/84codes/sparoid/runs/3166205736?check_suite_focus=true#step:3:48 Apparently their new repo ("openSUSE Build Service") will only host the latest release: crystal-lang/crystal#10949 (comment) ...but there's no Docker image for 1.1.1 right now. Also no need to compile src/ext/sigfault.c since crystal-lang/crystal#10463

maxfierke force-pushed the mf-crystal_libext branch from 074e63d to c558360 Compare March 2, 2021 06:17

straight-shoota added kind:feature topic:stdlib:runtime kind:refactor platform labels Mar 2, 2021

maxfierke added 2 commits March 2, 2021 18:06

SigsetT#val should be ULong[16] on aarch64

5b88d9f

maxfierke force-pushed the mf-crystal_libext branch from c558360 to 5b88d9f Compare March 3, 2021 03:36

hugopl mentioned this pull request Mar 4, 2021

Rename "sigfault" to "segfault" #10454

Closed

si_addr should be Void* on OpenBSD too

e48bc47

si_addr was erroneously char* on OpenBSD < 2017 but has since been fixed (openbsd/src@91587a9)

maxfierke marked this pull request as ready for review March 5, 2021 16:47

straight-shoota reviewed Mar 5, 2021

View reviewed changes

straight-shoota added the topic:infrastructure label Mar 5, 2021

Move __crystal_sigfault_handler into segfault_handler proc

fc93817

straight-shoota approved these changes Mar 6, 2021

View reviewed changes

maxfierke mentioned this pull request Mar 8, 2021

Bug: Undefined symbol _roundeven #10475

Closed

asterite reviewed Apr 7, 2021

View reviewed changes

asterite approved these changes Apr 7, 2021

View reviewed changes

Fryguy reviewed Apr 7, 2021

View reviewed changes

maxfierke added 2 commits April 7, 2021 12:04

Merge branch 'master' into mf-crystal_libext

6427b47

Don't expose STKSZ on Signal

786c7d9

straight-shoota approved these changes Apr 8, 2021

View reviewed changes

asterite added this to the 1.1.0 milestone Apr 8, 2021

asterite merged commit a5014ed into crystal-lang:master Apr 8, 2021

maxfierke mentioned this pull request Apr 12, 2021

Remove compilation of libcrystal.a crystal-lang/distribution-scripts#87

Merged

dentarg mentioned this pull request Jul 27, 2021

Use Crystal 1.1 in CI 84codes/sparoid#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement segfault handler in Crystal #10463

Implement segfault handler in Crystal #10463

maxfierke commented Mar 2, 2021 •

edited

Loading

straight-shoota left a comment

straight-shoota commented Mar 5, 2021

maxfierke commented Apr 7, 2021

straight-shoota commented Apr 7, 2021

asterite Apr 7, 2021

maxfierke Apr 7, 2021

asterite Apr 7, 2021

maxfierke Apr 8, 2021

asterite left a comment

Fryguy Apr 7, 2021

	Crystal::System.print_error "Invalid memory access (signal %d) at address 0x%lx\n", sig, addr
	Crystal::System.print_error "Invalid memory access (signal %d: SIG%s) at address 0x%lx\n", sig, Signal.new(sig), addr

Implement segfault handler in Crystal #10463

Implement segfault handler in Crystal #10463

Conversation

maxfierke commented Mar 2, 2021 • edited Loading

straight-shoota left a comment

Choose a reason for hiding this comment

straight-shoota commented Mar 5, 2021

maxfierke commented Apr 7, 2021

straight-shoota commented Apr 7, 2021

asterite Apr 7, 2021

Choose a reason for hiding this comment

maxfierke Apr 7, 2021

Choose a reason for hiding this comment

asterite Apr 7, 2021

Choose a reason for hiding this comment

maxfierke Apr 8, 2021

Choose a reason for hiding this comment

asterite left a comment

Choose a reason for hiding this comment

Fryguy Apr 7, 2021

Choose a reason for hiding this comment

maxfierke commented Mar 2, 2021 •

edited

Loading