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

[Resolves #142] Add lifetimes to VAddr & PAddr #150

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

michalfita
Copy link
Contributor

Description

An unfinished attempt to add lifetimes to VAddr & PAddr (as per #142).

@nuta I stuck with:

error[E0308]: mismatched types
   --> runtime/backtrace.rs:113:56
    |
113 |         Backtrace::current_frame().traverse(|_, vaddr: VAddr<'memory>| {
    |                                                        ^^^^^^^^^^^^^^ lifetime mismatch
    |
    = note: expected struct `address::VAddr<'_>`
               found struct `address::VAddr<'memory>`
note: the anonymous lifetime #1 defined here...
   --> runtime/backtrace.rs:113:45
    |
113 |           Backtrace::current_frame().traverse(|_, vaddr: VAddr<'memory>| {
    |  _____________________________________________^
114 | |             if let Some(symbol) = resolve_symbol(vaddr) {
115 | |                 let _ = trace.try_push(CapturedBacktraceFrame::<'memory> {
116 | |                     vaddr,
...   |
120 | |             }
121 | |         });
    | |_________^
note: ...does not necessarily outlive the lifetime `'memory` as defined here
   --> runtime/backtrace.rs:111:43
    |
111 |     pub fn capture() -> CapturedBacktrace<'memory> {
    |                                           ^^^^^^^

I'm not the expert of lifetimes, but I read this a problem with the lifetime of the closure as argument of traverse(). Any suggestions welcome.

Pre-Submission Checklist

When you submit a PR, please make sure your PR satisfies the following checklist:

  • I assert this contribution was authored by me.
  • I license this contribution under the license of this project.
  • The PR title describes your changes briefly and uses the present tense, without a trailing full stop.
  • The PR description describes the reason why we need the change.
  • This is an isolated change. No multiple changes and no unrelated changes are included.
  • Fixed all cargo clippy warnings.
  • Applied rustfmt.

Copy link
Owner

@nuta nuta left a comment

Choose a reason for hiding this comment

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

Some nitpicks.

};
use kerla_utils::alignment::align_down;

/// Represents a physical memory address.
#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
#[repr(transparent)]
pub struct PAddr(usize);
pub struct PAddr<'memory> {
Copy link
Owner

Choose a reason for hiding this comment

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

Use 'a to follow a naming convention in Rust.

Suggested change
pub struct PAddr<'memory> {
pub struct PAddr<'a> {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nuta there's no such convention - lifetimes should be named in meaningful way.

@@ -61,33 +61,33 @@ bitflags! {
#[derive(Debug)]
pub struct PageAllocError;

pub struct OwnedPages {
paddr: PAddr,
pub struct OwnedPages<'memory> {
Copy link
Owner

Choose a reason for hiding this comment

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

Ditto

Suggested change
pub struct OwnedPages<'memory> {
pub struct OwnedPages<'a> {

@nuta
Copy link
Owner

nuta commented Feb 8, 2022

I'm not the expert of lifetimes, but I read this a problem with the lifetime of the closure as argument of traverse(). Any suggestions welcome.

I haven't tested locally but how about deleting : VAddr<'_>? Doesn't rustc infer it?

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.

2 participants