libs: Add static compatiblity check for Rust #13245

no1wudi · 2024-08-30T07:36:09Z

Summary

This patch adds a static compatiblity check
for most of structures from NuttX, which is used by Rust.

Currently, for most of them have a reserved field, which is for possible future use, you can refer to https://github.com/no1wudi/libc/blob/39277b5357cb2aa7af1bd8344956ec292c35abed/src/unix/nuttx/mod.rs#L71 for more details.

I think we should define the initial version carefully, to avoid compatiblity problems in future as much as possible:

Struct reserved size is 2 pointer size, is it sufficient?
Which option should we preferred to use? Such as SYSTEM_TIME64 and FS_LARGEFILE?
Should we enable it by default to cover more cases in build CI? It can prevent introduce compatiblity issues.

Please feel free to comment

anchao · 2024-08-30T07:49:47Z

Why not generate structure code dynamically, but bind statically?

no1wudi · 2024-08-30T08:01:32Z

Why not generate structure code dynamically, but bind statically?

Please see #12960, generate structure code dynamically is OK on technical, but binary compatibility cannot be guaranteed.

Thus the libc/libstd of Rust can not be built without NuttX develop enviroment, if you want to use them, you must use the nighlty toolchain of Rust instead of stable toolchain.

Consider in Rust, the direct use of structs from libc is not very common, so there is an opportunity to implement binary compatibility for these structs.

lupyuen

The checking of Struct Sizes might become more complex in future, but it's a good start. Thanks!

anchao · 2024-08-30T15:34:39Z

Why not generate structure code dynamically, but bind statically?

Please see #12960, generate structure code dynamically is OK on technical, but binary compatibility cannot be guaranteed.

Thus the libc/libstd of Rust can not be built without NuttX develop enviroment, if you want to use them, you must use the nighlty toolchain of Rust instead of stable toolchain.

Consider in Rust, the direct use of structs from libc is not very common, so there is an opportunity to implement binary compatibility for these structs.

libc/libstd definitely needs the participation of nuttx to be compiled, similar to the WAMR glue code we did before, which allows rust to be bound to a certain configuration to avoid doing these meaningless work. It is a common practice for different nuttx header files to generate different lib/libstd.

anchao

I'm voting no on this PR unless there are other RTOS that use similar alternative practices.

xiaoxiang781216 · 2024-08-30T16:21:48Z

I'm voting no on this PR unless there are other RTOS that use similar alternative practices.

Why not support both approach?

NuttX is compliant with POSIX, so it could take the integration approach used by Linux and *BSD as libs: Add static compatiblity check for Rust #13245 and [HELP] FFI compatiblity issues #12960.
Or RTOS approach as risc-v/qemu-rv: Add Build Config for leds64_rust #12862 and examples/leds_rust: Add Rust App for blinking the LED nuttx-apps#2462

lupyuen · 2024-08-31T00:20:23Z

I'm voting no on this PR unless there are other RTOS that use similar alternative practices.

Hi @anchao are we thinking of the bindgen approach? As discussed, it's potentially possible, but it will require special tools because we don't want to recompile libstd for every NuttX Platform. Unlike other OSes (like Linux), I don't think we can afford the effort to create these special tools.

no1wudi · 2024-08-31T01:05:12Z

libc/libstd definitely needs the participation of nuttx to be compiled, similar to the WAMR glue code we did before, which allows rust to be bound to a certain configuration to avoid doing these meaningless work. It is a common practice for different nuttx header files to generate different lib/libstd.

@anchao You can refer to the libc crate for other supported platforms, for example TEEOS:
https://github.com/rust-lang/libc/blob/0e6afd534ed7a0406e9dfc9242c2c9370a5b8d05/src/teeos/mod.rs#L106-L109
and newlib based ROTS:
https://github.com/rust-lang/libc/blob/0e6afd534ed7a0406e9dfc9242c2c9370a5b8d05/src/unix/newlib/mod.rs#L240-L272

For NuttX, reuse the unix port is better.

Only a few data structures need to be defined on the Rust side, and they should be kept in sync with the native definitions on the NuttX side, which are usually related to libc and POSIX/pthread. For function definitions, there should be no doubt; follow the standard definitions to proceed.

For other NuttX private interfaces, such as various peripheral drivers and tasks, the bindgen method can be used for support. These additional packages are not within the scope of libstd and should be provided through additional crates.

BTW：In fact, to make bindgen work out of the box requires support from libstd. If we want to use bindgen for libstd/libc itself, we need to hack it like: https://github.com/lvgl/lv_binding_rust mentions.

lupyuen · 2024-08-31T03:17:34Z

Hi @acassis do you think we should proceed with this approach, to verify the NuttX Struct Sizes? This is for creating the initial version of Rust Standard Library for NuttX. Thanks!

xiaoxiang781216 · 2024-08-31T13:15:51Z

@no1wudi could we check the struct layout by parsing symbol from image's elf file, which is less intrusive and more accurate?

no1wudi · 2024-08-31T14:42:14Z

@no1wudi could we check the struct layout by parsing symbol from image's elf file, which is less intrusive and more accurate?

That need NuttX to build with debug symbol enabled and add a post action after linking. But, why parsing symbol from elf file is more accurate than this approach?

acassis · 2024-08-31T15:12:02Z

Hi @acassis do you think we should proceed with this approach, to verify the NuttX Struct Sizes? This is for creating the initial version of Rust Standard Library for NuttX. Thanks!

@no1wudi @lupyuen maybe we should have an option to let the user test it even when we don't have the guarantees of Struct Sizes case he/she enables EXPERIMENTAL, what to you think?

Something like:

	depends on (SYSTEM_TIME64 && FS_LARGEFILE && !SMALL_MEMORY) || EXPERIMENTAL

no1wudi · 2024-09-01T01:38:28Z

@no1wudi @lupyuen maybe we should have an option to let the user test it even when we don't have the guarantees of Struct Sizes case he/she enables EXPERIMENTAL, what to you think?

Something like:
	depends on (SYSTEM_TIME64 && FS_LARGEFILE && !SMALL_MEMORY) || EXPERIMENTAL

@acassis Do the test for more case is good idea but if the essential kconfig is missing, the test will fail and affects normal usage with EXPERIMENTAL enabled.

Or we use a python script to do the check with elf file after build.

lupyuen · 2024-09-01T03:54:01Z

@no1wudi I agree with Alan:

Rust Standard Library on NuttX will need lots of testing by the NuttX Community. So it's good to release the Initial Version early, get feedback, improve and iterate.
EXPERIMENTAL is good to have because it encourages more folks to try out. And to propose fixes if it breaks.
About the Python Script checking with ELF File: Maybe we can do this in the next release? I think it's better to release the first version early, without too many features. Thanks!

no1wudi · 2024-09-01T04:39:06Z

@lupyuen @acassis OK, indeed, a significant amount of work is still required before it stabilizes. I would like to clarify that the issue regarding the compatibility of struct definitions will take a longer period to address. Firstly, if NuttX introduces any breaking changes, these changes will first be integrated into libc.

Then, we will wait for the libc crate to release a new version, followed by upgrading the Rust libstd's dependency on libc to the latest version. The entire cycle could potentially take several months.

Currently, I would like to draw everyone's attention to the fact that, in order to reduce the likelihood of such occurrences, my current approach is to reserve space equivalent to two pointer sizes for each structure definition on the Rust side to accommodate updates on the NuttX side:

Once this PR is merged, it means that any modifications to these structures will be restricted, especially when adding new structure members.
Is the reservation of space equivalent to two pointer sizes appropriate?

Regarding question 1, I personally believe that the structures currently under review are all included in the POSIX definition. POSIX defines the minimum set of members within these structures and allows OS implementations to extend them. Therefore, if modifications to these definitions are necessary, especially when adding internal data required by NuttX's implementation, special handling for them should be acceptable. For example, with struct tm:

nuttx/include/time.h

Line 139 in 32801d3

long tm_gmtoff; /* Offset from UTC in seconds */

tm_gmtoff is not in POSIX: https://pubs.opengroup.org/onlinepubs/9699919799.orig/basedefs/time.h.html

If in the future we need to add other similar non-standard members that cause the reserved space to be insufficient, then we can wrap these members into an internal NuttX structure and point to this internal structure from struct tm using a pointer.

If it is OK, I'll add EXPERIMENTAL to Kconfig, and enable the static check for QEMU based ARM32/ARM64/RISCV32/RISCV64 in some defconfig to catch breaking changes for the struct.

no1wudi · 2024-09-03T02:06:36Z

The CI report error if EXPERIMENTAL enabled:

====================================================================================
Cmake in present: qemu-armv8a/sotest
Configuration/Tool: qemu-armv8a/sotest
2024-09-02 14:37:07
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
 Select HOST_LINUX=y
 Select HOST_X86_64=y
  Building NuttX...
[1/334] cd /github/workspace/sources/nuttx/build/libs/libc/misc && /usr/local/bin/cmake -E touch /github/workspace/sources/nuttx/libs/libc/misc/lib_utsname.c
[2/334] Building C object libs/librust/CMakeFiles/rust.dir/lib_rust.c.obj
FAILED: libs/librust/CMakeFiles/rust.dir/lib_rust.c.obj 
/tools/ccache/bin/aarch64-none-elf-gcc -D__NuttX__ -isystem /github/workspace/sources/nuttx/include -isystem /github/workspace/sources/nuttx/build/include -isystem /github/workspace/sources/nuttx/build/include_arch -march=armv8-a -mcpu=cortex-a53 -Os -fno-strict-aliasing -fomit-frame-pointer -D_LDBL_EQ_DBL -fno-common -Wall -Wshadow -Wundef -Wno-attributes -Wno-unknown-pragmas -Werror -Wstrict-prototypes -Wno-psabi -ffunction-sections -fdata-sections -g -fdiagnostics-color=always -fmacro-prefix-map=/github/workspace/sources/nuttx= -fmacro-prefix-map=/github/workspace/sources/apps= -fmacro-prefix-map=/github/workspace/sources/nuttx/boards/arm64/qemu/qemu-armv8a= -fmacro-prefix-map=/github/workspace/sources/nuttx/arch/arm64/src/qemu= -MD -MT libs/librust/CMakeFiles/rust.dir/lib_rust.c.obj -MF libs/librust/CMakeFiles/rust.dir/lib_rust.c.obj.d -o libs/librust/CMakeFiles/rust.dir/lib_rust.c.obj -c /github/workspace/sources/nuttx/libs/librust/lib_rust.c
In file included from /github/workspace/sources/nuttx/libs/librust/lib_rust.c:27:
Error: /github/workspace/sources/nuttx/libs/librust/lib_rust.c:139:1: error: static assertion failed: "blkcnt_t size mismatch with Rust"
  139 | static_assert(sizeof(blkcnt_t) == 8, "blkcnt_t size mismatch with Rust");
      | ^~~~~~~~~~~~~
Error: /github/workspace/sources/nuttx/libs/librust/lib_rust.c:144:1: error: static assertion failed: "fsblkcnt_t size mismatch with Rust"
  144 | static_assert(sizeof(fsblkcnt_t) == 8, "fsblkcnt_t size mismatch with Rust");
      | ^~~~~~~~~~~~~
Error: /github/workspace/sources/nuttx/libs/librust/lib_rust.c:149:1: error: static assertion failed: "off_t size mismatch with Rust"
  149 | static_assert(sizeof(off_t) == 8, "off_t size mismatch with Rust");
      | ^~~~~~~~~~~~~
Error: /github/workspace/sources/nuttx/libs/librust/lib_rust.c:157:1: error: static assertion failed: "rlim_t size mismatch with Rust"
  157 | static_assert(sizeof(rlim_t) == 8, "rlim_t size mismatch with Rust");
      | ^~~~~~~~~~~~~

Should we enable the essential options for them in defconfig?

TIME64 should be OK for QEMU, especially for the coming 2038 issue
FS_LARGEFILE should be OK for QEMU, it's useful for complex product with many on device files from our experience
!SMALL_MEMORY, QEMU has nearly unlimit memory

lupyuen · 2024-09-03T02:28:18Z

@no1wudi Sorry I didn't realise the implications. Could we remove EXPERIMENTAL for now, and add it to the next version? So we don't hold up the release of librust. Thanks!

no1wudi · 2024-09-10T11:34:25Z

Let's wait feedback from rust-lang/libc#3909 before merge this PR.

github-actions · 2024-09-24T07:45:19Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

yamt · 2024-09-24T08:17:15Z

does this mean to require everyone here, who might not care rust at all, to maintain certain abi compatibility for rust?
i'm not sure if it's a good idea.
if rust doesn't work well w/o os-level abi stability, it's a problem in rust, not nuttx.

no1wudi · 2024-09-24T08:39:35Z

does this mean to require everyone here, who might not care rust at all, to maintain certain abi compatibility for rust? i'm not sure if it's a good idea. if rust doesn't work well w/o os-level abi stability, it's a problem in rust, not nuttx.

Not just for Rust, it's a common issue for NuttX now, image that you have a prebuilt .a library that referenced to NuttX header, and once the structure or config changed in NuttX side, you must re-compile the static library from source.

Maybe by this approach, not only Rust but also can provide relative stable binary interface for this usage.

yamt · 2024-09-24T09:04:38Z

does this mean to require everyone here, who might not care rust at all, to maintain certain abi compatibility for rust? i'm not sure if it's a good idea. if rust doesn't work well w/o os-level abi stability, it's a problem in rust, not nuttx.

Not just for Rust, it's a common issue for NuttX now, image that you have a prebuilt .a library that referenced to NuttX header, and once the structure or config changed in NuttX side, you must re-compile the static library from source.

Maybe by this approach, not only Rust but also can provide relative stable binary interface for this usage.

my impression is that "recompile everything on configuration changes" isn't a big problem for many of users of nuttx.
users tend to prefer better optimizations (smaller structures, lto, etc) over abi stability.

as it's a big policy change with pros and cons, i suspect we should discuss this topic with a wider audience.

github-actions · 2024-09-24T09:18:16Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

no1wudi · 2024-09-24T09:23:14Z

as it's a big policy change with pros and cons, i suspect we should discuss this topic with a wider audience.

Agree, so I open an issue to collect the idea from NuttX community: #12960

lupyuen · 2024-09-24T09:31:45Z

Hi @no1wudi could you share on the Mailing List the current approach for Rust Standard Library? And explain the tradeoff between Better Optimizations (smaller structures, lto, etc) vs ABI Stability? I think we need the community to agree on this. Thanks!

no1wudi · 2024-09-24T10:02:04Z

Hi @no1wudi could you share on the Mailing List the current approach for Rust Standard Library? And explain the tradeoff between Better Optimizations (smaller structures, lto, etc) vs ABI Stability? I think we need the community to agree on this. Thanks!

OK

no1wudi · 2024-09-25T02:21:26Z

@yamt @lupyuen The current approach is not to enforce a mandatory requirement that we cannot modify these structures or the values of macro definitions, but rather to strengthen the review of their modifications. If possible, we should try not to modify them. However, if these modifications are indeed necessary, we should make the changes in conjunction with the Rust side.

no1wudi · 2024-09-25T02:23:03Z

Make it ready to review since rust-lang/libc#3909 merged

