phil-opp / blog_os

Writing an OS in Rust
http://os.phil-opp.com
Apache License 2.0
14.26k stars 1.01k forks source link

Comments for "https://os.phil-opp.com/integration-tests/" #443

Closed utterances-bot closed 3 years ago

utterances-bot commented 6 years ago

This is a general purpose comment thread for the “Integration Tests” post.

skierpage commented 6 years ago

Great stuff! Why is the QEMU exit device at I/O Port 0xf4? Is it just a spare port that's available in a typical x86 machine?

AleVul commented 6 years ago

In Additional Test Executables and Bootimage Test paragraphs the author mentions unit test, but judging by context, aren't those supposed be integration tests?

atedp commented 6 years ago

Fist off thanks for the great tutorial!

On my machine I seem to have to append --target x86_64-blog_os.json to all bootimage commands...not a huge deal, just curious if I am doing something obviously wrong.

without the target called out specifically I get: error: language item required, but not found: eh_personality

phil-opp commented 6 years ago

@AleVul Good catch, thanks! Fixed in 22470e7.

phil-opp commented 6 years ago

@atedp We configured bootimage to use the x86_64-blog_os.json target as default target here. Maybe you read that post before we introduced this paragraph. Without setting the default, bootimage tries to build for your host architecture, I think that's why the error occurs.

phil-opp commented 6 years ago

@skierpage Thanks!

Why is the QEMU exit device at I/O Port 0xf4? Is it just a spare port that's available in a typical x86 machine?

Yeah, I think so. Here is an incomplete list of the common port ranges.

atedp commented 6 years ago

@phil-opp Woops, I definitely missed that :)

Thanks!!

pbn4 commented 6 years ago

Hello very good tutorial. Thanks for sharing this. One question popped up though:

What is the purpose of using:

#![feature(const_fn)]

in src/main.rs?

skierpage commented 6 years ago

What is the purpose of using:

![feature(const_fn)]

in src/main.rs?

I don't see it in main.rs. It's in src/lib.rs, possibly a holdover from earlier version. https://os.phil-opp.com/vga-text-mode/#a-global-interface references const functions, but explains how it's using lazy_static instead.

phil-opp commented 5 years ago

possibly a holdover from earlier version.

Seems like it. Should be removed in 3365a4f. Thanks for reporting!

dodikk commented 5 years ago

@phil-opp , thank you for another great article.

Could you please explain the reason of choosing the "Additional Test Executables" approach over "Read the test function name from the serial port" (and dispatch it to actual Rust functions with match operator or some other means. Basically, that would be a simple interpreter over a serial port).

