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

src: lazily initialize global BuiltinLoader #46256

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Jan 18, 2023

Using simdutf during startup can fail when compilation units are linked in the wrong order by a static initialization order issue.

#45942 would also solve this, but since that is a larger effort, this patch focuses on solving the immediate problem. Alternatively, simdutf should arguably be made usable during static initialization, but that is also not as straightforward as this patch.

Fixes: #46235

Using simdutf during startup can fail when compilation units are
linked in the wrong order by a static initialization order issue.

nodejs#46235 would also solve this,
but since that is a larger effort, this patch focuses on solving
the immediate problem. Alternatively, simdutf should arguably
be made usable during static initialization, but that is also not
as straightforward as this patch.

Fixes: nodejs#46235
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@addaleax addaleax requested a review from anonrig January 18, 2023 16:55
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jan 18, 2023
@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 18, 2023
Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

looks good to me 👍

@lemire
Copy link
Member

lemire commented Jan 18, 2023

@addaleax

Working on a patch in simdutf. I flagged the issue in simdutf back in 2021... simdutf/simdutf#87 but it was theoretical at the time and I never revisited it.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 18, 2023
@nodejs-github-bot
Copy link
Collaborator

@lemire
Copy link
Member

lemire commented Jan 18, 2023

@addaleax If you prefer, I have the following patch in simdutf: simdutf/simdutf#209 It should solve the issue.

@lemire
Copy link
Member

lemire commented Jan 18, 2023

Upgrading to simdutf 3.1.0 is an alternative fix.

@addaleax
Copy link
Member Author

@anonrig Do you want to update #46211? This does seem like the better fix to me if we can.

@anonrig
Copy link
Member

anonrig commented Jan 18, 2023

@anonrig Do you want to update #46211? This does seem like the better fix to me if we can.

I can do it in a couple of hours. I agree.

@addaleax
Copy link
Member Author

I'll close this in favor of #46257, thanks @lemire and @anonrig for fixing this!

@addaleax addaleax closed this Jan 18, 2023
@addaleax addaleax deleted the 46235-dev branch January 18, 2023 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

segfault with shared builtin
4 participants