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

Don't rely on promotion of PageTableEntry::new inside a const fn #175

Merged
merged 2 commits into from Sep 3, 2020

Conversation

ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Sep 1, 2020

Hi! Rust compiler team member here. When the const_fn feature is enabled, the implementation of PageTable::new relies on a compiler bug to work. Specifically, PageTableEntry::new should not be eligible for promotion as part of an array initializer. See rust-lang/rust#75502 for more information. This bug may be fixed in the future, which would cause this crate to stop compiling.

Hi! Rust compiler team member here. When the `const_fn` feature is enabled, the implementation of `PageTable::new` relies on a compiler bug to work. Specifically, `PageTableEntry::new` should not be eligible for promotion as part of an array initializer. See rust-lang/rust#75502 for more information. This bug may be fixed in the future, which will cause this crate to stop compiling.
@phil-opp
Copy link
Member

phil-opp commented Sep 2, 2020

Thanks a lot for the early warning and for taking the time to fix this for us!

I'm wondering if the following would work too:

    pub const fn new() -> Self {
        const EMPTY: PageTableEntry = PageTableEntry::new();
        PageTable {
            entries: [EMPTY; ENTRY_COUNT],
        }
    }

Here we initialize the array with a const, which should be ok, right? And assigning the result of a const fn to a const should be ok as well, or am I missing something?

The advantage of this implementation would be that it doesn't need access to the private entry field of the PageTableEntry struct, which seems a bit cleaner to me. (I'm happy to implement this change myself, I just wanted to ensure that isn't buggy too.)

@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Sep 2, 2020

That works too! The problems that can result from promoting calls to const fn are described in more detail in rust-lang/const-eval#19. In this particular case, promotion of a panicking const fn would actually be fine, since if an array initializer of a non-Copy type cannot be promoted, compilation will fail regardless. However, we're not yet willing to commit to these semantics for the unstable const_in_array_repeat_expressions feature, hence the (possible) breakage.

@phil-opp
Copy link
Member

phil-opp commented Sep 3, 2020

Thanks a lot for the explanation!

@phil-opp phil-opp merged commit ed9fb42 into rust-osdev:master Sep 3, 2020
phil-opp added a commit that referenced this pull request Sep 3, 2020
@phil-opp
Copy link
Member

phil-opp commented Sep 3, 2020

Published as v0.11.5.

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.

None yet

2 participants