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

Help Clang find bottom of stacks for safety, use desired stack size #873

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dtzWill
Copy link

@dtzWill dtzWill commented Apr 1, 2022

noteBottomOfStack:

Without this, checks against stack space within Clang don't work as
Clang doesn't know where the stack begins.

Needed per-thread, as early as possible.
(on threads using Clang)

Using Clang's desired stack size:

Additionally increase stack size of pthreads to Clang's desired size.

This is presently 8MB, and is used by Clang's stack management
mechanisms to check* if close to stack exhaustion when determining if
there's sufficient space (and warn or run on a new thread with more).
(see runWithSufficientStackSpace)

The constant is available in LLVM 7 onwards.

  • (abs(cur - bottom) > DesiredStackSize - threshold)

noteBottomOfStack:

Without this, checks against stack space within Clang don't work as
Clang doesn't know where the stack begins.

Needed per-thread, as early as possible.
(on threads using Clang)

Using Clang's desired stack size:

Additionally increase stack size of pthreads to Clang's desired size.

This is presently 8MB, and is used by Clang's stack management
mechanisms to check* if close to stack exhaustion when determining if
there's sufficient space (and warn or run on a new thread with more).
(see runWithSufficientStackSpace)

The constant is available in LLVM 7 onwards.

* (abs(cur - bottom) > DesiredStackSize - threshold)
@dtzWill
Copy link
Author

dtzWill commented Apr 1, 2022

  • Didn't add to the windows spawnThread, if that's desired (and otherwise this seems good) I can take a look! 👍
  • main maybe doesn't need to noteBottomOfStack (if it never calls into clang on that thread), but it doesn't hurt.
  • Reviewing the various threads spawned elsewhere (via std::thread) made it seem this (spawnThread) was the best point to add this, but it's maybe not ideal.

This fixes crashes encountered pointing ccls at CIRCT (+llvm submodule), which was my motivation. I haven't looked into making that reproducible (on other systems), so not sure if that's specific to my setup. Stack was very deep (just shy of 10k frames) and included many calls to runWithSufficientStackSpace that were doing nothing. For some reason this actually took down my ccls instance (despite CrashRecoveryContext/runSafely), which made it particularly problematic.

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

Successfully merging this pull request may close these issues.

None yet

1 participant