-
Notifications
You must be signed in to change notification settings - Fork 104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Increase width
's width for trace values
#2172
Changes from 6 commits
3dde52c
3a144db
4afed4b
5a9ddff
b90a275
9e009af
1c84a54
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,6 +35,8 @@ use std::os::unix::process::ExitStatusExt; | |
use std::path::PathBuf; | ||
use std::process::{Child, ChildStdout}; | ||
|
||
const RESULT_ITEM_PREFIX: &str = " {\n \"result\":"; | ||
|
||
/// A parser item is a top-level unit of output from the CBMC json format. | ||
/// See the parser for more information on how they are processed. | ||
#[derive(Debug, Deserialize)] | ||
|
@@ -57,6 +59,17 @@ pub enum ParserItem { | |
}, | ||
} | ||
|
||
/// Struct that is equivalent to `ParserItem::Result`. | ||
/// | ||
/// Note: this struct is only used to provide better error messages when there | ||
/// are issues deserializing a `ParserItem::Result`. See `Parser::parse_item` | ||
/// for more details. | ||
#[allow(unused)] | ||
#[derive(Debug, Deserialize)] | ||
struct ResultStruct { | ||
result: Vec<Property>, | ||
} | ||
|
||
/// Struct that represents a single property in the set of CBMC results. | ||
/// | ||
/// Note: `reach` is not part of the parsed data, but it's useful to annotate | ||
|
@@ -285,7 +298,7 @@ pub struct TraceValue { | |
pub name: String, | ||
pub binary: Option<String>, | ||
pub data: Option<TraceData>, | ||
pub width: Option<u8>, | ||
pub width: Option<u32>, | ||
adpaco-aws marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
/// Enum that represents a trace data item. | ||
|
@@ -420,6 +433,25 @@ impl<'a, 'b> Parser<'a, 'b> { | |
if let Ok(item) = result_item { | ||
return item; | ||
} | ||
// If we failed to parse a `ParserItem::Result` earlier, we will get | ||
// this error message when we attempt to parse it using the complete | ||
// string: | ||
// ``` | ||
// thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: | ||
// Error("data did not match any variant of untagged enum ParserItem", line: 0, column: 0)' | ||
// ``` | ||
// This error message doesn't provide information about what went wrong | ||
// while parsing due to `ParserItem` being an untagged enum. A more | ||
// informative error message will be produced if we attempt to | ||
// deserialize it into a struct. The attempt will still fail, but it | ||
// shouldn't be hard to debug with that information. The same strategy | ||
// can be used for other `ParserItem` variants, but they're normally | ||
// easier to debug. | ||
if string_without_delimiter.starts_with(RESULT_ITEM_PREFIX) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you considered changing the signature of this function to Result and in the case of failure you can add the context that we failed on (i.e., what was the item that we were trying to parse). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't know which item we're trying to parse, that's why we use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made the change but then dropped it because the new case would return a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for explaining the issue offline. I'll resolve this comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I cannot get rid of |
||
let result_item: Result<ResultStruct, _> = | ||
serde_json::from_str(string_without_delimiter); | ||
result_item.unwrap(); | ||
} | ||
let complete_string = &self.input_so_far[0..self.input_so_far.len()]; | ||
let result_item: Result<ParserItem, _> = serde_json::from_str(complete_string); | ||
result_item.unwrap() | ||
|
@@ -681,4 +713,45 @@ mod tests { | |
serde_json::from_str(prop_id_string); | ||
let _prop_id = prop_id_result.unwrap(); | ||
} | ||
|
||
#[test] | ||
fn check_trace_value_deserialization_works() { | ||
let data = format!( | ||
r#"{{ | ||
"binary": "{:0>1000}", | ||
"data": "0", | ||
"name": "integer", | ||
"type": "unsigned __CPROVER_bitvector[960]", | ||
"width": 960 | ||
}}"#, | ||
0 | ||
); | ||
let trace_value: Result<TraceValue, _> = serde_json::from_str(&data); | ||
assert!(trace_value.is_ok()); | ||
} | ||
|
||
/// Checks that a valid CBMC "result" item can be deserialized into a | ||
/// `ParserItem` or `ResultStruct`. | ||
#[test] | ||
fn check_result_deserialization_works() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! |
||
let data = r#"{ | ||
"result": [ | ||
{ | ||
"description": "assertion failed: 1 > 2", | ||
"property": "long_function_name.assertion.1", | ||
"sourceLocation": { | ||
"column": "16", | ||
"file": "/home/ubuntu/file.rs", | ||
"function": "long_function_name", | ||
"line": "815" | ||
}, | ||
"status": "SUCCESS" | ||
} | ||
] | ||
}"#; | ||
let parser_item: Result<ParserItem, _> = serde_json::from_str(&data); | ||
let result_struct: Result<ResultStruct, _> = serde_json::from_str(&data); | ||
assert!(parser_item.is_ok()); | ||
assert!(result_struct.is_ok()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use regex instead or will this run all the time? I'm worried this is very fragile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were a few things I considered here, for example trimming the string without delimiters (so we can check against
"{\"result\":"
). The problem is that trimming that string could impact performance: the one that caused the crash in #2169 is 921K lines (33MB). So I went for the most performant check (this one), which should run all the time.