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

Refactor exception code into new interrupts module #475

Merged
merged 8 commits into from
Oct 18, 2018
Merged

Refactor exception code into new interrupts module #475

merged 8 commits into from
Oct 18, 2018

Conversation

acheronfail
Copy link
Contributor

@acheronfail acheronfail commented Oct 17, 2018

As discussed in #473 and https://github.com/acheronfail/blog_os/pull/1/files.

This PR re-writes the 6th and 7th posts so that all exception related code lives in src/interrupts.rs.

Copy link
Owner

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

= help: have you added the `#[macro_use]` on the module/import?
```

And this error occurs because the `print!` and `println!` macros we created in the `vga_buffer` _must be defined_ before we use them. This is one case where import order matters in Rust. We can easily fix this by ensuring the order of our imports places the macros first:
Copy link
Owner

Choose a reason for hiding this comment

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

Isn't the problem also that macro_use is missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately no, it really is the order of the imports. Just adding #[macro_use] by itself will still result in the following error:

error: cannot find macro `print!` in this scope                                                                                                                                               
  --> src/interrupts.rs:52:5                                                                                                                                                                  
   |                                                                                                                                                                                          
52 |     print!(".");                                                                                                                                                                         
   |     ^^^^^                                                                                                                                                                                
   |                                                                                                                                                                                          
   = help: have you added the `#[macro_use]` on the module/import?                                                                                                                            

Due to the way that Rust compiles macros, they must be defined first before they're used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

I meant that we need to change the order and add macro_use (i.e. the macro_use was not there before, so we should probably mention in the text that we need to add it).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah right! Sorry I misunderstood. I'll add that in now. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went for something more verbose because I think it's clearer for those new to Rust - this is a case which can be confusing for some.

pub mod serial;
```

Now we can use our `print!` and `println!` macros in `interrupts.rs`. If you'd like to know more about the ins and outs of macros and how they differ from functions [you can find more information here](in-depth-rust-macros).
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
Now we can use our `print!` and `println!` macros in `interrupts.rs`. If you'd like to know more about the ins and outs of macros and how they differ from functions [you can find more information here](in-depth-rust-macros).
Now we can use our `print!` and `println!` macros in `interrupts.rs`. If you'd like to know more about the ins and outs of macros and how they differ from functions [you can find more information here][in-depth-rust-macros].

Fix markdown link syntax (with parantheses it links to ./in-depth-rust-macros).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

@@ -403,7 +442,7 @@ static BREAKPOINT_HANDLER_CALLED: AtomicUsize = AtomicUsize::new(0);
#[cfg(not(test))]
#[no_mangle]
pub extern "C" fn _start() -> ! {
init_idt();
blog_os::interrupts::init_idt();
Copy link
Owner

Choose a reason for hiding this comment

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

This isn't correct. The test works by creating an own IDT that references the new breakpoint_handler function. See https://github.com/phil-opp/blog_os/blob/master/src/bin/test-exception-breakpoint.rs.

The problem is that we don't show the full file because it would be too long. But I think we need to rework which parts we show and which we don't.

Copy link
Owner

Choose a reason for hiding this comment

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

I opened https://github.com/acheronfail/blog_os/pull/3 against your branch to fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome! I think renaming it to init_test_idt() was a good idea. 🔥 💯

@phil-opp
Copy link
Owner

Awesome, thanks a lot for the quick updates!

@phil-opp phil-opp added the relnotes "Release notes" – Notable changes that are rendered on the blog. label Oct 18, 2018
@phil-opp phil-opp changed the title refactor exception code into interrupts.rs Refactor exception code into new interrupts module Oct 18, 2018
@phil-opp phil-opp merged commit a1dd6b2 into phil-opp:master Oct 18, 2018
@acheronfail acheronfail deleted the refactor-interrupts branch October 18, 2018 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes "Release notes" – Notable changes that are rendered on the blog.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants