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

Allow Neutrino QNX SDK dependent values for PTHREAD_MUTEX_INITIALIZER #4192

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

flba-eb
Copy link
Contributor

@flba-eb flba-eb commented Dec 10, 2024

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
  • No placeholder or unstable values like *LAST or *MAX are
    included (see #3131)
  • Tested locally (cd libc-test && cargo test --target mytarget);
    especially relevant for platforms that may not be checked in CI

@rustbot
Copy link
Collaborator

rustbot commented Dec 10, 2024

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

@flba-eb flba-eb marked this pull request as ready for review December 10, 2024 14:25
@AkhilTThomas
Copy link

@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 {
        //...
    }
}

@flba-eb
Copy link
Contributor Author

flba-eb commented Dec 10, 2024

@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).

@AkhilTThomas
Copy link

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

@flba-eb
Copy link
Contributor Author

flba-eb commented Dec 10, 2024

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.

@AkhilTThomas
Copy link

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.

@tgross35
Copy link
Contributor

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

@flba-eb
Copy link
Contributor Author

flba-eb commented Dec 11, 2024

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
Copy link
Contributor Author

flba-eb commented Dec 11, 2024

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?

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

Successfully merging this pull request may close these issues.

4 participants