This patch adds a static compatiblity check for most of structures from NuttX, which is used by Rust. Currently, for most of them have a reserved field, which is for possible future use, you can refer to https://github.com/no1wudi/libc/blob/39277b5357cb2aa7af1bd8344956ec292c35abed/src/unix/nuttx/mod.rs#L71 for more details. I think we should define the initial version carefully, to avoid compatiblity problems in future as much as possible: 1. Struct reserved size is 2 pointer size, is it sufficient? 2. Which option should we preferred to use? Such as SYSTEM_TIME64 and FS_LARGEFILE? 3. Should we enable it by default to cover more cases in build CI? It can prevent introduce compatiblity issues. Signed-off-by: Huang Qi <[email protected]>

It does not mean to enforce Rust compatibility, but rather to provide a way for maintainers to check if any breaking changes were introduced. Signed-off-by: Huang Qi <[email protected]>

github-actions · 2024-09-27T02:43:03Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

xiaoxiang781216 · 2024-09-28T11:32:23Z

should we move the code to apps(e.g. rustcheck)? since all of them is checking the size and offset of posix type.

xiaoxiang781216 · 2024-09-28T11:18:09Z

libs/libc/librust/lib_rust.c

+                  offsetof(struct stat, st_dev) + sizeof(dev_t),
+              "st_ino offset mismatch");
+static_assert(offsetof(struct stat, st_mode) ==
+                  offsetof(struct stat, st_ino) + sizeof(mode_t),


why sizeof(mode_t)?

Some members have memory align issue:

dev_t is 4 byte

ino_t is 2 byte

mode_t is 4 byte

so if use offsetof(struct stat, st_ino) + sizeof(ino_t) = 6, but offset of mode_t will be aligned to 8.

no1wudi marked this pull request as draft August 30, 2024 07:36

no1wudi force-pushed the rust branch from 080a2e0 to 16cf522 Compare August 30, 2024 07:39

no1wudi mentioned this pull request Aug 30, 2024

[HELP] FFI compatiblity issues #12960

Open

1 task

lupyuen approved these changes Aug 30, 2024

View reviewed changes

no1wudi force-pushed the rust branch 3 times, most recently from 752964e to f78bdcc Compare August 30, 2024 10:00

anchao requested changes Aug 30, 2024

View reviewed changes

no1wudi force-pushed the rust branch 2 times, most recently from aa7d49a to f5b5c35 Compare August 31, 2024 02:21

acassis requested review from pkarashchenko and patacongo August 31, 2024 10:01

no1wudi force-pushed the rust branch from f5b5c35 to 8ae4f07 Compare September 2, 2024 13:56

acassis approved these changes Sep 2, 2024

View reviewed changes

no1wudi force-pushed the rust branch from 8ae4f07 to be963fc Compare September 3, 2024 12:22

no1wudi mentioned this pull request Sep 11, 2024

Initial support for NuttX rust-lang/libc#3909

Merged

no1wudi force-pushed the rust branch from be963fc to fe69339 Compare September 19, 2024 15:17

github-actions bot added the Size: L The size of the change in this PR is large label Sep 19, 2024

no1wudi force-pushed the rust branch from fe69339 to e34fe4c Compare September 20, 2024 03:39

github-actions bot added Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces. and removed Size: L The size of the change in this PR is large labels Sep 20, 2024

no1wudi force-pushed the rust branch from e34fe4c to 0f0de9a Compare September 24, 2024 07:44

github-actions bot added the Area: OS Components OS Components issues label Sep 24, 2024

no1wudi force-pushed the rust branch from 0f0de9a to eabab17 Compare September 24, 2024 09:17

no1wudi marked this pull request as ready for review September 25, 2024 02:22

no1wudi added 2 commits September 27, 2024 10:29

boards/qemu-rv: Add rust/rust64 as Rust compatibility checker

f492d79

It does not mean to enforce Rust compatibility, but rather to provide a way for maintainers to check if any breaking changes were introduced. Signed-off-by: Huang Qi <[email protected]>

no1wudi force-pushed the rust branch from eabab17 to f492d79 Compare September 27, 2024 02:42

github-actions bot added the Area: Board support Board support issues label Sep 27, 2024

lupyuen approved these changes Sep 27, 2024

View reviewed changes

xiaoxiang781216 reviewed Sep 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libs: Add static compatiblity check for Rust #13245

libs: Add static compatiblity check for Rust #13245

no1wudi commented Aug 30, 2024

anchao commented Aug 30, 2024

no1wudi commented Aug 30, 2024

lupyuen left a comment

anchao commented Aug 30, 2024

anchao left a comment

xiaoxiang781216 commented Aug 30, 2024

lupyuen commented Aug 31, 2024 •

edited

Loading

no1wudi commented Aug 31, 2024 •

edited

Loading

lupyuen commented Aug 31, 2024

xiaoxiang781216 commented Aug 31, 2024

no1wudi commented Aug 31, 2024

acassis commented Aug 31, 2024 •

edited

Loading

no1wudi commented Sep 1, 2024

lupyuen commented Sep 1, 2024

no1wudi commented Sep 1, 2024 •

edited

Loading

no1wudi commented Sep 3, 2024 •

edited

Loading

lupyuen commented Sep 3, 2024

no1wudi commented Sep 10, 2024

github-actions bot commented Sep 24, 2024

yamt commented Sep 24, 2024

no1wudi commented Sep 24, 2024

yamt commented Sep 24, 2024

github-actions bot commented Sep 24, 2024

no1wudi commented Sep 24, 2024

lupyuen commented Sep 24, 2024

no1wudi commented Sep 24, 2024

no1wudi commented Sep 25, 2024

no1wudi commented Sep 25, 2024 •

edited

Loading

github-actions bot commented Sep 27, 2024

xiaoxiang781216 commented Sep 28, 2024

xiaoxiang781216 Sep 28, 2024

no1wudi Sep 28, 2024

libs: Add static compatiblity check for Rust #13245

Are you sure you want to change the base?

libs: Add static compatiblity check for Rust #13245

Conversation

no1wudi commented Aug 30, 2024

Summary

anchao commented Aug 30, 2024

no1wudi commented Aug 30, 2024

lupyuen left a comment

Choose a reason for hiding this comment

anchao commented Aug 30, 2024

anchao left a comment

Choose a reason for hiding this comment

xiaoxiang781216 commented Aug 30, 2024

lupyuen commented Aug 31, 2024 • edited Loading

no1wudi commented Aug 31, 2024 • edited Loading

lupyuen commented Aug 31, 2024

xiaoxiang781216 commented Aug 31, 2024

no1wudi commented Aug 31, 2024

acassis commented Aug 31, 2024 • edited Loading

no1wudi commented Sep 1, 2024

lupyuen commented Sep 1, 2024

no1wudi commented Sep 1, 2024 • edited Loading

no1wudi commented Sep 3, 2024 • edited Loading

lupyuen commented Sep 3, 2024

no1wudi commented Sep 10, 2024

github-actions bot commented Sep 24, 2024

yamt commented Sep 24, 2024

no1wudi commented Sep 24, 2024

yamt commented Sep 24, 2024

github-actions bot commented Sep 24, 2024

no1wudi commented Sep 24, 2024

lupyuen commented Sep 24, 2024

no1wudi commented Sep 24, 2024

no1wudi commented Sep 25, 2024

no1wudi commented Sep 25, 2024 • edited Loading

github-actions bot commented Sep 27, 2024

xiaoxiang781216 commented Sep 28, 2024

xiaoxiang781216 Sep 28, 2024

Choose a reason for hiding this comment

no1wudi Sep 28, 2024

Choose a reason for hiding this comment

lupyuen commented Aug 31, 2024 •

edited

Loading

no1wudi commented Aug 31, 2024 •

edited

Loading

acassis commented Aug 31, 2024 •

edited

Loading

no1wudi commented Sep 1, 2024 •

edited

Loading

no1wudi commented Sep 3, 2024 •

edited

Loading

no1wudi commented Sep 25, 2024 •

edited

Loading