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

enable v8_enable_local_off_stack_check compile time flag #2752

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

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Sep 19, 2024

Referencing the original commit introducing the flag:

This CL introduces a compile-time flag v8_enable_local_off_stack_check
which enforces a run-time DCHECK, that all v8::Local objects are
indeed stack-allocated. The check is disabled by default. It will
fail for all heap data structures containing local handles.

Local handles (object of the v8::Local) should never be allocated on the heap, and we should use v8::LocalVector instead of heap allocation. This flag ensures that we follow v8 recommendation.

@anonrig anonrig requested review from a team as code owners September 19, 2024 20:47
.bazelrc Outdated Show resolved Hide resolved
@fhanau
Copy link
Collaborator

fhanau commented Sep 19, 2024

Nit: We reproduce V8 build flags in compile_flags.txt so that they are available to clangd when editing code – let's add the define there too.

@anonrig
Copy link
Member Author

anonrig commented Sep 20, 2024

I've submitted a patch to v8 to fix this error. https://chromium-review.googlesource.com/c/v8/v8/+/5876953

@jasnell
Copy link
Member

jasnell commented Sep 23, 2024

Should be noted that the v8 patch is temporary. A future v8 update will include it and the patch will be dropped.

Copy link
Collaborator

@fhanau fhanau left a comment

Choose a reason for hiding this comment

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

Code LGTM, this can probably go in after the update to 13.0. I wonder if there's any performance impact with this. It looks like Local<> stops being trivially copyable and the Local constructor calls StackAllocated::VerifyOnStack() which is mostly a DCHECK – I'd imagine that's an acceptable impact.
I could also see libkj's fibers performing arcane magic on the stack that cause the check to fail when it shouldn't, but let's try this out.

@kentonv
Copy link
Member

kentonv commented Sep 30, 2024

I could also see libkj's fibers performing arcane magic on the stack that cause the check to fail when it shouldn't, but let's try this out.

We don't call any V8 APIs on fibers so this shouldn't be an issue.

@jasnell
Copy link
Member

jasnell commented Oct 2, 2024

Optional Side Quest: we should see about a clang-tidy rule to catch uses of things like std::vector<v8::Local<T>>, I know this compile time flag is supposed to help check that but a clang-tidy rule might be a bit faster to help flag issues in development.

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.

4 participants