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.

Sign up for GitHub

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

Jump to bottom

init_process_flags.is_lock_free check fails on mipsel #45152

Closed
kapouer opened this issue Oct 24, 2022 · 3 comments · Fixed by #45221
Closed

init_process_flags.is_lock_free check fails on mipsel #45152

kapouer opened this issue Oct 24, 2022 · 3 comments · Fixed by #45221
Labels
build Issues and PRs related to build files or the CI. mips Issues and PRs related to the MIPS architecture.

Comments

@kapouer
Copy link
Contributor

kapouer commented Oct 24, 2022

Version

v18.11.0

Platform

Linux 4.19.0-21-octeon #1 SMP Debian 4.19.249-2 (2022-06-30) mips64el (mips64)

Subsystem

node

What steps will reproduce the bug?

Build nodejs with mipsel target. On debian.

How often does it reproduce? Is there a required condition?

Something about the slowness of the host arch maybe ?

What is the expected behavior?

Same thing used to work with nodejs 18.10.0.

What do you see instead?

./out/Release/node --completion-bash > ./debian/nodejs.bash-completion
./out/Release/node[12071]: ../src/node.cc:603:void node::PlatformInit(ProcessFlags::Flags): Assertion `init_process_flags.is_lock_free()' failed.
Aborted

full build log

Additional information

As usual, I know this isn't a supported architecture.
However it might be a hidden bug actually affecting other architectures, only on high load.

Build machine info.
@bnoordhuis
Copy link
Member

The comment for that assert explains it:

// init_process_flags is accessed in ResetStdio(),
// which can be called from signal handlers.

If it's not lock-free, it's protected by a mutex, and mutexes aren't safe to use inside a signal handler (can deadlock.)

Try changing it from an atomic uint64_t to a uint32_t. There are only 12 flags so that should fit and uint32_t likely is lock-free on mipsel. Pull request welcome.

@kvakil kvakil added build Issues and PRs related to build files or the CI. mips Issues and PRs related to the MIPS architecture. labels Oct 26, 2022
@kapouer
Copy link
Contributor Author

kapouer commented Oct 28, 2022

Changing it to std:atomic<uint32_t> works, but you meant I should change it to just uint32_t right ?

@bnoordhuis
Copy link
Member

Sorry, no: std:atomic<uint32_t> - it needs to stay atomic.

kapouer added a commit to kapouer/node that referenced this issue Oct 28, 2022
@kapouer
lock-free init_process_flags for all archs
a35ba7e 
Fix nodejs#45152
@kapouer kapouer mentioned this issue Oct 28, 2022
Merged
kapouer added a commit to kapouer/node that referenced this issue Oct 28, 2022
@kapouer
src: lock-free init_process_flags
d94edb8 
Fix nodejs#45152
kapouer added a commit to kapouer/node that referenced this issue Oct 28, 2022
@kapouer
src: lock-free init_process_flags
2fc42b0 
Fix nodejs#45152
kapouer added a commit to kapouer/node that referenced this issue Oct 28, 2022
@kapouer
src: lock-free init_process_flags
d6147ca 
Fix nodejs#45152
kapouer added a commit to kapouer/node that referenced this issue Oct 28, 2022
@kapouer
src: lock-free init_process_flags
1627557 
Fix nodejs#45152
kapouer added a commit to kapouer/node that referenced this issue Oct 28, 2022
@kapouer
src: lock-free init_process_flags
212d228 
Fix nodejs#45152
nodejs-github-bot pushed a commit that referenced this issue Oct 31, 2022
@kapouer
src: lock-free init_process_flags
3d1bdc9 
Fix #45152

PR-URL: #45221
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
RafaelGSS pushed a commit that referenced this issue Nov 1, 2022
@kapouer @RafaelGSS
src: lock-free init_process_flags
db81931 
Fix #45152

PR-URL: #45221
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
lucshi pushed a commit to lucshi/node that referenced this issue Nov 9, 2022
@kapouer
src: lock-free init_process_flags
82173c1 
Fix nodejs#45152

PR-URL: nodejs#45221
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
RafaelGSS pushed a commit that referenced this issue Nov 10, 2022
@kapouer @RafaelGSS
src: lock-free init_process_flags
97547bc 
Fix #45152

PR-URL: #45221
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
danielleadams pushed a commit that referenced this issue Dec 30, 2022
@kapouer @danielleadams
src: lock-free init_process_flags
9d63883 
Fix #45152

PR-URL: #45221
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
danielleadams pushed a commit that referenced this issue Dec 30, 2022
@kapouer @danielleadams
src: lock-free init_process_flags
8247d10 
Fix #45152

PR-URL: #45221
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
danielleadams pushed a commit that referenced this issue Jan 3, 2023
@kapouer @danielleadams
src: lock-free init_process_flags
bf028a6 
Fix #45152

PR-URL: #45221
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. mips Issues and PRs related to the MIPS architecture.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

lock-free init_process_flags for all archs kapouer/node
3 participants
@kapouer @bnoordhuis @kvakil