refactor: Add pass for converting exit and abort#1587
refactor: Add pass for converting exit and abort#1587randomPoison wants to merge 6 commits intomasterfrom
exit and abort#1587Conversation
randomPoison
changed the title
| if local_no_mangle_names.contains(&fi.ident.name) { | ||
| return; | ||
| } | ||
| if def_id.is_local() { |
There was a problem hiding this comment.
Isn't this redundant if you're immediately calling get_if_local? The latter will return None.
| 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.
| // Collect names of locally-defined #[no_mangle] functions that might | ||
| // shadow libc functions. | ||
| let mut local_no_mangle_names = HashSet::new(); | ||
| visit_nodes(krate, |item: &Item| { | ||
| if let ItemKind::Fn(_) = item.kind { | ||
| if crate::util::contains_name(&item.attrs, sym::no_mangle) { | ||
| local_no_mangle_names.insert(item.ident.name); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
This isn't too complex, but we do the exact same thing in #1586, so could we pull it out into a separate function?
There was a problem hiding this comment.
Actually, the same for most of the visit_nodes call, too. Maybe just have a function like visit_non_local_no_mangle_items?
There was a problem hiding this comment.
Yeah that's reasonable. Let's land #1586 first, then I'll rebase this on top and pull out the common logic.
kkysen
changed the title
exit and abort
3 participants
Convert calls to libc
abortandexittostd::process::abortandstd::process::exit. Part of #1564.