Conversation
|
Submitted the PR to main branch by mistake, which is causing CI errors for push. |
danielsn
left a comment
There was a problem hiding this comment.
General comment: how do we protect against rustc making small changes to the string which would break this?
| /// This function extracts the "assertion failed" string which is created by the rust stdl. It | ||
| /// causes confusing outputs like "assertion failed: 1 == 1" SUCCESS to be printed out. This function | ||
| /// can extract the "assertion failed" component out and change the string to "assertion" |
There was a problem hiding this comment.
This comment needs to be more clearly written
| /// can extract the "assertion failed" component out and change the string to "assertion" | ||
| fn extract_panic_string(e: &Expr) -> Option<String> { | ||
| // The MIR represents the StringConstant as `&"constant"[0]`. We are representing a rust str type as a struct. | ||
| let arg: &str = match e.struct_expr_values().unwrap()[0].value() { |
There was a problem hiding this comment.
is there a clearer name than arg here?
| /// causes confusing outputs like "assertion failed: 1 == 1" SUCCESS to be printed out. This function | ||
| /// can extract the "assertion failed" component out and change the string to "assertion" | ||
| fn extract_panic_string(e: &Expr) -> Option<String> { | ||
| // The MIR represents the StringConstant as `&"constant"[0]`. We are representing a rust str type as a struct. |
There was a problem hiding this comment.
This is goto, not MIR at this point.
There was a problem hiding this comment.
I believe the issue is that by the time we get the string, its been lowered from a string constant to a char*. Maybe have a utility function to reverse that lowering, and then just use that here would be more self documeting
| _ => return None, | ||
| }; | ||
|
|
||
| // "assertion failed: 1 == 1: SUCCESS" was confusing, so we removed the "failed" |
There was a problem hiding this comment.
So far, have we always seen that prefix? If so, maybe we should just assert that we find it so we're more stable against rustc changes
There was a problem hiding this comment.
We're in codegen_panic so any panic! message might be here. It's just the assert! case we're trying to recognize and rewrite.
celinval
left a comment
There was a problem hiding this comment.
I'm really sorry for the late comment, but I don't think this is the right approach to this problem. Panic messages tend to state an error. Assertion is just one way out of tens if not hundreds panic messages that are generated by rustc. We can't just rewrite them all.
Here are a few examples:
[dummy.assertion.1] line 7 internal error: entered unreachable code: SUCCESS
[main.pointer_dereference.1] line 17 dereference failure: pointer NULL in *var_7: SUCCESS
[__builtin_powif.assertion.1] assertion false: SUCCESS
I think we should be polishing the CBMC output instead.
Description of changes:
This change fixes the confusing "assertion failed" output that appears in test cases, even in success. For example,
"[main.assertion.2] line 30 assertion failed: weird_string == "w": SUCCESS".
Resolved issues:
Resolves #189
Call-outs:
Testing:
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.