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

Add an assert-based segfault handler to etc.linux.memoryerror #20643

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Reavershark
Copy link
Contributor

Continuation of #15331.

Implements registerMemoryAssertHandler and deregisterMemoryAssertHandler.
Has some advantages over the existing registerMemoryErrorHandler:

  • Doesn't use inline assembly.
  • Supports multiple architectures.
  • Usable by all compilers.
  • Properly handles segfaults caused by stack overflows.

Some thoughts from the previous pr summarized:

  • How does this work in multi-threaded programs? How do we want this to work? Write a test?
    Already determined that this breaks stack overflow detection.
  • Does this prevent core dumps from generating? Should it? Can we let the user decide?
  • Does this prevent debuggers from catching the segfault? (no)
  • Is SEGFAULT this the only signal we want to be able to handle? What about SIGFPE, SIGILL, SIGBUS...
    • If so, do we build a more generic and transparent api for this? This can also solve the awkward (re)storing of old signal handlers.
  • Can the dependency on the long deprecated ucontext be removed?
  • Could this be extended to more posix platforms beyond linux?

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @Reavershark! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#20643"

@rikkimax
Copy link
Contributor

rikkimax commented Jan 6, 2025

How does this work in multi-threaded programs? How do we want this to work? Write a test?

The only way to solve this is to throw an exception.

Anything else can cause memory and data corruption.

@Reavershark
Copy link
Contributor Author

Behavior currently depends on how the program calling registerMemoryAssertHandler is compiled:

  • With -check=assert (enabled by default, disabled by -release):
    • With either -checkaction=D or -checkaction=context:
      • An Error is thrown from the signal handler, which unwinds to the location of the segfault and further.
      • Even if unhandled, no core dump is generated.
        • If this happens in a thread, the error can be propagated to another thread calling Thread.join, but is otherwise silent.
    • With -checkaction=C:
      • The segfault reason is printed and abort is called.
      • A core dump is generated (default behaviour of SIGABRT).
    • With -checkaction=halt:
      • A core dump is generated (default behaviour of SIGILL).
  • Without -check=assert (E.g. with -release enabled)
    • The assert still works (assert(false) is kept in release builds), but immediately halts no matter the -checkaction, forcing -checkaction=halt behavior.
    • A core dump is generated (default behaviour of SIGILL).

@Reavershark
Copy link
Contributor Author

Additionally, note that for hardware exception signals like SIGSEGV and SIGFPE, the handler always runs in the thread that caused it. (see man 7 signal).
If a thread masked SIGSEGV, the handler never runs and the process crashes with the usual segmentation fault (core dumped) messsage.

@thewilsonator thewilsonator added the Druntime Specific to druntime label Jan 7, 2025
* providing a more descriptive error message and stack trace if the program is
* compiled with debug info and D assertions (as opposed to C assertions).
*
* Differences with version 1 are:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Differences with version 1 are:
* Differences with version registerMemoryErrorHandler are:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Druntime Specific to druntime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants