misc: miscellaneous improvements (#60)

* update GH workflows and commands

* update documentation

* cleanup code

- resolved many TODOs
- moved linker script
- added timeout when running tests

* bump version

.github/ISSUE_TEMPLATE/bug_report.yml

@@ -15,7 +15,7 @@ body:
       description: |
         By submitting this issue, you agree to our [Code of Conduct](https://georglauterbach.github.io/uncore/community/code_of_conduct/).
       options:
-        - label: I tried searching for an existing issue and followed the debugging docs (TODO) advice, but still need assistance.
+        - label: I tried searching for existing issues and followed the debugging documentation advice, but still need assistance.
           required: true
   - type: textarea
     id: what-happened

.github/workflows/code_linting.yml

@@ -4,6 +4,8 @@ name: Code / Linting
 on: # yamllint disable-line rule:truthy
   workflow_dispatch:
   pull_request:
+  push:
+    branches: [master]
 
 jobs:
   linting:

.github/workflows/code_security.yml

@@ -23,7 +23,7 @@ jobs:
         uses: actions/checkout@v4
 
       - name: Install Rust
-        run: ../.github/scripts/install_rust.sh
+        run: ../misc/scripts/install_rust.sh
 
       - name: Install `cargo audit`
         run: cargo install cargo-audit

.github/workflows/code_tests_and_checks.yml

@@ -25,7 +25,7 @@ jobs:
         uses: actions/checkout@v4
 
       - name: Install Rust
-        run: ../.github/scripts/install_rust.sh
+        run: ../misc/scripts/install_rust.sh
 
       - name: Run all checks
         run: >-
@@ -34,7 +34,6 @@ jobs:
   unit-and-integration-tests:
     name: Run unit- and integration-tests
     runs-on: ubuntu-22.04
-    # needs: kernel-linting
     steps:
       - name: Checkout code
         uses: actions/checkout@v4
@@ -45,12 +44,12 @@ jobs:
           qemu-system-riscv64 --version
 
       - name: Install Rust
-        run: ../.github/scripts/install_rust.sh
+        run: ../misc/scripts/install_rust.sh
 
       - name: Run all unit-test
         run: >-
           cargo run -- -vv u-test
 
       - name: Run all integration-tests
         run: >-
-          cargo run -- -vv u-test
+          cargo run -- -vv i-test

README.md

@@ -40,4 +40,3 @@ docker run --rm -it -v ./documentation:/docs -p 8080:8080 docker.io/squidfunk/mk
 [www::docs::rustdoc]: https://doc.rust-lang.org/rustdoc/what-is-rustdoc.html
 [docs::main-landing-page]: https://georglauterbach.github.io/uncore/
 [docs::github-pages]: https://pages.github.com/
-[docs::getting-started]: https://georglauterbach.github.io/uncore/#getting-started

code/Cargo.toml

@@ -69,6 +69,7 @@ colored = "2.0.4"
 log = "0.4.20"
 regex = "1.10.2"
 toml = "0.8.4"
+wait-timeout = "0.2.0"
 which = "5.0.0"
 
 # -----------------------------------------------

code/rust-toolchain.toml

@@ -11,6 +11,6 @@
 # General information about the keys below can be found under
 # https://rust-lang.github.io/rustup/concepts/index.html
 [toolchain]
-channel = "nightly-2023-10-17"
-components = [ "cargo", "llvm-tools", "rustc", "rustfmt", "rust-std", "rust-src", "clippy" ]
+channel = "nightly-2023-11-10"
+components = [ "cargo", "rustc", 'rust-std', "clippy", "rustfmt" ]
 targets = ["riscv64gc-unknown-none-elf"]

code/src/arguments.rs

@@ -67,7 +67,7 @@ pub struct ArchitectureSpecification {
   pub linker_script_path: String,
   /// The parameters of the QEMU command to execute
   qemu_arguments:         Vec<&'static str>,
-  /// TODO
+  /// Default path to the kernel binary when `main.rs` is used (i.e. no tests are run)
   kernel_binary_path:     String,
 }
 
@@ -95,7 +95,7 @@ impl ArchitectureSpecification {
     Self {
       target:             "riscv64gc-unknown-none-elf",
       qemu_command:       "qemu-system-riscv64",
-      linker_script_path: Self::append_to_base_dir("/uncore/src/library/arch/risc_v/ld/new.ld"),
+      linker_script_path: Self::append_to_base_dir("/uncore/src/library/arch/risc_v/linking.ld"),
       qemu_arguments:     vec![
         "-machine",
         "virt",

code/src/command.rs

@@ -125,15 +125,60 @@ macro_rules! run_command_and_check {
   };
 
   ($command_name:expr, $arguments:expr, $envs:expr) => {{
-    let __special: anyhow::Result<()> = if std::process::Command::new($command_name)
+    let __status = std::process::Command::new($command_name)
       .args($arguments)
       .envs($envs)
-      .status()?
-      .success()
-    {
+      .status()?;
+    let __special: anyhow::Result<()> = if __status.success() {
       Ok(())
     } else {
-      anyhow::bail!("Failure: could not determine exit status - terminated by signal?")
+      let message = if let Some(code) = __status.code() {
+        format!("Failure: command exited with exit code {}", code)
+      } else {
+        format!("Failure: could not determine exit status - terminated by signal?")
+      };
+
+      anyhow::bail!("{}", message);
+    };
+    __special
+  }};
+}
+
+/// Run a given command, taking arguments and environment variables if necessary, and
+/// evaluates the exit status in the end, while also checking for a given timeout, so that
+/// the command does not run forever.
+macro_rules! run_command_and_check_with_timeout {
+  ($command_name:expr, $arguments:expr, $timeout:expr) => {
+    run_command_and_check_with_timeout!($command_name, $arguments, [("__UNUSED", "")], $timeout)
+  };
+
+  ($command_name:expr, $arguments:expr, $envs:expr, $timeout_in_secs:expr) => {{
+    use wait_timeout::ChildExt;
+
+    let mut __child = std::process::Command::new($command_name)
+      .args($arguments)
+      .envs($envs)
+      .spawn()?;
+
+    let __timeout = std::time::Duration::from_secs($timeout_in_secs);
+    let __status = match __child.wait_timeout(__timeout)? {
+      Some(status) => status.code(),
+      None => {
+        // Child has not exited yet
+        __child.kill()?;
+        __child.wait()?;
+        anyhow::bail!("Failure: last command timed out");
+      },
+    };
+
+    let __special: anyhow::Result<()> = if let Some(code) = __status {
+      if code == 0 {
+        Ok(())
+      } else {
+        anyhow::bail!("Failure: command exited with exit code {}", code);
+      }
+    } else {
+      anyhow::bail!("Failure: last command terminated by signal?");
     };
     __special
   }};
@@ -250,18 +295,23 @@ where
   )
 }
 
-/// TODO
+/// When we parse Cargo's output (when `--message-format=json`) with `jq`, we get the
+/// paths on the file system to the test binaries. This function takes such a path and
+/// returns the test's name. This is useful for logging but also required when dynamically
+/// creating a GDB init script (see [`create_gdb_init_file`]).
 fn parse_binary_name(binary_name: &str) -> anyhow::Result<String> {
   let regex = regex::Regex::new(r".*/(.+)-.+")?;
   regex
     .captures(binary_name)
     .map_or_else(|| Ok(String::from("unknown")), |name| Ok(name[1].to_string()))
 }
 
-/// TODO
+/// A GDB initialization file is useful when debugging with GDB. Such a file provides GDB
+/// with symbol information, architecture, breakpoints, definition, etc. We need to
+/// dynamically create it because test binary names are different for each test.
 fn create_gdb_init_file(binary_location: &String, test_name: &String) -> anyhow::Result<()> {
   use std::io::Write;
-  /// TODO
+  /// A constant location in `/tmp` for storing temporary GDB initialization files.
   const FILE_LOCATION: &str = "/tmp/init.gdb";
   let mut w = std::fs::File::create(FILE_LOCATION)?;
   writeln!(
@@ -305,22 +355,22 @@ fn run_unit_tests(
   is_debug: bool,
 ) -> anyhow::Result<()> {
   log::info!("Building unit test binary");
-  let test_binaries = create_test_binaries(arch_specification, ["--lib"])?;
-  let test_binary = test_binaries.get(0);
-  if let Some(binary) = test_binary {
+  let test_binary_path = create_test_binaries(arch_specification, ["--lib"])?;
+  let test_binary_path = test_binary_path.first();
+  if let Some(binary_path) = test_binary_path {
     let mut qemu_arguments = arch_specification.qemu_arguments();
-    qemu_arguments.append(&mut vec!["-kernel", binary]);
+    qemu_arguments.append(&mut vec!["-kernel", binary_path]);
 
     if is_debug {
       log::info!("Debugging unCORE unit tests");
       qemu_arguments.append(&mut vec!["-s", "-S"]);
-      create_gdb_init_file(&parse_binary_name(binary)?, binary)?;
+      create_gdb_init_file(&parse_binary_name(binary_path)?, binary_path)?;
     } else {
       log::info!("Running unCORE unit tests");
-      log::trace!("The unit test binary file is '{}'", binary);
+      log::trace!("The unit test binary file is '{}'", binary_path);
     }
 
-    run_command_and_check!(arch_specification.qemu_command, qemu_arguments)?;
+    run_command_and_check_with_timeout!(arch_specification.qemu_command, qemu_arguments, 1)?;
   } else {
     // This part is unreachable because [`test_helper`] does already check whether the vector
     // contains at least one element and returns an error otherwise; which is caught by the
@@ -342,7 +392,7 @@ fn run_integration_tests(
 ) -> anyhow::Result<()> {
   log::info!("Building integration test binaries");
   let mut qemu_arguments = arch_specification.qemu_arguments();
-  let test_binaries = if let Some(test) = test {
+  let test_binary_paths = if let Some(test) = test {
     // If a test name is supplied, we may debug it
     if is_debug {
       qemu_arguments.append(&mut vec!["-s", "-S"]);
@@ -353,7 +403,7 @@ fn run_integration_tests(
   };
 
   // Run every test in the list of integration tests
-  for binary in test_binaries {
+  for binary in test_binary_paths {
     let test_name = parse_binary_name(&binary)?;
     if is_debug {
       log::info!("Debugging integration test '{}'", test_name);
@@ -364,7 +414,7 @@ fn run_integration_tests(
     log::trace!("The integration test binary file is '{}'", binary);
     let mut current_arguments = qemu_arguments.clone();
     current_arguments.append(&mut vec!["-kernel", binary.as_str()]);
-    run_command_and_check!(arch_specification.qemu_command, current_arguments)?;
+    run_command_and_check_with_timeout!(arch_specification.qemu_command, current_arguments, 60)?;
     log::info!("Integration test '{}' finished successfully", test_name);
   }
 
@@ -383,7 +433,7 @@ fn check(arch_specification: &arguments::ArchitectureSpecification) -> anyhow::R
   /// A simple wrapper around [`run_command_and_check`] to ease calling checks.
   macro_rules! check {
     ($arguments:expr) => {{
-      results.push(run_command_and_check!(env!("CARGO"), $arguments));
+      results.push(run_command_and_check_with_timeout!(env!("CARGO"), $arguments, 60));
     }};
   }
 

code/src/environment.rs

@@ -2,7 +2,8 @@
 
 //! This module provides functions to work efficiently with environment variables.
 
-/// TODO
+/// This constant stores the path of the directory containing the workspace's root
+/// `Cargo.toml`.
 const CARGO_MANIFEST_DIR: &str = env!("CARGO_MANIFEST_DIR");
 
 /// Returns the version of `rustc` used for compiling `unCORE`.
@@ -47,7 +48,7 @@ fn get_toolchain() -> anyhow::Result<String> {
       .expect("Could not get array 'targets' in table 'package' from rust-toolchain.toml")
       .as_array()
       .expect("Could not convert array 'targets' in table 'package' to proper array")
-      .get(0)
+      .first()
       .expect("Could not get first element of toolchain array")
       .to_string(),
   )

code/uncore/Cargo.toml

@@ -2,7 +2,7 @@ cargo-features = ["per-package-target"]
 
 [package]
 name = "uncore"
-version = "1.0.0-alpha1"
+version = "1.0.0-alpha2"
 edition = "2021"
 workspace = "../"
 
@@ -37,7 +37,8 @@ categories = [
   "config",
 ]
 
-# TODO
+# Setting this is useful (but requires an entry in `cargo-features`)
+# because tools like rust-analyzer do not need extra setup this way.
 default-target = "riscv64gc-unknown-none-elf"
 
 # Cargo's auto-detection of library files is turned on. Therefore,
@@ -59,7 +60,6 @@ autotests = false
 [dependencies]
 log = "0.4.20"
 owo-colors = "3.5.0"
-panic-halt = "0.2.0"
 riscv-rt = { git = "https://github.com/rust-embedded/riscv-rt.git", rev = "28b916d", features = ["s-mode"] }
 sbi = "0.2.0"
 spin = "0.9.8"

code/uncore/src/lib.rs

@@ -49,7 +49,7 @@
 // Use custom test runners. Since we cannot use the standard
 // library, we have to use our own test framework.
 #![feature(custom_test_frameworks)]
-// TODO
+// Allows reading the message of a call to `panic!()`.
 #![feature(panic_info_message)]
 
 // ? MODULES and GLOBAL / CRATE-LEVEL FUNCTIONS
@@ -65,24 +65,16 @@
 /// is important, and we are not allowed to mix them up.
 mod library;
 
-/// TODO
+/// Public re-exports that ought to be used by `main.rs`, by integration tests, or inside
+/// the kernel with `crate::`.
 pub use library::{
   arch,
   test,
-  Condition,
+  prelude::*,
 };
 
-/// TODO
-#[cfg(all(target_arch = "riscv64", test))]
-#[riscv_rt::entry]
-fn riscv64_entry() -> ! {
-  crate::arch::initialize();
-  setup_kernel();
-  crate::__test_runner();
-  arch::exit_kernel(library::Condition::Success);
-}
-
-/// TODO
+/// This function can be described as the kernel's "main" function. It usually runs after
+/// architecture-specific setup functions have run.
 pub fn setup_kernel() {
   library::log::initialize();
   library::log::display_initial_information();

code/uncore/src/library/arch/mod.rs

@@ -1,13 +1,19 @@
 // SPDX-License-Identifier: GPL-3.0-or-later
 
-/// TODO
-// #[cfg(target_arch = "riscv64")]
-mod risc_v;
-// #[cfg(target_arch = "riscv64")]
-use risc_v as architecture;
+//! This module contains all architecture-specific functionality. The trick to having a
+//! uniform interface is to use `pub use architecture::{stuff, to, re-export}` where
+//! `architecture` is a local re-export of a specific architecture (.e.g, `use risc_v as
+//! architecture`) guarded by conditional compilation (e.g., `#[cfg(target_arch =
+//! "riscv64`) so that only one architecture is enabled at any given time.
 
+/// Re-exported intra-kernel API that any implementation of an architecture must satisfy.
 pub use architecture::{
   drivers,
+  heap,
   exit_kernel,
   initialize,
 };
+
+#[cfg(target_arch = "riscv64")]
+mod risc_v;
+#[cfg(target_arch = "riscv64")] use risc_v as architecture;