add set_general_handler macro #285

Freax13 · 2021-07-26T11:11:09Z

This pr adds the set_general_handler! macro to set a general handler in an idt using only macro_rules. It's an alternative to #168 and all of the tests are taken from it.

As already mentioned in #167 (comment) there are no loops in macro_rules macros, so I used recursion instead:
https://github.com/Freax13/x86_64/blob/8252697d6ef5b567ff6a8110656f9a50e00df051/src/structures/idt.rs#L936-L956

I believe this to be a relatively short and concise solution. The only major drawback with this approach is that we can't do the range checks a compile time, so the macro always has to expand to setting all 256 interrupts even if some of those will be skipped at runtime. The upside of this approach is that the range doesn't necessarily need to be known at compile time (set_general_handler! takes $range as an expression).

Closes #167

Freax13 · 2021-07-26T12:49:02Z

I'm not sure why ci is failing as I can't reproduce the error (running in a windows vm).

I think I managed to track down where the error is reported though:
Line 872 in https://llvm.org/doxygen/MCStreamer_8cpp_source.html

josephlr · 2021-07-27T09:16:01Z

I'm not sure why ci is failing as I can't reproduce the error (running in a windows vm).

If you change the generic handler function to be extern "C" does that fix the error?

josephlr

This is a cool idea, I'm wondering if we can make the macros less complex.

josephlr · 2021-07-27T09:22:16Z

src/structures/idt.rs

+    ($idt:expr, $handler:ident, $range:expr) => {{
+        fn set_general_handler(
+            idt: &mut $crate::structures::idt::InterruptDescriptorTable,
+            range: impl core::ops::RangeBounds<usize>,


Why not a range of u8? This removes the concerns about out of bounds indexing.

Currently InterruptDescriptorTable only uses usize for indexing operations and doesn't allow indexing with u8, so I tried to mirror that api. I agree that using u8 would make more sense. Perhaps we should also implement Index<u8> and IndexMut<u8> for InterruptDescriptorTable in a seperate pr.

The problem with implementing both Index<u8> and Index<usize> might be that type annotations are required in more cases. For example, idt[20] leads to the non-helpful error message "idt::InterruptDescriptorTable cannot be indexed by i32" because integer literals default to i32 in these cases.

However, I'm happy to change the Index impl from usize to u8 in the next breaking release. We should do this separately though, since this PR is a non-breaking change otherwise. We can of course already use u8 for the macro if we want.

src/structures/idt.rs

josephlr · 2021-07-27T09:27:50Z

src/structures/idt.rs

+/// use x86_64::structures::idt::{InterruptDescriptorTable, InterruptStackFrame};
+///
+/// let mut idt = InterruptDescriptorTable::new();
+/// fn my_general_handler(


If we are going to impose such a constraint, we should define the relevant function pointer type, and have the macro check that the function passed is actually of the correct type.

I agree, I'll add that next

src/structures/idt.rs

Freax13 · 2021-07-27T11:38:40Z

I'm not sure why ci is failing as I can't reproduce the error (running in a windows vm).

If you change the generic handler function to be extern "C" does that fix the error?

No, it's still failing with the same error

phil-opp

Sorry for the long delay since the last review iteration. This adds quite a bit of macro trickery, which makes it less readable than #168, but keeping the proc macro dependencies out is worth it in my opinion. The only concern I had are the error messages, but they don't seem much worse than the proc macro errors.

Was https://github.com/rust-osdev/x86_64/pull/285/files#r677279319 resolved? It doesn't seem to point to the right part of the source code anymore (or I don't understand what it is about).

From my side this is ready to be merged. Thanks a lot for your work!

Freax13 · 2021-11-07T12:47:18Z

I finally figured out how to do a proper rebase from next to master 🥳:

git rebase --onto upstream/master upstream/next

Let's hope that in the future I will know ahead of time whether something is breaking the api and never have to use this git command again.

Freax13 · 2021-11-07T12:59:05Z

The only concern I had are the error messages, but they don't seem much worse than the proc macro errors.

8695b39 makes the errors pretty well behaved. For example adding an invalid parameter to a general handlers only results in this single error message:

error[E0308]: mismatched types
  --> src/structures/idt.rs:1144:1
   |
19 | set_general_handler!(&mut idt, my_general_handler, 14);
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ incorrect number of function parameters
   |
   = note: expected fn pointer `fn(InterruptStackFrame, u8, Option<u64>)`
                 found fn item `fn(InterruptStackFrame, u8, Option<u64>, u16) {my_general_handler}`
   = note: this error originates in the macro `$crate::set_general_handler` (in Nightly builds, run with -Z macro-backtrace for more info)

Was https://github.com/rust-osdev/x86_64/pull/285/files#r677279319 resolved? It doesn't seem to point to the right part of the source code anymore (or I don't understand what it is about).

I completely forgot about that thanks for reminding me! It's now fixed.

Freax13 · 2021-11-07T13:17:27Z

I'm not sure why ci is failing as I can't reproduce the error (running in a windows vm).

If you change the generic handler function to be extern "C" does that fix the error?

No, it's still failing with the same error

This time the compiler on windows just straight up segfaulted: https://github.com/rust-osdev/x86_64/runs/4130900835?check_suite_focus=true#step:16:23

phil-opp · 2021-11-07T15:59:10Z

Oh wow, sounds like a bug in rustc. I'll try to look into it.

phil-opp · 2021-11-07T16:42:18Z

Ok, I think this is a bug in the x86-interrupt calling convention in LLVM. I'm not sure where the error occurs exactly, but my guess is at https://github.com/llvm/llvm-project/blob/e356027016c6365b3d8924f54c33e2c63d931492/llvm/lib/MC/MCStreamer.cpp#L874. The "error: offset is not a multiple of 16" message is interpreted as a warning by rustc in the CI run you linked, that's why I didn't notice it at first.

Since the x86-interrupt calling convention isn't typically used on Windows-based systems (or systems with an underlying OS in general), it's probably not worth the effort to try to fix this in LLVM. So let's just annotate the default_handlers test with a #[cfg(not(windows))] and add a comment that explains why (including a link to this thread/comment).

phil-opp · 2021-11-07T18:41:35Z

Thanks again for all your work on this!

josephlr reviewed Jul 27, 2021

View reviewed changes

Freax13 force-pushed the default-irq-handler branch from c0efffd to 8252697 Compare July 27, 2021 11:37

phil-opp added the waiting-for-review Waiting for a review from the maintainers. label Nov 6, 2021

phil-opp approved these changes Nov 6, 2021

View reviewed changes

phil-opp added waiting-on-author Waiting for the author to act on review feedback. and removed waiting-for-review Waiting for a review from the maintainers. labels Nov 6, 2021

Freax13 and others added 3 commits November 7, 2021 13:40

add set_general_handler macro

f9b134e

add GeneralHandlerFunc for set_general_handler

8695b39

use u8 instead of usize

d41756a

Freax13 force-pushed the default-irq-handler branch from d654ade to d41756a Compare November 7, 2021 12:42

Freax13 changed the base branch from next to master November 7, 2021 12:42

fix tests

43233b6

Tom Dohrmann added 3 commits November 7, 2021 14:03

put set_general_handler behind feature gate

6ba1dd6

support vmm_communication_exception

be14601

fix clippy warning in test

e633293

Tom Dohrmann added 2 commits November 7, 2021 18:11

don't run default_handlers test on windows

9b0065d

skip set_general_handler in doctests on windows

e447d88

phil-opp merged commit f4c2e26 into rust-osdev:master Nov 7, 2021

Freax13 deleted the default-irq-handler branch November 7, 2021 18:43

phil-opp mentioned this pull request Nov 7, 2021

Support setting a default handler in the IDT through a macro for automatic generation of wrapper functions #168

Closed

Freax13 mentioned this pull request Nov 7, 2021

implement Index<u8> for IDT instead of Index<usize> #319

Merged

phil-opp mentioned this pull request Dec 18, 2021

Release version 0.14.7 #331

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add set_general_handler macro #285

add set_general_handler macro #285

Freax13 commented Jul 26, 2021

Freax13 commented Jul 26, 2021

josephlr commented Jul 27, 2021

josephlr left a comment

josephlr Jul 27, 2021

Freax13 Jul 27, 2021

phil-opp Nov 6, 2021

josephlr Jul 27, 2021

Freax13 Jul 27, 2021

Freax13 commented Jul 27, 2021

phil-opp left a comment

Freax13 commented Nov 7, 2021

Freax13 commented Nov 7, 2021

Freax13 commented Nov 7, 2021

phil-opp commented Nov 7, 2021

phil-opp commented Nov 7, 2021

phil-opp commented Nov 7, 2021

add set_general_handler macro #285

add set_general_handler macro #285

Conversation

Freax13 commented Jul 26, 2021

Freax13 commented Jul 26, 2021

josephlr commented Jul 27, 2021

josephlr left a comment

Choose a reason for hiding this comment

josephlr Jul 27, 2021

Choose a reason for hiding this comment

Freax13 Jul 27, 2021

Choose a reason for hiding this comment

phil-opp Nov 6, 2021

Choose a reason for hiding this comment

josephlr Jul 27, 2021

Choose a reason for hiding this comment

Freax13 Jul 27, 2021

Choose a reason for hiding this comment

Freax13 commented Jul 27, 2021

phil-opp left a comment

Choose a reason for hiding this comment

Freax13 commented Nov 7, 2021

Freax13 commented Nov 7, 2021

Freax13 commented Nov 7, 2021

phil-opp commented Nov 7, 2021

phil-opp commented Nov 7, 2021

phil-opp commented Nov 7, 2021