As far as I remember, UART allows a two-way communication... (and you've also mentioned that fact in the beginning of an article).

P.S. explaining how to build a library and a "hybrid project" clearly adds some education value to the article. Still, please consider highlighting the pros and cons of different test framework implementation approaches. So far It's a bit unclear for the beginners (like me) "what the best practice is".

P.P.S. I have created a dedicated issue https://github.com/phil-opp/blog_os/issues/451

nukeop commented 5 years ago

I followed the tutorial and found that lib.rs off also needs this piece of code at the top to make cargo test work:

#[cfg(test)]
extern crate std;

#[cfg(test)]
extern crate array_init;

Since this was previously in main.rs, the tests won't run without it. Or did I miss something?

Johanneslueke commented 5 years ago

Hello again @phil-opp :D,

working again through your tutorial. I encountered one minor problem. Which i believe is just my in proper use of the tools. Nonetheless i've got a question:

the qemu commandline option "-device isa-debug-exit,iobase=0xf4,iosize=0x04"

i cannot find any concrete information about this option. could you please tell from where you got it?

mtn commented 5 years ago

@nukeop It's included in the example code for lib.rs:

#![no_std] // don't link the Rust standard library

...

#[cfg(test)]
extern crate array_init;
#[cfg(test)]
extern crate std;

...
phil-opp commented 5 years ago

@nukeop As @mtn said, it is part of the code listing in the “Split Off A Library” section.

phil-opp commented 5 years ago

@Johanneslueke It is mentioned in the “Shutdown” article of the OSDev wiki and this Stackoverflow answer. The official documentation is rather sparse unfortunately. You see it under "Misc devices" when executing qemu-system-x86_64 -device help and with qemu-system-x86_64 -device isa-debug-exit,help you see the two configuration options. Other than that, I only found the commit that added it.

ksqsf commented 5 years ago

We mark the function (exit_qemu) as unsafe because it relies on the fact that a special QEMU device is attached to the I/O port with address 0xf4.

I'm not exactly sure why this would be a reason to mark exit_qemu unsafe. I think if a program exits, there will be no memory safety problems. Could you please elaborate on this? Thanks!

phil-opp commented 5 years ago

@ksqsf The code relies on two conditions: (1) There is a device attached at port 0xf4 and (2) this device is the QEMU shutdown device. Both conditions might not hold in a real system. In a really bad theoretical case there could a different device at that address that interprets the sent command as "overwrite some memory region via DMA", which would break memory safety. So because we cannot prove that this function is safe in all cases, we mark it as unsafe and use it only in the integration test scenario where we know that the QEMU device exists at that address.

ghost commented 5 years ago

I am getting the following error :

error: format argument must be a string literal --> src/bin/test-panic.rs:22:5 22 serial_println!("ok"); ^^^^^^^^^^^^^^^^^^^^^^

= note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) help: you might be missing a string literal to format with | 2 | serial_print ! ( "{}", concat ( $ fmt , "\n" ) ) ) ; ( | ^^^^^

Though if is use serial_println!("{}","ok") instead of serial_println!("ok") it works fine

????

phil-opp commented 5 years ago

You might be missing a ! at the end of concat (i.e. it should be concat!(…) instead of concat(…)). The reason that it works with serial_println!("{}","ok") is that it uses a different case of the macro, which seems to use the correct concat!(…) syntax.

mdunnegan commented 5 years ago

I am having issue with your code in the "Split Off A Library" section, specifically the line:

use blog_os::println;

I am getting this error when I run cargo build

no println in the root

phil-opp commented 5 years ago

@mdunnegan Do you have the #[macro_export] attribute on your println macro? We recently updated the blog to the 2018 edition of Rust and changed the macro imports to normal use imports instead of #[macro_use] attributes.

In case you're using the older version in your code and don't want to update it, you can add a #[macro_use] extern crate blog_os instead of use blog_os::println.

krsoninikhil commented 5 years ago

Hi @phil-opp, thank you for this wonderful series. I'm learning about OS as well as Rust.

I've few questions/doubts if you could please help me with:

phil-opp commented 5 years ago

Hi @krsoninikhil,

  • Since integration tests don't have to be compiled while doing unit testing (cargo test), can these test-*.rs files have #![cfg(not(test))] as inner attribute? This would eliminate the need of same check for every function.

Yes, you could do that if you are sure that you'll never have unit tests in these files. The disadvantage of this approach is that any tests that you might add in the future will be silently ignored. A better solution in my opinion would be to change the behavior of cargo test so that no output is printed for binaries without unit tests. Such a quiet mode is discussed in https://github.com/rust-lang/rust/issues/38758.

  • Why did you not use snake_case for naming integration test files?

No real reason. Seperation with - seems more common for binaries in my experience, but snake case is fine too.

  • Can writing to serial port (0x3F8) and port I/O (0xf4) be done using same crate - uart_16550 or x86_64? Aren't both of them doing port-mapped I/O?

There is a difference between serial ports and I/O ports. I/O ports are the fundamental building block that can be used to read/write device registers. The serial port controller is such an device with multiple device registers.

If you look at the source code of the uart_16550 crate, you see that the SerialPort structs contains 6 device registers, which can be individually addressed using port I/O. For performing these port I/O operations, the uart_16550 crate uses the x86_64 crate behind the scenes.

I hope this answers your questions!

krsoninikhil commented 5 years ago

Thanks @phil-opp for clarifying, this certainly helps.

guidao commented 5 years ago

I am getting the following error when i run 'bootimage run' command: os: macos rust version: rustc 1.34.0-nightly (097c04cf4 2019-02-24)

blog_os git:(post-05)> bootimage run --bin test-basic-boot -- \ -serial mon:stdio -display none \ -device isa-debug-exit,iobase=0xf4,iosize=0x04 Building kernel Compiling core v0.0.0 (/Users/xxx/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/src/libcore) Finished release [optimized] target(s) in 26.20s error: failed to load source for a dependency on compiler_builtins

Caused by: Unable to update /Users/xxx/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/src/libcompiler_builtins

Caused by: failed to read /Users/xxx/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/src/libcompiler_builtins/Cargo.toml

Caused by: No such file or directory (os error 2) error: "cargo" "rustc" "-p" "compiler_builtins" "--release" "--manifest-path" "/var/folders/w3/25y_3fds4l989bp5mlb07m4w0000gn/T/xargo.0DAHd9Da08yd/Cargo.toml" "--target" "/Users/xxx/code/rust/blog_os/x86_64-blog_os.json" "--" "--sysroot" "/Users/xxx/code/rust/blog_os/target/sysroot" "-Z" "force-unstable-if-unmarked" failed with exit code: Some(101) note: run with RUST_BACKTRACE=1 for a backtrace

AntoineSebert commented 5 years ago

Caused by: failed to read /Users/xxx/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/src/libcompiler_builtins/Cargo.toml

It does seem to come from the compiler itself. Maybe the rust toolchain you're using is broken ? Have you tried another nightly build ? You can use rustup to manage your rust toolchains.

phil-opp commented 5 years ago

@guidao Are you using rustup for managing your toolchains? What's the output of rustc --print sysroot for you? Do the libcore and libcompiler_builtin directories exist in /Users/xxx/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/src/?

guidao commented 5 years ago

@AntoineSebert @phil-opp yes, i am using rustup to manage my toolchains.

rustc --print sysroot /Users/xxx/.rustup/toolchains/nightly-x86_64-apple-darwin ➜ src pwd /Users/xxx/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/src ➜ src ls build_helper libcore libpanic_unwind libprofiler_builtins librustc_lsan librustc_tsan libterm libunwind tools liballoc libpanic_abort libproc_macro librustc_asan librustc_msan libstd libtest stdsimd

but libcompiler_builtin is not found in this directory

phil-opp commented 5 years ago

@guidao Thanks! I think the problem is that your cargo-xbuild version is too old: the compiler builtins library was moved to crates.io recently so cargo xbuild shouldn't look for the compiler builtins source locally. Could you try cargo install cargo-xbuild --force to update it?

guidao commented 5 years ago

@phil-opp it's working. thanks.