Skip to content

ffi: fix float passing in libffi for riscv64 - #63976

Closed
kxxt wants to merge 1 commit into
nodejs:mainfrom
kxxt:rv-float-ffi
Closed

ffi: fix float passing in libffi for riscv64#63976
kxxt wants to merge 1 commit into
nodejs:mainfrom
kxxt:rv-float-ffi

Conversation

@kxxt

@kxxtkxxt commented Jun 18, 2026

Copy link
Copy Markdown
Member

libffi was incorrectly passing floats as doubles on RISC-V. This PR applies the merged upstream fix (libffi/libffi#972) for the vendored copy of libffi since libffi appears to have a very long release cycle (last release was at 2025-08-02).

This fixes the following three new test failures caught in https://github.com/riscv-forks/node-riscv/actions/runs/27544719447/job/81437248188:

out/Release/node --experimental-ffi --expose-gc --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /home/kxxt/node-riscv/test/ffi/test-ffi-dynamic-library.js
out/Release/node --experimental-ffi --expose-internals --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /home/kxxt/node-riscv/test/ffi/test-ffi-shared-buffer.js
out/Release/node --experimental-ffi --expose-gc --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /home/kxxt/node-riscv/test/ffi/test-ffi-calls.js

CC @nodejs/platform-riscv64

I am testing this PR on riscv64 at https://github.com/riscv-forks/node-riscv/actions/runs/27756747452/job/82120430409?pr=4

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/ffi
  • @nodejs/security-wg

@nodejs-github-botnodejs-github-bot added dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run. labels Jun 18, 2026
@kxxt

kxxt commented Jun 18, 2026

Copy link
Copy Markdown
MemberAuthor

It appears that there is some problem with NixOS binary cache that causes CI to fail.

error: hash mismatch importing path '/nix/store/bl7rmhhsy7vjb9qm3jfwgqpv3cn7wfb1-openssl-3.6.2';
specified: sha256:0nagwij5x8yi54380yw53xy1ksn9jclp98lxmizbqq6hgngqp553
got: sha256:1fypgxjwksn6kh2ig4fmaskfnflicjq60zf5il2s9nxblhkkvnww
error: path '/nix/store/bl7rmhhsy7vjb9qm3jfwgqpv3cn7wfb1-openssl-3.6.2' is required, but there is no substituter that can build it

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

libffi was incorrectly passing floats as doubles on RISC-V.
This PR applies the merged upstream fix (libffi/libffi#972) for the
vendored copy of libffi since libffi appears to have a very long release
cycle (last release was at 2025-08-02).
This fixes the following three new test failures:
test/ffi/test-ffi-dynamic-library.js
test/ffi/test-ffi-shared-buffer.js
test/ffi/test-ffi-calls.js
Signed-off-by: Levi Zim <rsworktech@outlook.com>
@kxxt

kxxt commented Jun 21, 2026

Copy link
Copy Markdown
MemberAuthor

I have rebased this PR to fix the flaky test in the CI (#63923).

@ShogunPanda

Copy link
Copy Markdown
Contributor

I think this will be handled by #64040 which seems to include this fix. Do you think we can close this?

@kxxt

kxxt commented Jun 21, 2026

Copy link
Copy Markdown
MemberAuthor

I think this will be handled by #64040 which seems to include this fix. Do you think we can close this?

Yes. It’s great to see a new libffi release just a few days after my fix was merged, considering the last release happened 10 month ago. I am closing this PR in favor of #64040.

@kxxtkxxt closed this Jun 21, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependenciesPull requests that update a dependency file.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@kxxt@nodejs-github-bot@ShogunPanda@mcollina