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

Support QNX 7.1 with io-sock+libstd and QNX 8.0 (no_std only) #133631

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

flba-eb
Copy link
Contributor

@flba-eb flba-eb commented Nov 29, 2024

Changes of this pull request:

  1. Refactor code for qnx nto targets to share more code in file nto_qnx.rs

  2. Add support for an additional network stack on nto qnx 7.1.

    QNX 7.1 supports two network stacks:

    1. io-pkt, which is default
    2. io-sock, which is optional on 7.1 but default in QNX 8.0

    As one can see in the io-sock migration notes, this changes the libc API in a way similar to e.g. linux-gnu vs. linux-musl.

    This change adds a new target which has a different value for target_env, so that e.g. libc can distinguish between both APIs.

  3. Add initial support for QNX 8.0, thanks @AkhilTThomas. As it turned out, the problem with forking many processes still exists in QNX 8.0. Because if this, we are now using it for any QNX version (i.e. not check for target_env anymore).

@rustbot label +O-neutrino
CC: @jonathanpallant @japaric @gh-tr @AkhilTThomas

@rustbot
Copy link
Collaborator

rustbot commented Nov 29, 2024

r? @pnkfelix

rustbot has assigned @pnkfelix.
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 rustbot added O-unix Operating system: Unix-like S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. O-neutrino OS: QNX Neutrino, a POSIX-compatible real-time operating system labels Nov 29, 2024
@rust-log-analyzer

This comment has been minimized.

@flba-eb flba-eb force-pushed the add_nto_qnx71_iosock_support branch from 96a91e2 to 6116b6a Compare November 29, 2024 14:13
@rust-log-analyzer

This comment has been minimized.

@jonathanpallant
Copy link
Contributor

So, first up, excellent! The AWS QNX Neutrino 7.1.0 AMIs appears to use iosock and so I have to keep copying over libsocket.so.3 to run Rust code there. With this change hopefully I won't.

However, I note that there are currently no five-part-triples in the target list, so this would be the first. Perhaps aarch64-unknown-nto-qnx710_iosock would be better.

Will there be a x86_64-unknown-nto-qnx710_iosock to match?

@flba-eb

This comment was marked as resolved.

@flba-eb
Copy link
Contributor Author

flba-eb commented Nov 29, 2024

So, first up, excellent! The AWS QNX Neutrino 7.1.0 AMIs appears to use iosock and so I have to keep copying over libsocket.so.3 to run Rust code there. With this change hopefully I won't.

Beware that version 3 and 4 have different APIs -- they may seem to work, but there might be subtle issues up to undefined behavior.

However, I note that there are currently no five-part-triples in the target list, so this would be the first. Perhaps aarch64-unknown-nto-qnx710_iosock would be better.

Very good idea -- I found it confusing that target_env uses an underscore while the target name does not. I will rename it!

Will there be a x86_64-unknown-nto-qnx710_iosock to match?

Not planned so far, but should be quite easy to do. How big is the need for it?

@jonathanpallant
Copy link
Contributor

How big is the need for it?

No idea. Who was asking for this target?

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@flba-eb

This comment was marked as resolved.

@jonathanpallant
Copy link
Contributor

I was thinking to use an obvious dummy value rather than panicking if the environment variable isn't set. But maybe the project people have a better idea.

Are there another other targets that need to read environment variables?

@flba-eb

This comment was marked as resolved.

@flba-eb flba-eb force-pushed the add_nto_qnx71_iosock_support branch from 5051c34 to 00079dc Compare November 29, 2024 15:42
@rust-log-analyzer

This comment has been minimized.

@lcnr lcnr mentioned this pull request Nov 29, 2024
@flba-eb flba-eb force-pushed the add_nto_qnx71_iosock_support branch 6 times, most recently from 72b6642 to 32310c3 Compare November 29, 2024 17:38
@flba-eb
Copy link
Contributor Author

flba-eb commented Dec 18, 2024

Updated the nto-qnx.md document to reflect changes done by #134211 (and rebased).
@workingjubilee I would appreciate very much if you could have a quick look if there is something obvious which needs to be fixed. Changing this PR this week would be easier for me than around Christmas and new years eve.

@bors
Copy link
Contributor

bors commented Dec 23, 2024

☔ The latest upstream changes (presumably #134677) made this pull request unmergeable. Please resolve the merge conflicts.

@flba-eb flba-eb force-pushed the add_nto_qnx71_iosock_support branch from 8781120 to 5758d3b Compare December 24, 2024 08:25
@bors
Copy link
Contributor

bors commented Dec 27, 2024

☔ The latest upstream changes (presumably #134822) made this pull request unmergeable. Please resolve the merge conflicts.

@flba-eb flba-eb force-pushed the add_nto_qnx71_iosock_support branch from 5758d3b to 76dc10d Compare January 2, 2025 08:39
AR_x86_64_pc_nto_qnx710=ntox86_64-ar'
- `CC_<target>=qcc`
- `CFLAGS_<target>=<nto_cflag>`
- `CXX_<target>=qcc`
Copy link
Contributor

@AkhilTThomas AkhilTThomas Jan 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'AR_<target>=<nto<arch>-ar' is missed here as well as in the example below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be required anymore on recent master of Rust (at least to compile libstd). Do you observe issues on this branch?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I faced this issue while compiling this branch against the latest cc-rs. Since rust-lang/cc-rs#1319 is merged i followed the new build steps.
Is this change missed in the cc-rs ?
Similar to the modified get_base_compiler shouldn't the get_base_archiver modified to return qcc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should work without setting the archiever, at least it worked for me 😄 . To not risk delaying this PR further, I've restored the previous documentation, it does not hurt to still set the env variable.

@workingjubilee workingjubilee self-requested a review January 5, 2025 01:26
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does a networking stack change require an entirely new target? I'm not seeing any library code changes that justify it?

@flba-eb
Copy link
Contributor Author

flba-eb commented Jan 5, 2025

why does a networking stack change require an entirely new target? I'm not seeing any library code changes that justify it?

Good point. Sorry for missing out to link to the prepared libc changes: A lot of (networking related) constants have different values, i.e. libstd requires a recompilation at least. In libc, we need to distinguish between the QNX versions, including the network stack 😒 . The versions are not ABI compatible. Also, we need to specify an additional linking search path when using io-sock.

@workingjubilee
Copy link
Member

Lovely.

Thank you for the additional context and your patience, I was very busy in December unfortunately so didn't get around to a lot. Will try to review this soon.

flba-eb and others added 5 commits January 7, 2025 06:54
…rk stack on aarch64

Signed-off-by: Florian Bartels <Florian.Bartels@elektrobit.com>
Signed-off-by: Florian Bartels <Florian.Bartels@elektrobit.com>
Signed-off-by: Florian Bartels <Florian.Bartels@elektrobit.com>
Signed-off-by: Florian Bartels <Florian.Bartels@elektrobit.com>
Signed-off-by: Florian Bartels <Florian.Bartels@elektrobit.com>
@flba-eb flba-eb force-pushed the add_nto_qnx71_iosock_support branch from 76dc10d to 7755502 Compare January 7, 2025 06:55
@AkhilTThomas
Copy link
Contributor

Also, heads up - tomorrow is my last day of work in 2024, so if I can't do a build tomorrow, it'll have to wait until next year or you'll have to find other people with licences to verify this target change.

@jonathanpallant Hope you're back from vacation. Could you take a look at this PR ?

@jonathanpallant
Copy link
Contributor

  • I have build issues on QNX Neutrino 7.0, which I think is because libc needs updating.
  • I cannot build QNX Neutrino 7.1 because my licence expired.
  • I cannot build QNX Neutrino 8.0 because macOS support has been dropped.

But, as I agreed with Florian before Christmas, we should get this merged because the targets existing is a blocker on libc and cc-rs changes.

@jonathanpallant
Copy link
Contributor

jonathanpallant commented Jan 13, 2025

Build command:

#!/bin/bash
source ~/qnx700/qnxsdp-env.sh
export build_env='
         CC_aarch64_unknown_nto_qnx700=qcc
        CXX_aarch64_unknown_nto_qnx700=qcc
     CFLAGS_aarch64_unknown_nto_qnx700=-Vgcc_ntoaarch64le_cxx
         AR_aarch64_unknown_nto_qnx700=ntoaarch64-ar
         CC_i586_pc_nto_qnx700=qcc
        CXX_i586_pc_nto_qnx700=qcc
     CFLAGS_i586_pc_nto_qnx700=-Vgcc_ntox86_cxx
         AR_i586_pc_nto_qnx700=ntox86-ar
'

env $build_env ./x.py build \
  --target aarch64-apple-darwin,aarch64-unknown-nto-qnx700,i586-pc-nto-qnx700 \
  rustc library/core library/alloc library/std src/tools/remote-test-server

Error:

...
Building stage2 library artifacts {alloc, core, std} (aarch64-apple-darwin -> aarch64-unknown-nto-qnx700)
    Finished `release` profile [optimized] target(s) in 0.02s
Building stage2 library artifacts {alloc, core, std} (aarch64-apple-darwin -> i586-pc-nto-qnx700)
error[E0432]: unresolved imports `crate::c_char`, `crate::c_long`, `crate::c_ulong`
   --> /Users/jonathan/.cargo/registry/src/index.crates.io-6f17d22bba15001f/libc-0.2.169/src/macros.rs:84:17
    |
65  | / macro_rules! prelude {
66  | |     () => {
67  | |         /// Frequently-used types that are available on all platforms
68  | |         ///
...   |
84  | |                 c_char, c_double, c_float, c_int, c_long, c_longlong, c_short, c_uchar, c_uint,
    | |                 ^^^^^^                            ^^^^^^ no `c_long` in the root
    | |                 |
    | |                 no `c_char` in the root
    | |                 help: a similar name exists in the module: `c_schar`
85  | |                 c_ulong, c_ulonglong, c_ushort, c_void, intptr_t, size_t, ssize_t, uintptr_t,
    | |                 ^^^^^^^ no `c_ulong` in the root
...   |
88  | |     };
89  | | }
    | |_- in this expansion of `prelude!`
    |
   ::: /Users/jonathan/.cargo/registry/src/index.crates.io-6f17d22bba15001f/libc-0.2.169/src/lib.rs:138:9
    |
138 |           prelude!();
    |           ---------- in this macro invocation
    |
    = help: consider importing this type alias instead:
            rustc_std_workspace_core::ffi::c_char
    = help: consider importing this type alias instead:
            rustc_std_workspace_core::ffi::c_long
    = help: consider importing this type alias instead:
            rustc_std_workspace_core::ffi::c_ulong

@jonathanpallant
Copy link
Contributor

jonathanpallant commented Jan 13, 2025

I think that's a libc bug. I'll open an issue for it.

Edit: rust-lang/libc#4241

QNX SDP 8.0 comes with newly renamed QNX OS 8.0, so update the page to talk about QNX, QNX Neutrino 7.0, QNX Neutrino 7.1 or QNX OS 8.0.

Also actually add a list of target triples.
@jonathanpallant
Copy link
Contributor

jonathanpallant commented Jan 13, 2025

I'm proposing some wording changes to the nto-qnx.md README now that QNX are calling the new version QNX OS 8.0 (dropping the 'Neutrino' label that 7.1 and earlier versions had):

https://github.com/jonathanpallant/rust/tree/add_nto_qnx71_iosock_support_readme_updates

@flba-eb do you want to merge them into this PR, or should we do that and then I'll do that separately?

Edit: also the build failures above are simply because there's no libstd support for i586-pc-nto-qnx700 - it's libcore only, as noted in the support table.

@flba-eb
Copy link
Contributor Author

flba-eb commented Jan 15, 2025

I'm proposing some wording changes to the nto-qnx.md README now that QNX are calling the new version QNX OS 8.0 (dropping the 'Neutrino' label that 7.1 and earlier versions had):

https://github.com/jonathanpallant/rust/tree/add_nto_qnx71_iosock_support_readme_updates

@flba-eb do you want to merge them into this PR, or should we do that and then I'll do that separately?

Edit: also the build failures above are simply because there's no libstd support for i586-pc-nto-qnx700 - it's libcore only, as noted in the support table.

I would love to do this separately in case it helps getting this PR merged sooner, but if there are no review comments in the next few days I will probably merge your improvements.

@workingjubilee
Copy link
Member

I would be happier to see the correct nomenclature in place.

@workingjubilee
Copy link
Member

I must confess I am an insufferable pedant at heart.

@flba-eb
Copy link
Contributor Author

flba-eb commented Jan 15, 2025

I would be happier to see the correct nomenclature in place.

If this is about the change proposed by @jonathanpallant : It is now merged and pushed 😄 .

@jonathanpallant
Copy link
Contributor

I must confess I am an insufferable pedant at heart.

As is the Rust compiler, so we're all in good company :)

@@ -18,30 +20,29 @@ and [Blackberry QNX][BlackBerry].

## Requirements

Currently, the following QNX Neutrino versions and compilation targets are supported:
Currently, the following QNX versions and compilation targets are supported:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the target_os should be migrated to "qnx"... (we don't require the target tuple and the target_os to line up perfectly)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a random thought right now, I won't blockade anything on that.

@@ -139,7 +139,8 @@ test = true
level = "warn"
check-cfg = [
'cfg(bootstrap)',
'cfg(target_arch, values("xtensa"))',
'cfg(target_arch, values("xtensa", "aarch64-unknown-nto-qnx710_iosock"))',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...this seems incorrect?

Some("ARM64 QNX Neutrino 7.1 RTOS with io-sock network stack".into());
target.options.pre_link_args =
nto_qnx::pre_link_args(nto_qnx::ApiVariant::IoSock, nto_qnx::Arch::Aarch64);
target.options.env = "nto71_iosock".into();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel it is almost a misuse of the field but we could use the target_abi for the iosock distinction, or a target_family even? All of these targets are slightly Wrong, because they should be using os_version_min instead, but as that is an RFC and not implemented yet, and thus these targets are defined by crude hackery, I feel like some more crude hackery might be acceptable? It seems incorrect to have to #[cfg(any(target_env = "nto71", target_env = "nto71_iosock"))] everywhere the iosock change doesn't matter.

```
```toml
profile = "compiler"
change-id = 115898
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tbh I usually just use

    change-id = 999999

.unwrap_or_else(|_| "QNX_TARGET_not_set_please_source_qnxsdp-env.sh".into());
let linker_param = format!("-L{target_dir}/{arch_lib_dir}/io-sock/lib");

linker_param.leak()
Copy link
Member

@workingjubilee workingjubilee Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixing this is not necessarily in-scope for this PR, but

Suggested change
linker_param.leak()
// FIXME: leaking this is kind of weird: we're feeding these into something that expects an `AsRef<OsStr>`, but often converts to `OsString` anyways, so shouldn't we just demand an `OsString`?
linker_param.leak()

Comment on lines +52 to +61
llvm_target: "x86_64-pc-unknown".into(),
metadata: meta(),
pointer_width: 64,
data_layout:
"e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128".into(),
arch: "x86_64".into(),
options: TargetOptions {
cpu: "x86-64".into(),
plt_by_default: false,
max_atomic_width: Some(64),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does QNX really support AMD Phenoms?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-neutrino OS: QNX Neutrino, a POSIX-compatible real-time operating system O-unix Operating system: Unix-like relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants