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

update __st_ino_truncated from i32 to i64 #19567

Closed
wants to merge 1 commit into from

Conversation

walkingeyerobot
Copy link
Collaborator

According to system/lib/libc/musl/arch/emscripten/bits/stat.h, __st_ino_truncated is a long, so should be i64. I was running my os in such a way that I was getting large enough inodes that this was causing problems.

According to system/lib/libc/musl/arch/emscripten/bits/stat.h, __st_ino_truncated is a long, so should be i64. I was running my os in such a way that I was getting large enough inodes that this was causing problems.
@kripken
Copy link
Member

kripken commented Jun 8, 2023

I think long is 32 bits on wasm32 - were you testing on wasm64?

@sbc100
Copy link
Collaborator

sbc100 commented Jun 8, 2023

I guess the problem happens with -sNODERAWFS when the native/raw inode number is > 32 bit.. should we change our internal stat definition to be 64-bit?

@walkingeyerobot is your code sensitive to inode numbers? What is it doing with them?

@walkingeyerobot
Copy link
Collaborator Author

yes, this was running with -sNODERAWFS and the raw inode numbers were just too large.

@sbc100
Copy link
Collaborator

sbc100 commented Jun 8, 2023

yes, this was running with -sNODERAWFS and the raw inode numbers were just too large.

And what is the side effect? do we assert somewhere in emscripten? Or is the code under test that failing because the number is wrong?

@walkingeyerobot
Copy link
Collaborator Author

emscripten is asserting, and that's failing: https://github.com/emscripten-core/emscripten/blob/main/src/parseTools.js#L383-L387

Aborted(Assertion failed: value (10989164315) too large to write as 32-bit value)

@walkingeyerobot
Copy link
Collaborator Author

cc @trybka

@sbc100
Copy link
Collaborator

sbc100 commented Jun 8, 2023

So we could fix my have NODERAWFS somehow truncate the inode numbers .. or we could update our headers..

@sbc100
Copy link
Collaborator

sbc100 commented Jun 8, 2023

Oh wait, it called st_ino_truncated for a reason. We should just truncate it by doing stat.ino >>> 0 on that line

sbc100 added a commit that referenced this pull request Jun 8, 2023
This field is designed to hold a potentially truncated inode number,
as opposes the st_ino which hold the full 64-bit value.

Fixes: #19567
sbc100 added a commit that referenced this pull request Jun 8, 2023
Musl needs these in order to support the linux kernel but we don't need
to use the same ABI in emscripten.

Fixes: #19567
@sbc100 sbc100 closed this in #19569 Jun 8, 2023
sbc100 added a commit that referenced this pull request Jun 8, 2023
Musl needs these in order to support the linux kernel but we don't need
to use the same ABI in emscripten.

Fixes: #19567
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.

3 participants