Merge #281
281: Run Miri with more flag combinations, and add a Miri Tips doc. r=alistair23 a=jrvanwhy
[Rendered doc](https://github.com/jrvanwhy/libtock-rs/blob/miri-flags/doc/MiriTips.md)
This runs code under Miri with the `-Zmiri-symbolic-alignment-check` flag, as well as both with and without `-Zmiri-track-raw-pointers`. This identifies a lot of "gray area" behavior -- that may or may not become specified as undefined behavior in the future -- as undefined behavior.
I also added a tip on how to write tests that identify undefined behavior that Miri doesn't catch by default, such as having uninitialized values in local variables. The documentation tip will be used in the `libtock_unittest` crate as well as per-crate unit tests.
Co-authored-by: Johnathan Van Why <jrvanwhy@google.com>
diff --git a/core/platform/src/command_return_tests.rs b/core/platform/src/command_return_tests.rs
index 439f9e7..85583ad 100644
--- a/core/platform/src/command_return_tests.rs
+++ b/core/platform/src/command_return_tests.rs
@@ -130,7 +130,7 @@
assert_eq!(command_return.get_failure_2_u32(), None);
assert_eq!(
command_return.get_failure_u64(),
- Some((ErrorCode::Busy, 0x00001003_00001002))
+ Some((ErrorCode::Busy, 0x0000_1003_0000_1002))
);
assert_eq!(command_return.get_success_u32(), None);
assert_eq!(command_return.get_success_2_u32(), None);
@@ -240,7 +240,10 @@
assert_eq!(command_return.get_failure_u64(), None);
assert_eq!(command_return.get_success_u32(), None);
assert_eq!(command_return.get_success_2_u32(), None);
- assert_eq!(command_return.get_success_u64(), Some(0x00001002_00001001));
+ assert_eq!(
+ command_return.get_success_u64(),
+ Some(0x0000_1002_0000_1001)
+ );
assert_eq!(command_return.get_success_3_u32(), None);
assert_eq!(command_return.get_success_u32_u64(), None);
assert_eq!(command_return.return_variant(), return_variant::SUCCESS_U64);
@@ -299,7 +302,7 @@
assert_eq!(command_return.get_success_3_u32(), None);
assert_eq!(
command_return.get_success_u32_u64(),
- Some((1001, 0x00001003_00001002))
+ Some((1001, 0x0000_1003_0000_1002))
);
assert_eq!(
command_return.return_variant(),
diff --git a/core/platform/src/lib.rs b/core/platform/src/lib.rs
index d57644b..d474f2b 100644
--- a/core/platform/src/lib.rs
+++ b/core/platform/src/lib.rs
@@ -1,4 +1,5 @@
#![no_std]
+#![warn(unsafe_op_in_unsafe_fn)]
mod async_traits;
mod command_return;
diff --git a/core/runtime/src/lib.rs b/core/runtime/src/lib.rs
index 7e2e1ca..f705c45 100644
--- a/core/runtime/src/lib.rs
+++ b/core/runtime/src/lib.rs
@@ -20,6 +20,7 @@
#![feature(asm)]
#![no_std]
+#![warn(unsafe_op_in_unsafe_fn)]
mod startup;
diff --git a/core/runtime/src/syscalls_impl_riscv.rs b/core/runtime/src/syscalls_impl_riscv.rs
index 3e635e8..0e5f58a 100644
--- a/core/runtime/src/syscalls_impl_riscv.rs
+++ b/core/runtime/src/syscalls_impl_riscv.rs
@@ -41,14 +41,16 @@
mut r3: usize,
class: u8,
) -> (u32, usize, usize, usize) {
- asm!("ecall",
- inlateout("a0") r0,
- inlateout("a1") r1,
- inlateout("a2") r2,
- inlateout("a3") r3,
- in("a4") class,
- options(preserves_flags, nostack),
- );
+ unsafe {
+ asm!("ecall",
+ inlateout("a0") r0,
+ inlateout("a1") r1,
+ inlateout("a2") r2,
+ inlateout("a3") r3,
+ in("a4") class,
+ options(preserves_flags, nostack),
+ );
+ }
(r0, r1 as usize, r2, r3)
}
diff --git a/core/src/entry_point/start_item_arm.rs b/core/src/entry_point/start_item_arm.rs
index 894088e..5d88d6b 100644
--- a/core/src/entry_point/start_item_arm.rs
+++ b/core/src/entry_point/start_item_arm.rs
@@ -1,18 +1,12 @@
-use core::hint;
-
/// Tock programs' entry point. Called by the kernel at program start. Sets up
/// the stack then calls rust_start() for the remainder of setup.
#[doc(hidden)]
#[no_mangle]
#[naked]
#[link_section = ".start"]
-pub unsafe extern "C" fn _start(
- app_start: usize,
- mem_start: usize,
- _memory_len: usize,
- app_heap_break: usize,
-) -> ! {
- llvm_asm!("
+pub unsafe extern "C" fn _start() -> ! {
+ asm!(
+ "
// Because ROPI-RWPI support in LLVM/rustc is incomplete, Rust
// applications must be statically linked. An offset between the
// location the program is linked at and its actual location in flash
@@ -102,12 +96,8 @@
mov r2, r8
// Call rust_start
- bl rust_start"
- : // No output operands
- : "{r0}"(app_start), "{r1}"(mem_start), "{r3}"(app_heap_break) // Input operands
- : "r0", "r1", "r2", "r3", "r4", "r5", "r6", "r8", "r12",
- "cc", "memory" // Clobbers
- : "volatile" // Options
- );
- hint::unreachable_unchecked()
+ bl rust_start",
+ // No clobbers because we don't return.
+ options(noreturn),
+ )
}
diff --git a/core/src/entry_point/start_item_riscv32.rs b/core/src/entry_point/start_item_riscv32.rs
index f8a0e19..ec63591 100644
--- a/core/src/entry_point/start_item_riscv32.rs
+++ b/core/src/entry_point/start_item_riscv32.rs
@@ -1,5 +1,3 @@
-use core::hint;
-
/// Tock programs' entry point. Called by the kernel at program start. Sets up
/// the stack then calls rust_start() for the remainder of setup.
#[doc(hidden)]
@@ -14,12 +12,12 @@
// Due to Rust issue: https://github.com/rust-lang/rust/issues/42779 we can't have
// args to the function
pub unsafe extern "C" fn _start() -> ! {
- llvm_asm!(
- // Compute the stack top.
- //
- // struct hdr* myhdr = (struct hdr*) app_start;
- // uint32_t stacktop = (((uint32_t) mem_start + myhdr->stack_size + 7) & 0xfffffff8);
- "lw t0, 36(a0) // t0 = myhdr->stack_size
+ asm!(
+ // Compute the stack top.
+ //
+ // struct hdr* myhdr = (struct hdr*) app_start;
+ // uint32_t stacktop = (((uint32_t) mem_start + myhdr->stack_size + 7) & 0xfffffff8);
+ "lw t0, 36(a0) // t0 = myhdr->stack_size
addi t0, t0, 7 // t0 = myhdr->stack_size + 7
add t0, t0, a1 // t0 = mem_start + myhdr->stack_size + 7
li t1, 7 // t1 = 7
@@ -89,14 +87,10 @@
mv s0, sp // Set the frame pointer to sp.
mv a1, s1 // second arg is stacktop
mv a2, t1 // third arg is app_heap_break that we told the kernel
- jal rust_start"
- : // No output operands
- :
- : "memory", "x10", "x11", "x12", "x13", "x14", "x15", "x16", "x17",
- "x5", "x6", "x7", "x28", "x29", "x30", "x31", "x1" // Clobbers
- : "volatile" // Options
- );
- hint::unreachable_unchecked();
+ jal rust_start",
+ // No clobbers needed for a noreturn asm! block.
+ options(noreturn),
+ )
}
/// Ensure an abort symbol exists.
diff --git a/core/src/lib.rs b/core/src/lib.rs
index 717dc41..6281d18 100644
--- a/core/src/lib.rs
+++ b/core/src/lib.rs
@@ -1,4 +1,4 @@
-#![feature(lang_items, llvm_asm, naked_functions)]
+#![feature(asm, lang_items, llvm_asm, naked_functions)]
#![cfg_attr(any(target_arch = "arm", target_arch = "riscv32"), no_std)]
#![cfg_attr(feature = "alloc", feature(alloc_error_handler))]
diff --git a/layout_generic.ld b/layout_generic.ld
index 0bbef87..3a77ad7 100644
--- a/layout_generic.ld
+++ b/layout_generic.ld
@@ -24,9 +24,15 @@
/* Section for just the app crt0 header.
* This must be first so that the app can find it.
*/
- .crt0_header :
+ /* Runtime setup logic. The crt0_header symbol is used by the entry point
+ * assembly. We have to include start here rather than .text because
+ * otherwise elf2tab fails to recognize that the process binary's flash
+ * region should start at the beginning of .start.
+ */
+ .start :
{
_beginning = .; /* Start of the app in flash. */
+ crt0_header = .;
/**
* Populate the header expected by `crt0`:
*
@@ -68,6 +74,8 @@
* between the header and subsequent .data section. It's unclear why,
* but LLD is aligning sections to a multiple of 32 bytes. */
. = ALIGN(32);
+
+ *(.start)
} > FLASH =0xFF
/* Text section, Code! */
@@ -75,7 +83,6 @@
{
. = ALIGN(4);
_text = .;
- KEEP (*(.start))
*(.text*)
*(.rodata*)
KEEP (*(.syscalls))
@@ -140,30 +147,10 @@
{
} > FLASH
- /* ARM Exception support
- *
- * This contains compiler-generated support for unwinding the stack,
- * consisting of key-value pairs of function addresses and information on
- * how to unwind stack frames.
- * https://wiki.linaro.org/KenWerner/Sandbox/libunwind?action=AttachFile&do=get&target=libunwind-LDS.pdf
- *
- * .ARM.exidx is sorted, so has to go in its own output section.
- *
- * __NOTE__: It's at the end because we currently don't actually serialize
- * it to the binary in elf2tbf. If it was before the RAM sections, it would
- * through off our calculations of the header.
- */
- PROVIDE_HIDDEN (__exidx_start = .);
- .ARM.exidx :
- {
- /* (C++) Index entries for section unwinding */
- *(.ARM.exidx* .gnu.linkonce.armexidx.*)
- } > FLASH
- PROVIDE_HIDDEN (__exidx_end = .);
-
+ /* Sections we do not need. */
/DISCARD/ :
{
- *(.eh_frame)
+ *(.ARM.exidx .eh_frame)
}
}
diff --git a/rust-toolchain b/rust-toolchain
index 0a379e7..1674405 100644
--- a/rust-toolchain
+++ b/rust-toolchain
@@ -1,7 +1,7 @@
[toolchain]
# See https://rust-lang.github.io/rustup-components-history/ for a list of
# recently nightlies and what components are available for them.
-channel = "nightly-2020-08-20"
+channel = "nightly-2021-03-25"
components = ["clippy", "miri", "rustfmt"]
targets = ["thumbv7em-none-eabi",
"riscv32imac-unknown-none-elf",
diff --git a/src/ble_composer.rs b/src/ble_composer.rs
index 589dfc3..d470378 100644
--- a/src/ble_composer.rs
+++ b/src/ble_composer.rs
@@ -15,7 +15,7 @@
}
impl BlePayload {
- pub fn add(&mut self, kind: u8, content: &[u8]) -> Result<(), ()> {
+ pub fn add(&mut self, kind: u8, content: &[u8]) -> Result<(), Overflow> {
self.check_can_write_num_bytes(content.len() + 2)?;
self.bytes[self.occupied] = (content.len() + 1) as u8;
@@ -26,7 +26,7 @@
Ok(())
}
- pub fn add_flag(&mut self, flag: u8) -> Result<(), ()> {
+ pub fn add_flag(&mut self, flag: u8) -> Result<(), Overflow> {
self.check_can_write_num_bytes(3)?;
self.bytes[self.occupied] = 2;
@@ -36,7 +36,7 @@
Ok(())
}
- pub fn add_service_payload(&mut self, uuid: [u8; 2], content: &[u8]) -> Result<(), ()> {
+ pub fn add_service_payload(&mut self, uuid: [u8; 2], content: &[u8]) -> Result<(), Overflow> {
self.check_can_write_num_bytes(4 + content.len())?;
self.bytes[self.occupied] = (content.len() + 3) as u8;
self.bytes[self.occupied + 1] = gap_types::SERVICE_DATA;
@@ -49,11 +49,11 @@
Ok(())
}
- fn check_can_write_num_bytes(&self, number: usize) -> Result<(), ()> {
+ fn check_can_write_num_bytes(&self, number: usize) -> Result<(), Overflow> {
if self.occupied + number <= self.bytes.len() {
Ok(())
} else {
- Err(())
+ Err(Overflow)
}
}
}
@@ -72,6 +72,10 @@
}
}
+// Error type returned when the buffer is too full to perform an operation.
+#[derive(Debug)]
+pub struct Overflow;
+
#[cfg(test)]
mod test {
use super::*;
diff --git a/src/debug/mod.rs b/src/debug/mod.rs
index 85b4337..68d32eb 100644
--- a/src/debug/mod.rs
+++ b/src/debug/mod.rs
@@ -28,7 +28,7 @@
write_as_hex(&mut buffer[12..22], *address);
for index in 0..4 {
let byte = *(address as *const u8).offset(index);
- let byte_is_printable_char = byte >= 0x20 && byte < 0x80;
+ let byte_is_printable_char = (0x20..0x80).contains(&byte);
if byte_is_printable_char {
buffer[23 + index as usize] = byte;
}
diff --git a/src/sensors/mod.rs b/src/sensors/mod.rs
index f3a0e7a..f9196d2 100644
--- a/src/sensors/mod.rs
+++ b/src/sensors/mod.rs
@@ -46,9 +46,9 @@
}
}
- impl Into<i32> for $type_name {
- fn into(self) -> i32 {
- self.value
+ impl From<$type_name> for i32 {
+ fn from(sensor: $type_name) -> i32 {
+ sensor.value
}
}
diff --git a/test_runner/src/main.rs b/test_runner/src/main.rs
index b21b945..5ab9c1a 100644
--- a/test_runner/src/main.rs
+++ b/test_runner/src/main.rs
@@ -53,8 +53,8 @@
}
fn test_succeeded(input: String, failed_tests: &mut Vec<String>) -> Option<bool> {
- let success = input.find("[ OK ]").is_some();
- let failure = input.find("[ FAILURE ]").is_some();
+ let success = input.contains("[ OK ]");
+ let failure = input.contains("[ FAILURE ]");
let input = input.replace("[ OK ]", "");
let input = input.replace("[ FAILURE ]", "");
let input = input.trim();
diff --git a/tools/print_sizes/src/main.rs b/tools/print_sizes/src/main.rs
index dcd7777..12f05c4 100644
--- a/tools/print_sizes/src/main.rs
+++ b/tools/print_sizes/src/main.rs
@@ -117,18 +117,21 @@
.expect("No examples found");
let section_width = 7;
- // TODO: We do not currently print out .rodata's size. Currently, the linker
- // script embeds .rodata in .text, so we don't see it as a separate section
- // here. We should modify the linker script to put .rodata in its own
- // section. Until that is done, .rodata's size will be counted as part of
- // .text, so we'll just print .text's size for now.
println!(
- "{0:1$} {2:3$} {4:>7$} {5:>7$} {6:>7$}",
- "Example", name_width, "Architecture", arch_width, ".bss", ".data", ".text", section_width
+ "{0:1$} {2:3$} {4:>8$} {5:>8$} {6:>8$} {7:>8$}",
+ "Example",
+ name_width,
+ "Architecture",
+ arch_width,
+ ".bss",
+ ".data",
+ ".text",
+ ".rodata",
+ section_width
);
for data in example_data {
println!(
- "{0:1$} {2:3$} {4:7$} {5:7$} {6:7$}",
+ "{0:1$} {2:3$} {4:8$} {5:8$} {6:8$} {7:8$}",
data.name,
name_width,
data.arch,
@@ -136,6 +139,7 @@
data.sizes.bss,
data.sizes.data,
data.sizes.text,
+ data.sizes.rodata,
section_width
);
}
