Manually tell V8 where the stack starts whenever we enter it. #544
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
This uses a V8 patch. If the patch isn't present, it automatically skips it and relies on V8 to figure things out the normal way. The patch is needed in order to work around issues in our internal environment, especially the fact that /proc/self/maps is not accessible. The patch isn't needed when using workerd stand-alone.
jasnell
reviewed
Apr 15, 2023
| @@ -2053,6 +2053,26 @@ class Lock { | |||
| bool warningsLogged; | |||
| }; | |||
|
|
|||
| class V8StackScope { | |||
| // One of this MUST be allocated on the stack before taking an isolate lock. It must be allocated | |||
| // before any handles on the stack. This MUST NOT be allocated on the heap. | |||
There was a problem hiding this comment.
Non-blocking nit: should this use KJ_DISALLOW_COPY_AND_MOVE?
jasnell
reviewed
Apr 15, 2023
| @@ -310,7 +310,7 @@ class Isolate: public IsolateBase { | |||
| // constructing a `Lock` on the stack. | |||
|
|
|||
| public: | |||
| Lock(const Isolate& isolate) | |||
| Lock(const Isolate& isolate, V8StackScope& scope) | |||
There was a problem hiding this comment.
I'm assuming that this is passed here only to enforce the rule that it must exist before taking the lock? If so, a comment to that effect here would be helpful to warn off anyone who might come into this later and mistakenly assume that scope should be removed because it is unused here.
jasnell
approved these changes
Apr 15, 2023
mikea
approved these changes
Apr 15, 2023
| // allocated on the heap. The purpose of V8StackScope is to capture the start of the stack | ||
| // range that V8 must scan when performing conservative stack-scanning garbage collection. | ||
| public: | ||
| // No interface. |
There was a problem hiding this comment.
(not necessary) I wonder if in debug mode we can compare this with stack pointer and detect when it was created on the heap.
|
Agreed with comments but will address in a separate PR for logistical reasons. |
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
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.
This uses a V8 patch. If the patch isn't present, it automatically skips it and relies on V8 to figure things out the normal way.
The patch is needed in order to work around issues in our internal environment, especially the fact that /proc/self/maps is not accessible. The patch isn't needed when using workerd stand-alone.