This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
Implement stack probing using helpers #26807
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…PROBE in src/inc/readytorun.h src/inc/readytorunhelpers.h
|
/azp run coreclr-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@dotnet/jit-contrib @janvorli PTAL |
|
@BruceForstall These are stack traces when StackOverflow happens in a funclet: win-x64 linux-x64 |
|
EH backtraces look great! |
|
I know you've got test for this. Would it be worthwhile adding this to the repo, even if it isn't built and run as part of the test tree? (I understand your test is somewhat manual in nature.) |
|
@BruceForstall Sure, I will add the test and T4 template that generates the tests |
BruceForstall
suggested changes
Oct 2, 2019
| end | ||
| PAGE_SIZE equ 1000h | ||
|
|
||
| LEAF_ENTRY JIT_StackProbe, _TEXT |
There was a problem hiding this comment.
I'd like to see a line or two of comments in these asm helpers describing the purpose and function of the helper (what it does), in addition to the register "on entry" / "on exit" documentation (which is super useful).
It would also be useful to indicate what all the requirements are around each helper (as the requirements differ per platform). E.g., on Linux you can't probe beyond ESP/RSP.
There was a problem hiding this comment.
@BruceForstall I believe I have addressed all your suggestions - please take a look and let me know if I need to clarify anything else
src/vm/amd64/JitHelpers_Fast.asm
Outdated
|
|
||
| ProbeLoop: | ||
| test dword ptr [rax], eax | ||
| sub rax, PAGE_SIZE |
There was a problem hiding this comment.
The probing math is a little tricky, to ensure exactly the right number of pages are probed. I would suggest a couple comments, e.g.:
or rax, (PAGE_SIZE - 1) ; rax points to the **highest address** on the first unprobed page
; This is done to make the loop end condition simpler.
...
sub rax, PAGE_SIZE ; rax points to the highest address of the next page to probe
...
cmp rax, r11 ; if rax >= r11, then we need to probe the page pointed to by rax.
| @@ -955,5 +955,29 @@ endif ; _DEBUG | |||
|
|
|||
| NESTED_END TailCallHelperStub, _TEXT | |||
|
|
|||
| end | |||
| PAGE_SIZE equ 1000h | |||
There was a problem hiding this comment.
An interesting side-effect here is that the page size (that we probe) is hard-coded to 0x1000, whereas in PAL builds, the page size is currently dynamic. For >4K pages, we might over-probe. But I suppose that is ok -- better perhaps than burning a register to pass in the page size, or creating extra page size specific helpers.
There was a problem hiding this comment.
I though about several options here:
- Have several versions of the helpers (e.g. JIT_StackProbe_0x1000 and JIT_StackProbe_0x10000) on platforms that can have different page sizes (e.g. arm64) and do selection when emitting a call to the helper from JIT.
- Burn a register and pass a page size from the JIT side (which feels weird to me since JIT asks EE for a page size). It's also going to be tough for find a spare register on x86.
- Use a stack for passing parameters and basically do the same as in 2).
- Hard-code the page size as I did
I chose 4 and as a contingency plan if there will be a strong requirement for using "true" page size we can add a logic that will patch the helper during the process startup and adjust the page size.
BruceForstall
approved these changes
Oct 4, 2019
echesakov
commented
Oct 4, 2019
MichalStrehovsky
added a commit
to MichalStrehovsky/coreclr
that referenced
this pull request
Oct 9, 2019
Follow up to dotnet#26807.
MichalStrehovsky
added a commit
that referenced
this pull request
Oct 9, 2019
29 tasks
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #21061
This adds implementation of stack probing via helpers (similar to VC++
_chkstk()function) on win-x86, win-x64, linux-x64 and linux-x86.The following are the stack traces collected on different OS and under different debuggers:
linux-x86 gdb
linux-x64 lldb
linux-x64 gdb - can not unwind the stack beyond
CallDescrWorkerInternalwin-x86 Visual Studio
win-x86 WinDbg
win-x64 Visual Studio
win-x64 WinDbg