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

Problematic macro translation with --translate-const-macros #803

Open
Remmirad opened this issue Jan 28, 2023 · 2 comments
Open

Problematic macro translation with --translate-const-macros #803

Remmirad opened this issue Jan 28, 2023 · 2 comments
Labels
bug Something isn't working

Comments

@Remmirad
Copy link

I believe there exists a Problem with the --translate-const-macros option. A macro #define TEST foo() gets translated to pub const TEST: t = foo();.

This does not compile because foo() is a C function and therefore an unsafe binding but no unsafe Block is generated. In addition foo would need to be compiletime const. Which as far as I know is not possible for bindings? And even if that would be possible c2rust would need to be able to check if it really is compiletime const, which it cant if only provided the functions definition.

And last the achieved behavior in Rust differs from the behavior in C.
I think it would be ok to just completely forbid macro translation into a const if the macro expression is a function call?

The only scenario where this would be a bit problematic, is when there exists a function foo and c2rust has access to the implementation of the function and is able to correctly translate it into Rust and check that it can be evaluated at compile time. Than the code could compile with the unsafe block added. But it would still not represent the C behavior as far as I can see. So maybe it is not really a problem.

Test :

test.h

int foo();

#define GET_FOO (foo())

int use_get_foo() {
    return GET_FOO;
}

transpile flags:

--translate-const-macros
@kkysen kkysen changed the title Problematic macro translation with --translate-const-macros Problematic macro translation with --translate-const-macros Jan 30, 2023
@kkysen kkysen added the bug Something isn't working label Jan 30, 2023
Remmirad added a commit to Remmirad/fix-wrong-macro-translation that referenced this issue Jan 31, 2023
* add fix for the problematic macro translation mentioned in the issue
* add test for the fix
@fw-immunant
Copy link
Contributor

Thanks for the bug report--I can confirm the problematic behavior. The --translate-const-macros flag is pretty experimental and it's not surprising that it breaks here.

This issue is subtler than it may appear because the real constraint here is on the Rust side--we cannot generate a const whose body calls non-const functions. It isn't enough to look for function calls in the C source of the macro definition, though, because c2rust converts some C function calls (e.g. builtins) into Rust expressions that consist only of builtin operators and const function calls, and which we can translate. For example, #define FOO __builtin_clz(34) becomes pub const DOIT: libc::c_int = printf(b"hi\n\0" as *const u8 as *const libc::c_char); which compiles just fine.

We could, then, say "is the Rust code we generate for this macro definition const-safe?" as the predicate to use when deciding if we can translate a macro. This seems like the right way to capture the constraint that's actually relevant here. Unfortunately, c2rust rather naïvely builds an AST of the code to output and doesn't have a notion of which functions are const/not in that AST. It would be significant engineering effort to implement this notion, and as --translate-const-macros is already an experimental, somewhat hacky, approach, I think we probably should leave it as-is rather than complicating how it's broken without knowing it's a path to a fully reliable feature.

@Remmirad
Copy link
Author

Thanks for the feedback. Yes a fully reliable feature like this would need the complex const check. I briefly thought about changing the scope of the feature to "promise" only to translate value like macros e.g. #define M 5 but than this would have limited use and one quickly would want to do things like #define M (5+foo()) and would be here again.

In a concrete use-case it is possibly to work around this by injecting C functions via a build-script like this:

int macro_const() {
    return MACRO;
}

Its only annoying because you have to keep a list of the Macros around. Maybe in the future C2Rust will be able to check for const or at least something that comes close to do this correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants