Skip to content
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

Prohibit function call macro to const translation #810

Closed
wants to merge 2 commits into from

Conversation

Remmirad
Copy link

@Remmirad Remmirad commented Jan 31, 2023

closes #803

Tackles the issue, that C macros consisting of a function call are translated to rust const expressions which do not compile and are not semantically equivalent.

As described in the code comment there might be a more thorough way to do this but that would, as far as i can tell, at the least require some redesigning of how C2rust handles/represents sideeffects and only would increase performance a bit in a special case, as described in the issue. So for now this here might suffice.

The fix I wrote here stops C2Rust from producing code that does not compile in this case and achieves the behavior that one would expect: All occurrences of the macro in code are replaced by the function call.

I also added corresponding test for this situation.

* add fix for the problematic macro translation mentioned in the issue
* add test for the fix
@Remmirad Remmirad changed the title Prohibit Macro function call expansion Prohibit function call macro to const translation Feb 6, 2023
Copy link
Contributor

@fw-immunant fw-immunant left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

Unfortunately, I don't think this approach is what we want here. Forbidding call expressions on the Rust side is too restrictive (due to const fns), and unfortunately so is forbidding call expressions on the C side (due to builtins which we may translate to Rust builtins or const fns).

As this feature is experimental, I think it's best to simply leave it uncomplicated for now. --translate-const-macros is only useful for the simplest cases, and we don't have a clear path toward making it useful everywhere.

Comment on lines +2239 to +2248
let expr = match expr.result_map(|v| {
if matches!(*v, Expr::Call(_)) {
Err(())
} else {
Ok(v)
}
}) {
Ok(v) => v,
Err(_) => return Err(format_err!("Can not expand macro to function call")),
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be written more simply:

Suggested change
let expr = match expr.result_map(|v| {
if matches!(*v, Expr::Call(_)) {
Err(())
} else {
Ok(v)
}
}) {
Ok(v) => v,
Err(_) => return Err(format_err!("Can not expand macro to function call")),
};
let expr = expr.result_map(|v| {
if matches!(*v, Expr::Call(_)) {
Err(format_err!("Macro definition contains function call"))
} else {
Ok(v)
}
})?;

Comment on lines +2235 to +2238
// This actually should be catched by `to_unsafe_pure_expr` see below
// but a function call expression is translated as having no sideeffects by `convert_expr` (above)
// which is not necessarily correct, we just might not know them (e.g only decl in header exists).
// So we catch this here until there is a way to express unknown sideeffects of an expression.
Copy link
Contributor

Choose a reason for hiding this comment

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

The purpose of to_unsafe_pure_expr is not actually to enforce the kind of purity that Rust consts require. It checks a notion of purity based on "does the expression computing this value have prerequisite statements that we need to ensure are evaluated prior to the expression itself".

We sadly don't have a good notion of "is this Rust expression const-safe", and it's difficult to build one because c2rust is oriented toward reasoning about C code, not Rust code. See this comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problematic macro translation with --translate-const-macros
2 participants