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

Add QNX 7.0 x86 target #1

Closed
wants to merge 1 commit into from
Closed

Conversation

samkearney
Copy link
Collaborator

@samkearney samkearney commented Mar 4, 2023

Add QNX 7.0 x86 target with no_std support only for now.

This is a preliminary PR for review before opening it against the main rust repo.

Successfully tested a no_std app on my x86 32-bit QNX 7.0 environment. It looks like running any part of the rust test suite requires std support for the target, so that will have to wait until I finish with the std support.

cc @gh-tr

@samkearney samkearney requested a review from flba-eb March 4, 2023 06:10
Copy link
Owner

@flba-eb flba-eb left a comment

Choose a reason for hiding this comment

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

@samkearney : Thank you very much for your contribution!

This looks good from my point of view. I cannot really review the target options like data_layout (same as in i686 linux gnu) and llvm_target (i586-pc-unknown).
@gh-tr: Is the same configuration used in qcc-clang? Any other thoughts?

In my opinion we should make this an official PR.

@flba-eb
Copy link
Owner

flba-eb commented Mar 6, 2023

Not relevant yet in this PR, but we will later need to set environment variables:

diff --git a/src/doc/rustc/src/platform-support/nto-qnx.md b/src/doc/rustc/src/platform-support/nto-qnx.md
index 0d815c9b598..fd74ab5cbf7 100644
--- a/src/doc/rustc/src/platform-support/nto-qnx.md
+++ b/src/doc/rustc/src/platform-support/nto-qnx.md
@@ -117,7 +117,11 @@ export build_env='
     CC_x86_64-pc-nto-qnx710=qcc
     CFLAGS_x86_64-pc-nto-qnx710=-Vgcc_ntox86_64_cxx
     CXX_x86_64-pc-nto-qnx710=qcc
-    AR_x86_64_pc_nto_qnx710=ntox86_64-ar'
+    AR_x86_64_pc_nto_qnx710=ntox86_64-ar
+    CC_i586-pc-nto-qnx700=qcc
+    CFLAGS_i586-pc-nto-qnx700=-Vgcc_ntox86_cxx
+    CXX_i586-pc-nto-qnx700=qcc
+    AR_i586_pc_nto_qnx700=ntox86-ar'
 
 env $build_env \
     ./x.py build \
@@ -147,7 +151,11 @@ export build_env='
     CC_x86_64-pc-nto-qnx710=qcc
     CFLAGS_x86_64-pc-nto-qnx710=-Vgcc_ntox86_64_cxx
     CXX_x86_64-pc-nto-qnx710=qcc
-    AR_x86_64_pc_nto_qnx710=ntox86_64-ar'
+    AR_x86_64_pc_nto_qnx710=ntox86_64-ar
+    CC_i586-pc-nto-qnx700=qcc
+    CFLAGS_i586-pc-nto-qnx700=-Vgcc_ntox86_cxx
+    CXX_i586-pc-nto-qnx700=qcc
+    AR_i586_pc_nto_qnx700=ntox86-ar'
 
 # Disable tests that only work on the host or don't make sense for this target.
 # See also:

@samkearney
Copy link
Collaborator Author

@flba-eb Thanks for the feedback! 2 questions:

  1. Should I wait for feedback from @gh-tr on your question about the target configuration?
  2. When opening the PR against the rust repo, should I fill out the target policy criteria again like was done here: Add tier 3 no_std AArch64/x86_64 support for the QNX Neutrino RTOS rust-lang/rust#102701, or is it not necessary since QNX support in general has already been merged?

Copy link

@gh-tr gh-tr left a comment

Choose a reason for hiding this comment

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

In general it looks fine. If I were adding this, I definitely would have added the other target architectures for completeness. I'm guessing your interesting doesn't extend outside of x86?

Copy link
Collaborator Author

@samkearney samkearney left a comment

Choose a reason for hiding this comment

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

If I were adding this, I definitely would have added the other target architectures for completeness. I'm guessing your interesting doesn't extend outside of x86?

Right now yes -- our support for other architectures like x86_64 will also include migrating to QNX 7.1, which you have already implemented support for. But the main reason is that I don't have any 7.0 images ready to test with on architectures besides x86, and I think the nice workflow with qemu and mkqnximage was added in SDP 7.1, so that doesn't help for 7.0. If you know of a quick way for me to spin up an emulated QNX 7.0 test environment for x86_64 and aarch64, I'd be happy to include them.

@gh-tr
Copy link

gh-tr commented Mar 14, 2023

If I were adding this, I definitely would have added the other target architectures for completeness. I'm guessing your interesting doesn't extend outside of x86?

Right now yes -- our support for other architectures like x86_64 will also include migrating to QNX 7.1, which you have already implemented support for. But the main reason is that I don't have any 7.0 images ready to test with on architectures besides x86, and I think the nice workflow with qemu and mkqnximage was added in SDP 7.1, so that doesn't help for 7.0. If you know of a quick way for me to spin up an emulated QNX 7.0 test environment for x86_64 and aarch64, I'd be happy to include them.

Just took a peak and the source code for mkqnximage exists on the 700 branch and I use it internally on 7.0. I can take a look in swcenter for you and see if it's available. Do you use the email first.last@company.com as your myqnx login? If not, you can fire me an email with the login name you use. It lets me see what's available from your perspective.

I apologize for bringing this up as I realize it's a little extra work. If it doesn't end up being easy from your side, I can add the targets and update this diff.

@flba-eb
Copy link
Owner

flba-eb commented Mar 14, 2023

@flba-eb Thanks for the feedback! 2 questions:

  1. When opening the PR against the rust repo, should I fill out the target policy criteria again like was done here: Add tier 3 no_std AArch64/x86_64 support for the QNX Neutrino RTOS rust-lang/rust#102701, or is it not necessary since QNX support in general has already been merged?

No, this is not required. As you pointed out, QNX support is already there.

@gh-tr
Copy link

gh-tr commented Mar 15, 2023

Just took a peak and the source code for mkqnximage exists on the 700 branch and I use it internally on 7.0. I can take a look in swcenter for you and see if it's available. Do you use the email first.last@company.com as your myqnx login? If not, you can fire me an email with the login name you use. It lets me see what's available from your perspective.

I apologize for bringing this up as I realize it's a little extra work. If it doesn't end up being easy from your side, I can add the targets and update this diff.

It appears that 7.0 mkqnximage is for internal use only. Just go ahead with x86. Other targets can be added another time.

@samkearney
Copy link
Collaborator Author

It appears that 7.0 mkqnximage is for internal use only. Just go ahead with x86. Other targets can be added another time.

OK! I will close this PR and reopen it against mainline Rust. Thanks for the review, guys

@samkearney samkearney closed this Mar 15, 2023
samkearney pushed a commit that referenced this pull request Jun 2, 2023
Stop turning transmutes into discriminant reads in mir-opt

Partially reverts rust-lang#109612, as after rust-lang#109993 these aren't actually equivalent any more, and I'm no longer confident this was ever an improvement in the first place.

Having this "simplification" meant that similar-looking code actually did somewhat different things.  For example,
```rust
pub unsafe fn demo1(x: std::cmp::Ordering) -> u8 {
    std::mem::transmute(x)
}
pub unsafe fn demo2(x: std::cmp::Ordering) -> i8 {
    std::mem::transmute(x)
}
```
in nightly today is generating <https://rust.godbolt.org/z/dPK58zW18>
```llvm
define noundef i8 `@_ZN7example5demo117h341ef313673d2ee6E(i8` noundef %x) unnamed_addr #0 {
  %0 = icmp uge i8 %x, -1
  %1 = icmp ule i8 %x, 1
  %2 = or i1 %0, %1
  call void `@llvm.assume(i1` %2)
  ret i8 %x
}

define noundef i8 `@_ZN7example5demo217h5ad29f361a3f5700E(i8` noundef %0) unnamed_addr #0 {
  %x = alloca i8, align 1
  store i8 %0, ptr %x, align 1
  %1 = load i8, ptr %x, align 1, !range !2, !noundef !3
  ret i8 %1
}
```

Which feels too different when the original code is essentially identical.

---

Aside: that example is different *after* optimizations too:
```llvm
define noundef i8 `@_ZN7example5demo117h341ef313673d2ee6E(i8` noundef returned %x) unnamed_addr #0 {
  %0 = add i8 %x, 1
  %1 = icmp ult i8 %0, 3
  tail call void `@llvm.assume(i1` %1)
  ret i8 %x
}

define noundef i8 `@_ZN7example5demo217h5ad29f361a3f5700E(i8` noundef returned %0) unnamed_addr #1 {
  ret i8 %0
}
```
so turning the `Transmute` into a `Discriminant` was arguably just making things worse, so leaving it alone instead -- and thus having less code in rustc -- seems clearly better.
samkearney pushed a commit that referenced this pull request Jun 2, 2023
Fixes rust-lang#111510 and complements rust-lang#106547 by adding support for encoding
type parameters and also by transforming trait objects' traits into
their identities before emitting type checks.
samkearney pushed a commit that referenced this pull request Jun 2, 2023
CFI: Fix encode_ty: unexpected Param(B/#1)

Fixes rust-lang#111510 and complements rust-lang#106547 by adding support for encoding type parameters and also by transforming trait objects' traits into their identities before emitting type checks.
flba-eb pushed a commit that referenced this pull request Dec 12, 2024
we get these declarations

```
; opt level 0
declare x86_intrcc void @page_fault_handler(ptr byval([8 x i8]) align 8, i64) unnamed_addr #1
; opt level > 0
declare x86_intrcc void @page_fault_handler(ptr noalias nocapture noundef byval([8 x i8]) align 8 dereferenceable(8), i64 noundef) unnamed_addr #1
```

The space after `i64` in the original regex made the regex not match for
opt level 0. Removing the space fixes the issue.

```
declare x86_intrcc void @page_fault_handler(ptr {{.*}}, i64 {{.*}}){{.*}}#[[ATTRS:[0-9]+]]
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants