Skip to content
This repository was archived by the owner on Jun 26, 2020. It is now read-only.

Print return values of run tests if type is not boolean. #1231

Closed
krk wants to merge 23 commits into from
Closed

Print return values of run tests if type is not boolean. #1231

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
18afb26
Add print and assertion options to the run tests.
krk Nov 14, 2019
b5cc84a
Revert verifier errors format string.
krk Nov 19, 2019
892a6b0
Implement FromStr for Operator.
krk Nov 21, 2019
5e78a80
Add function name to errors.
krk Nov 21, 2019
0e1b3de
Remove some return keywords.
krk Nov 21, 2019
ba49813
Refactor print_only parameter of check into a new print function.
krk Nov 21, 2019
62084b5
Add Value enum to represent parsed values in function_runner.
krk Nov 21, 2019
3405df7
Comment test failing in Windows, issue at #1279.
krk Dec 6, 2019
e7d02ea
Fix commented run command in test.
krk Dec 7, 2019
51c2e53
Substitute single line macro "consume".
krk Dec 15, 2019
0c9942c
Remove boolean_to_vec function and simplify its only callsite.
krk Dec 15, 2019
3a57022
Use ConstantData less in parsing test options.
krk Dec 15, 2019
6875284
Remove unnecessary type condition in FunctionRunner.invoke.
krk Dec 15, 2019
b076b68
Do not compile method that cannot be run in test.
krk Dec 15, 2019
02d887c
Fix return value count error message.
krk Dec 15, 2019
350c755
Simplify return type check for two I64s.
krk Dec 15, 2019
d772f3e
Remove unnecessary trivial_numeric_casts attribute.
krk Dec 18, 2019
7dc5f43
Merge use declarations.
krk Dec 18, 2019
7ec2f56
Remove overflowing_shl.
krk Dec 18, 2019
059ffcc
Mark invoke function as unsafe.
krk Dec 18, 2019
32d94ec
Remove unnecessary return.
krk Dec 18, 2019
bd443f8
Add descriptive panic message.
krk Dec 18, 2019
0be5b8d
Reword error message.
krk Dec 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 71 additions & 0 deletions cranelift-codegen/src/ir/constant.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,21 @@
//! - ensuring alignment of constants within the pool,
//! - bucketing constants by size.

#![allow(trivial_numeric_casts)]

use crate::ir::immediates::{IntoBytes, V128Imm};
use crate::ir::Constant;
use crate::HashMap;
use alloc::collections::BTreeMap;
use alloc::string::String;
use alloc::vec::Vec;
use core::fmt;
use core::iter::FromIterator;
use core::mem;
use core::slice::Iter;
use core::str::{from_utf8, FromStr};
use cranelift_entity::EntityRef;
use std::convert::TryInto;

/// This type describes the actual constant data. Note that the bytes stored in this structure are
/// expected to be in little-endian order; this is due to ease-of-use when interacting with
Expand Down Expand Up @@ -52,6 +57,72 @@ impl From<V128Imm> for ConstantData {
}
}

macro_rules! try_into_int {
($type: ty) => {
impl TryInto<$type> for ConstantData {
type Error = String;

fn try_into(self) -> Result<$type, Self::Error> {
if self.len() == (mem::size_of::<$type>()) {
let v = self.into_vec().into_iter().rev();
let mut r: $type = 0;
for b in v {
let (mut shifted, _) = r.overflowing_shl(8);
shifted |= b as $type;
r = shifted;
}
Ok(r)
} else {
Err(format!(
"Incorrect vector size: {}, expected {}",
self.len(),
mem::size_of::<$type>()
))
}
}
}
};
}

try_into_int!(u8);
try_into_int!(u16);
try_into_int!(u32);
try_into_int!(u64);
try_into_int!(u128);

impl TryInto<f32> for ConstantData {
type Error = String;

fn try_into(self) -> Result<f32, Self::Error> {
let bits = self.try_into();
match bits {
Ok(v) => Ok(f32::from_bits(v)),
Err(e) => Err(e),
}
}
}

impl TryInto<f64> for ConstantData {
type Error = String;

fn try_into(self) -> Result<f64, Self::Error> {
let bits = self.try_into();
match bits {
Ok(v) => Ok(f64::from_bits(v)),
Err(e) => Err(e),
}
}
}

impl TryInto<bool> for ConstantData {
type Error = String;

fn try_into(self) -> Result<bool, Self::Error> {
// Only check the least significant byte regardless of size.
Ok(self.into_vec()[0] != 0)
}
}

@abrown abrown Nov 20, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we should modify ConstantData for what we are doing; it is designed for constant pool stuff and adding these conversions might make it harder to decouple later from test functionality. Why not build our own Value enum:

enum Value {
  Bool(bool),
  F32(f32),
  F64(f64),
  ... 
}

I wouldn't worry too much about 128-bit types just yet until we get the basics in place. Then I would add a way to parse into this (using Rust's FromStr/parse) from text and a type:

impl Value {
  fn parse_as(text: &str, type: Type) -> Result<Self, ...> {
    match type {
      B8 | B16 | ... => Ok(Value::Bool(text.parse()?)),
      F32 => Ok(Value::F32(text.parse()?)),
      ...
}

Then I would #[derive(Eq, PartialEq)] on the enum so we can compare them to each other and add some From implementations so that invoke can get the raw value (e.g. f32), wrap it in a Value with Value::from (or into), and then compare it against the parsed Values that we get from the comments. (Hopefully that explains the other comments I've made; this isn't the only way to do things but it doesn't involve touching other components like ConstantData and it seems natural to Rust--from the little I know about Rust).

@abrown abrown Nov 20, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let me change my tune a bit: I now see down below that you implemented some parsing logic in parser.rs; if you want to use that for the FromStr on Value instead of the built-in Rust parsing that seems OK. We could use methods like Parser::new(text).match_bool(...) instead and Value may have to use different inner types (e.g. F32(Ieee32) instead of F32(f32))--but that seems fine. The underlying thing that I'm getting at is that we likely don't want to touch ConstantData in this case.

impl ConstantData {
/// Return the number of bytes in the constant.
pub fn len(&self) -> usize {
Expand Down
2 changes: 1 addition & 1 deletion cranelift-codegen/src/result.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ pub enum CodegenError {
///
/// This always represents a bug, either in the code that generated IR for Cranelift, or a bug
/// in Cranelift itself.
#[error("Verifier errors")]
#[error("Verifier errors\n{0}")]

@abrown abrown Nov 19, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we want to do this (I just learned): #1240

Verifier(#[from] VerifierErrors),

/// An implementation limit was exceeded.
Expand Down
Loading