refactor: Add pass for converting exit and abort#1587
Conversation
randomPoison
changed the title
| match func_name { | ||
| "abort" => { | ||
| *e = mk().span(e.span).call_expr( | ||
| mk().path_expr(vec!["", "std", "process", "abort"]), |
There was a problem hiding this comment.
Could we go further and call panic!? It's not exactly equivalent when panic unwinds, but we might want that anyway.
panic! will even take a message, and show a backtrace.
There was a problem hiding this comment.
This could be enabled with a CLI arg maybe?
There was a problem hiding this comment.
Hm, I like this idea, panic! seems like a more idiomatic error exit than abort. But maybe we should do that as a separate pass, maybe one that turns std abort into panic!? At least it feels to me like those should be separate transforms for a couple of reasons:
- The libc -> std translations is semantics-preserving and guaranteed to be valid. Going to
panic!introduces unwinding that wasn't in the original (though I suspect that's fine, and is likely the direction later lifting would go in anyway). - There might be some amount of judgement we want to apply in deciding which
aborts should becomepanic!s.abortis still valid in Rust, and there are some cases where you might use that instead of a panic, so blindly transforming all aborts into panics might not be right in all cases (though off the top of my head I can't think of an example where we'd want to keep abort over panic). - When we introduce
panic!we may also want to make changes to surrounding code, e.g. if the code is printing out an error immediately before aborting we might want to merge that into thepanic!invocation. It's unclear to me if that's something sensible to do in c2rust-refactor or if that needs to be an LLM-directed lifting.
There was a problem hiding this comment.
It makes sense to separate them, I'm even wondering if the current version should be two separate commands.
| unsafe { | ||
| ::std::process::abort(); | ||
| } | ||
| } | ||
|
|
||
| fn test_exit() { | ||
| unsafe { | ||
| ::std::process::exit(0); | ||
| } | ||
| } | ||
|
|
||
| fn test_exit_error() { | ||
| unsafe { | ||
| ::std::process::exit(1); | ||
| } | ||
| } | ||
|
|
||
| fn test_exit_variable() { | ||
| let code = 42; | ||
| unsafe { | ||
| ::std::process::exit(code); | ||
| } |
There was a problem hiding this comment.
+1, I was going to comment this exact thing. Could we remove the unsafe?
Alternatively, I think we have a transform elsewhere for that, but I don't know if it still works. cargo fix might also handle it.
kkysen
changed the title
exit and abort
randomPoison
force-pushed
the
legare/convert-exits
branch
from
da114dc to
721bb60
Compare
kkysen
left a comment
kkysen
left a comment
There was a problem hiding this comment.
LGTM now! Just some small nits.
kkysen
left a comment
kkysen
left a comment
There was a problem hiding this comment.
Oh wait, actually can this be updated to use the new snapshot tests now?
|
Ah yeah, if the snapshot PR already landed I can rebase and update the tests. |
randomPoison
force-pushed
the
legare/convert-exits
branch
from
10566db to
b458f0d
Compare
| } | ||
|
|
||
| #[test] | ||
| fn test_convert_exits() { |
There was a problem hiding this comment.
Can you keep these alphabetically sorted? e < m
There was a problem hiding this comment.
Sure, let me throw up another PR for that.
Convert calls to libc
abortandexittostd::process::abortandstd::process::exit. Part of #1564.