Skip to content
This repository was archived by the owner on Mar 24, 2022. It is now read-only.

move host_page_size to sysdeps#514

Merged
iximeow merged 1 commit intobytecodealliance:masterfrom
froydnj:move-host-page-size-to-sysdeps
May 11, 2020
Merged

move host_page_size to sysdeps#514
iximeow merged 1 commit intobytecodealliance:masterfrom
froydnj:move-host-page-size-to-sysdeps

Conversation

@froydnj
Copy link
Contributor

@froydnj froydnj commented May 8, 2020

I have this quixotic idea that most OS/arch-specific stuff should be able to live in lucet_runtime_internals::sysdeps; this patch is a first step along that path. Ideally, adding Windows support can be boiled down to figuring out how to expose the right interfaces from sysdeps that everybody can implement. Signals make this very tricky, among other things, but perhaps since Firefox doesn't care about signals + Lucet, the signal support for Windows can somehow be punted down the road.

Happy to hear thoughts about what might be a better path to structuring the codebase for Windows support.

@froydnj froydnj force-pushed the move-host-page-size-to-sysdeps branch from 51d44d4 to f238015 Compare May 8, 2020 18:25
@froydnj froydnj requested a review from acfoltzer May 11, 2020 12:13
Copy link
Contributor

@acfoltzer acfoltzer left a comment

Choose a reason for hiding this comment

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

This seems like a good change to me, thank you! This diff reminds me I've been meaning to convert this to a lazy_static, but that doesn't have to be done right now.

@iximeow I know you've been thinking a lot about cross-platform lately. Does leaning on the sysdeps module tree make sense to you too?

Copy link
Contributor

@iximeow iximeow left a comment

Choose a reason for hiding this comment

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

Does leaning on the sysdeps module tree make sense to you too?

I think so. I wanted to noodle over this because we have several places where a "system-dependent" value, function, ... is actually dependent on both system and architecture. Technically page size is too (x86_64-specific references etc), but we only support x86_64 at the moment, so it doesn't matter much. I was hoping to have some inspiration this weekend in dealing with that particular combinatorial explosion, but in reality it probably won't be too bad and sysdeps with a maybe future archdeps is probably fine.

I hadn't thought about this before in reading this code, but do we blow up on systems with hugepages?

None of these thoughts should block this change though, so ➕ and thank you @froydnj !

@iximeow iximeow merged commit 30ff57b into bytecodealliance:master May 11, 2020
@iximeow
Copy link
Contributor

iximeow commented May 11, 2020

just noting the CI failure on master after merging this commit:
how dare! mac CI failed

this looks like a transient network issue, from looking at the logs:

2020-05-11T19:12:40.6580090Z info: syncing channel updates for 'stable-x86_64-apple-darwin'
2020-05-11T19:13:10.6593780Z error: could not download file from 'https://static.rust-lang.org/dist/channel-rust-stable.toml.sha256' to '/Users/runner/.rustup/tmp/vgmrlcpzfdoxx_mx_file'
2020-05-11T19:13:10.6597480Z info: checking for self-updates
2020-05-11T19:13:40.6927990Z error: could not download file from 'https://static.rust-lang.org/rustup/release-stable.toml' to '/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/rustup-update1Ao2i3/release-stable.toml'
2020-05-11T19:13:40.6928260Z error: caused by: failed to make network request
2020-05-11T19:13:40.6928950Z error: caused by: https://static.rust-lang.org/rustup/release-stable.toml: timed out

I'll rerun the job, and it'll probably pass, so not a big deal.

@froydnj
Copy link
Contributor Author

froydnj commented May 11, 2020

Thanks for the reviews, everyone!

I hadn't thought about this before in reading this code, but do we blow up on systems with hugepages?

Yes, the check that we get back HOST_PAGE_SIZE_EXPECTED will blow things up on hugepage machines, or in some future world where we run on a machine with multiple supported page sizes (e.g. AArch64). Adjusting software to runtime-modifiable page sizes is hard. :(

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants