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

Get rid of C2Rust #19498

Open
1 of 4 tasks
chrysn opened this issue Apr 24, 2023 · 3 comments
Open
1 of 4 tasks

Get rid of C2Rust #19498

chrysn opened this issue Apr 24, 2023 · 3 comments
Labels
Type: tracking The issue tracks and organizes the sub-tasks of a larger effort

Comments

@chrysn
Copy link
Member

chrysn commented Apr 24, 2023

Description

C2Rust is an extra that occasionally causes trouble, eg. in #19495 (comment), where I was tempted to answer:

Yes, getting rid of c2rust would be an interesting goal, but a lot of our API surface is not actually API in the symbols-types-and-linker sense and more in the preprocessor-and-static-inline sense. Last time I checked there was a great many things that were only accessible through the latter in RIOT, too many to change on the short term, and without other users than Rust. (I've seen some usage from C++ where things like mutex_trylock_ffi, but just grepping for 'inline' (eg. in riot_sys::inline which is where the c2rust items come through, or in crate::inline_cast which converts the types of bindgen and c2rust with extra checks) reveals a lot of locations, and that's not yet accounting for the toplevel_from_inline list (26 items in riot-sys' build.rs) that makes sure things also work for items that are possibly static depending on the platform. A lot of these have not been checked in a while, but unless we aim to go full clean-API, there'd be c2rust left.

… so instead, I'm opening this.

I'm not sure now is a good point in time, neither ever, but let's at least use this issue to hypothesize, and to collect material, show-stoppers and input.

Why we use C2Rust now

  • API surface in RIOT is not clean API but a mixture of exported symbols, static inlines and preprocessor. bindgen works well when linking against a library, but RIOT is not designed to be used as a library.
    • We'd need to get rid of, or have equivalent versions, of all preprocessor macros that are regularly used.
    • We'd need to make all static inline functions usable from a linked program. I'm marking this as "solved" because bindgen has learned to turn public static functions into non-static functions that can be linked when an extra generated C file containing all their implementations is linked -- so there is a viable solution, but see next point.
  • Performance reasons: The reason for having functions static functions is that it allows inlining, and many such functions are on hot paths. Wouldn't be too bad, if we could
    • Fix everything that's broken when using LTO. (I'm not sure there's anything left, last time I checked was ages ago, but our CI doesn't use it, and if it ain't tested it ain't working)
    • Allow linking with non-GCC linkers
      for then we could both C and Rust code into LLVM object files and do cross-language LTO to eliminate any drawbacks of using non-static functions. (Cross-language LTO may have other issues, but right now we can't even test it because RIOT only works with ).

If we could tick all these boxes, we could migrate off C2Rust. But that'd probably only fly if there were other reasons as well that'd justify ticking the boxes. Do we have them?

@maribu
Copy link
Member

maribu commented Apr 24, 2023

LTO should be the default anyway, as this yields significant gains; being able to run more apps without soldering in a new MCU with more RAM/ROM is something we should do, regardless of this discussions.

With that, we don't gain anything from static inline anymore. If we would move then all static inline to C files, we could avoid including vendor header files in our headers. The vendor header files are perfectly fine C headers, but typically are not C++ conform (even in extern "C"). g++ is quite tolerant to C-but-not-C++ conformant code, while clang++ is not. This is why we stopped compile-testing with LLVM. Move static inline stuff to C file would be a major step towards regaining LLVM support (likely other issues have sneaked in in the meantime that would need fixing as well).

Getting our linker scripts compatible with other linkers is something that IMO would be worth some effort. I think that XFA may have raised the bar a bit here, though :/

@chrysn
Copy link
Member Author

chrysn commented Apr 24, 2023

I don't see any trouble with XFA: It's just another section that gets linked into RAM or ROM, that should be a regular feature. The trouble IIRC was that we did some constant arithmetic to be able to do things like accommodating different chips in a series with different sizes of memory in different situations of using or not using a boot loader and other flash size shenanigans -- where the alternative is to either have combinatorial lot of such files, or to generate them with a very small shellPython script.

@kaspar030
Copy link
Contributor

I can confirm that with fixed ldscripts, cross-language-lto works between Rust and RIOT's C code.
The arithmetics in ldscripts is indeed the problem. I worked around that by hard-coding the arithmetic results, which only makes sense for a single board and for testing. I'd love if we could fix this!

@maribu maribu added the Type: tracking The issue tracks and organizes the sub-tasks of a larger effort label May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: tracking The issue tracks and organizes the sub-tasks of a larger effort
Projects
None yet
Development

No branches or pull requests

3 participants