libc icon indicating copy to clipboard operation
libc copied to clipboard

Allow Neutrino QNX SDK dependent values for PTHREAD_MUTEX_INITIALIZER

Open flba-eb opened this issue 1 year ago • 11 comments

Description

This change tries to determine the value of a constant at compile time of libc: build.rs reads and "parses" the content of the C header file pthread.h. "Parse" means that it checks for the usage of other constants, which is sufficient for all observed cases.

Background:

In Neutrino QNX, some "constant" values get changed between releases of the toolchain, including in minor patch-versions. Whenever you get a new "QNX toolchain" (SDP), C and C++ developers know that they must recompile everything.

So far, I've only observed three variations, but there could be more that I'm not aware of. This is why I don't want to add and maintain a possibly long list of compiler targets, this would not work well.

Using this patch requires the environment variable QNX_TARGET which is always the case when compiling for QNX (required by the linker).

Conceptionally this change is similar to what FreeBSD is doing, but reads header file instead of running an external process (freebsd-version).

CC and todo list

CC: @jonathanpallant @japaric @gh-tr @AkhilTThomas

  • Relevant tests in libc-test/semver have been updated: 👎 unfortunately impossible
  • [x] No placeholder or unstable values like *LAST or *MAX are included (see #3131)
  • [x] Tested locally (cd libc-test && cargo test --target mytarget); especially relevant for platforms that may not be checked in CI

flba-eb avatar Dec 10 '24 14:12 flba-eb

r? @tgross35

rustbot has assigned @tgross35. They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot avatar Dec 10 '24 14:12 rustbot

@flba-eb We had discussed this but it was not clear for me as to why we cannot define the values for these consts under a cfg flag for each of the variants. Like this as an example

cfg_if! {
    if #[cfg(target_env= "nto71")] {
    pub const PTHREAD_MUTEX_INITIALIZER: pthread_mutex_t = pthread_mutex_t {
        __u: 0x80000000,
        __owner: 0xffffffff,
    };
    }
    else if #[cfg(target_env = "nto80")] {
        pub const PTHREAD_MUTEX_INITIALIZER: pthread_mutex_t = pthread_mutex_t {
            __u: _NTO_SYNC_NONRECURSIVE,
            __owner: _NTO_SYNC_MUTEX_FREE,
        };
    }
    else {
        //...
    }
}

AkhilTThomas avatar Dec 10 '24 14:12 AkhilTThomas

@flba-eb We had discussed this but it was not clear for me as to why we cannot define the values for these consts under a cfg flag for each of the variants. Like this as an example

cfg_if! {
    if #[cfg(target_env= "nto71")] {
    pub const PTHREAD_MUTEX_INITIALIZER: pthread_mutex_t = pthread_mutex_t {
        __u: 0x80000000,
        __owner: 0xffffffff,
    };
    }
    else if #[cfg(target_env = "nto80")] {
        pub const PTHREAD_MUTEX_INITIALIZER: pthread_mutex_t = pthread_mutex_t {
            __u: _NTO_SYNC_NONRECURSIVE,
            __owner: _NTO_SYNC_MUTEX_FREE,
        };
    }
    else {
        //...
    }
}

Well, basically this is what the change does, but it uses conditional compilation switches set by build.rs instead of target_env. To change target_env I would need to add a lot of compiler targets, and I wouldn't even know how to name them. Also, in this case only one value changed, but I would not be surprised if other versions (project specific) of QNX toolchains use other values and also change other constants. We would get an explosion of compiler targets.

I would prefer to determine the actual value in the build.rs script and use it in the libc code, but this does not work:

  • Conditional compilation cannot "transport" data (like a C preprocessor definition)
  • Generating Rust code would work theoretically (I've tried it) but will break libc's test framework (which parses the Rust code to generate C code from it).

flba-eb avatar Dec 10 '24 14:12 flba-eb

Well, basically this is what the change does, but it uses conditional compilation switches set by build.rs instead of target_env. To change target_env I would need to add a lot of compiler targets, and I wouldn't even know how to name them. Also, in this case only one value changed, but I would not be surprised if other versions (project specific) of QNX toolchains use other values and also change other constants. We would get an explosion of compiler targets.

I would prefer to determine the actual value in the build.rs script and use it in the libc code, but this does not work:

  • Conditional compilation cannot "transport" data (like a C preprocessor definition)
  • Generating Rust code would work theoretically (I've tried it) but will break libc's test framework (which parses the Rust code to generate C code from it).

IMHO this is still not a scalable solution as the other structs will have different number and named elements and hence users will need to extend the parser to more elements. I'd prefer that the consts/structs are divided into modules specific files like.

unix>nto>
├── aarch64.rs
├── mod.rs
├── neutrino.rs
├── nto7071
│   └── mod.rs
├── nto80
│   └── mod.rs
└── x86_64.rs

AkhilTThomas avatar Dec 10 '24 15:12 AkhilTThomas

Well, basically this is what the change does, but it uses conditional compilation switches set by build.rs instead of target_env. To change target_env I would need to add a lot of compiler targets, and I wouldn't even know how to name them. Also, in this case only one value changed, but I would not be surprised if other versions (project specific) of QNX toolchains use other values and also change other constants. We would get an explosion of compiler targets. I would prefer to determine the actual value in the build.rs script and use it in the libc code, but this does not work:

  • Conditional compilation cannot "transport" data (like a C preprocessor definition)
  • Generating Rust code would work theoretically (I've tried it) but will break libc's test framework (which parses the Rust code to generate C code from it).

IMHO this is still not a scalable solution as the other structs will have different number and named elements and hence users will need to extend the parser to more elements. I'd prefer that the consts/structs are divided into modules specific files like.

unix>nto>
├── aarch64.rs
├── mod.rs
├── neutrino.rs
├── nto7071
│   └── mod.rs
├── nto80
│   └── mod.rs
└── x86_64.rs

I don't want to determine all values at compile time (in build.rs). Only when necessary, e.g. between different "7.1" versions. For 7.0, 7.1 and 8.0 it should be perfectly fine to hard-code all remaining values (>99%). If it makes sense to store them in different more specific module files then let's do it! For now, I would like to only have the mutex initializer value determined at compile time. Maybe in one or two years, we have two or three such values, but hopefully not more.

flba-eb avatar Dec 10 '24 15:12 flba-eb

I don't want to determine all values at compile time (in build.rs). Only when necessary, e.g. between different "7.1" versions. For 7.0, 7.1 and 8.0 it should be perfectly fine to hard-code all remaining values (>99%). If it makes sense to store them in different more specific module files then let's do it! For now, I would like to only have the mutex initializer value determined at compile time. Maybe in one or two years, we have two or three such values, but hopefully not more.

In that case i agree :) Once the libc PR for iosock lands i'll start with the splitting into os version specific modules.

AkhilTThomas avatar Dec 10 '24 15:12 AkhilTThomas

I don't think we can go with this approach: this isn't robust against #ifs and I don't think we really want libc to start reading system headers anyway.

Well, basically this is what the change does, but it uses conditional compilation switches set by build.rs instead of target_env. To change target_env I would need to add a lot of compiler targets, and I wouldn't even know how to name them. Also, in this case only one value changed, but I would not be surprised if other versions (project specific) of QNX toolchains use other values and also change other constants. We would get an explosion of compiler targets.

Could you elaborate this a bit - do you mean that e.g. within the -qnx700 target this value may change depending on minor version (7.0.0 vs. 7.0.1)?

There is some recent discussion on breaking changes within a target string, if this applies here then it would be good if you could comment on that thread https://rust-lang.zulipchat.com/#narrow/channel/219381-t-libs/topic/How.20to.20handle.20breaking.20changes.20in.20libc.201.2E0

tgross35 avatar Dec 10 '24 18:12 tgross35

I don't think we can go with this approach: this isn't robust against #ifs and I don't think we really want libc to start reading system headers anyway.

Well, basically this is what the change does, but it uses conditional compilation switches set by build.rs instead of target_env. To change target_env I would need to add a lot of compiler targets, and I wouldn't even know how to name them. Also, in this case only one value changed, but I would not be surprised if other versions (project specific) of QNX toolchains use other values and also change other constants. We would get an explosion of compiler targets.

Could you elaborate this a bit - do you mean that e.g. within the -qnx700 target this value may change depending on minor version (7.0.0 vs. 7.0.1)?

It can be even more subtle than that. In one project, we have a project specific version like myproject-bla-bla2-1-4-8-0-r00006.1 which got updated to myproject-bla-bla2-1-5-3-0-r00008.2. The first version uses the same value for PTHREAD_RMUTEX_INITIALIZER as the official 7.1 version does, but the second version uses a different value. Both call themselves "QNX 7.1".

I am fully aware that my "parsing" approach is error-prone, and I've considered calling the C preprocessor instead and parsing it's output. However, I've decided against that because:

  1. I get a value that I cannot easily get into the libc source code (conditional compilation does not allow that, only features like "value_is_80" or "value_is_82".
  2. It should not matter, as the QNX users should compile and run the libc test application anyway each time their QNX toolchain is updated. In case my parsing is wrong (or any other value has changed), the test application will detect it.

There is some recent discussion on breaking changes within a target string, if this applies here then it would be good if you could comment on that thread https://rust-lang.zulipchat.com/#narrow/channel/219381-t-libs/topic/How.20to.20handle.20breaking.20changes.20in.20libc.201.2E0

I will comment there.

flba-eb avatar Dec 11 '24 08:12 flba-eb

As an alternative option, build.rs could compile a simple C file which defines a global variable and extract the value with objdump. Something like:

#include <pthread.h>
pthread_mutex_t _PTHREAD_MUTEX_INITIALIZER = PTHREAD_MUTEX_INITIALIZER;
$ qcc -Vgcc_ntoaarch64le -c check.c check.o && ntoaarch64-objdump check.o -d -j .data

check.o:     file format elf64-littleaarch64


Disassembly of section .data:

0000000000000000 <_PTHREAD_MUTEX_INITIALIZER>:
   0:   00 00 00 80 ff ff ff ff                             ........

Then build.rs could parse the output... Would this be acceptable?

flba-eb avatar Dec 11 '24 13:12 flba-eb

I'm sorry for not following up here; I think this probably would have been a good candidate for Zulip discussion.

Here is my stance on a few different approaches:

  • Parsing the header files ourselves I unfortunately have to say is a no-go: it's not a simple thing to have and maintain in libc's source, and it's too easy to break by something like a trivial #ifdef added to the Neutrino source code.

  • Doing a check build seems better but I'm iffy because that probably means we need to expose cc config somehow. You'd need to bring this up on the t-libs Zulip and convince the others that this this is reasonable to be doing in libc.

    If this winds up being the approach, it should be possible to get the info from the compiler invoked with -E -dM rather than checking an object file.

  • My preferred option would be checking an environment variable that can be set as part of a build from another build system. Or querying a tell-me-the-qnx-config CLI util if such a thing exists, though that has some of the same problems as building a C file. We already have these kind of approaches for a few different targets.

If QNX needs more than that or if there is the possibility that we need more of these options in the future, I think you may want to look into creating a dedicated crate for QNX bindings that allows for the needed config (having maintainers that have access to and familiarity with QNX would probably be pretty helpful). We already have a handful of such target-specific crates used by std, e.g. fortanix-sgx-abi, hermit-abi, r-efi, wasi, etc.

@rustbot author

tgross35 avatar Oct 29 '25 21:10 tgross35

Reminder, once the PR becomes ready for a review, use @rustbot ready.

rustbot avatar Oct 29 '25 21:10 rustbot