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 teeos std impl #116565

Merged
merged 1 commit into from
Dec 7, 2023
Merged

add teeos std impl #116565

merged 1 commit into from
Dec 7, 2023

Conversation

Sword-Destiny
Copy link
Contributor

@Sword-Destiny Sword-Destiny commented Oct 9, 2023

add teeos std library implement.

this MR is draft untill the libc update to 0.2.150

this MR is the final step for suppot rust in teeos.
first step(add target): #113480
second step(add teeos libc): rust-lang/libc#3333

@rustbot
Copy link
Collaborator

rustbot commented Oct 9, 2023

r? @cuviper

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 9, 2023
@rust-log-analyzer

This comment has been minimized.

@Sword-Destiny
Copy link
Contributor Author

r? @petrochenkov

@petrochenkov
Copy link
Contributor

untill the libc update to 0.2.150

Do you mean 0.2.149? That seems to be enough.
Blocked on #116518 or #116527.
@rustbot blocked

@rustbot rustbot added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 9, 2023
@petrochenkov petrochenkov marked this pull request as ready for review October 9, 2023 15:20
@rustbot
Copy link
Collaborator

rustbot commented Oct 9, 2023

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@petrochenkov
Copy link
Contributor

All the changes are under library/std and library/panic_abort.
r? library

@rustbot
Copy link
Collaborator

rustbot commented Oct 9, 2023

Failed to set assignee to library: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

@petrochenkov
Copy link
Contributor

r? rust-lang/libs-impl
r? rust-lang/libs

@rustbot
Copy link
Collaborator

rustbot commented Oct 9, 2023

Team or group rust-lang/libs-impl not found.

rust-lang team names can be found at https://github.com/rust-lang/team/tree/master/teams.
Reviewer group names can be found in triagebot.toml in this repo.

@rustbot rustbot assigned thomcc and unassigned petrochenkov Oct 9, 2023
@Sword-Destiny
Copy link
Contributor Author

untill the libc update to 0.2.150

Do you mean 0.2.149? That seems to be enough. Blocked on #116518 or #116527. @rustbot blocked

some code merged after 0.2.149, so I need to wait 0.2.150
rust-lang/libc#3379

@petrochenkov
Copy link
Contributor

cc @m-ou-se @Amanieu this is the library PR I mentioned.

@@ -0,0 +1,348 @@
//! copy from unix 100%, version 1.65
Copy link
Member

Choose a reason for hiding this comment

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

If this is an exact copy of unix/time.rs, it might be better to just import that file directly from mod.rs:

#[path = "../unix/time.rs"]
pub mod time;

Copy link
Member

Choose a reason for hiding this comment

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

(And similar for thread_local_key.rs and perhaps more.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

some code under unix is not allowed to be imported

Copy link
Contributor Author

Choose a reason for hiding this comment

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

time.rs can not be imported, thread_local_key.rs is optimized.

Copy link
Member

Choose a reason for hiding this comment

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

It seems the issue is due to pub(in crate::sys::unix) in the unix implementation. Maybe it would be better to just change that to pub(crate) to allow the code to be reused (timespec is very widespread outside "unix" targets).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I imported unix timespec from the beginning, if it could be a pub(crate) I would be more than happy to do that.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please just change the Unix code to use pub(crate) to avoid the code duplication.

} else if #[cfg(target_os = "teeos")] {
mod teeos;
pub use self::teeos::*;
use teeos as unsupported;
Copy link
Member

Choose a reason for hiding this comment

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

What is this line for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

accepted

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 line should be removed

@bors
Copy link
Contributor

bors commented Nov 7, 2023

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

@rustbot
Copy link
Collaborator

rustbot commented Nov 11, 2023

These commits modify compiler targets.
(See the Target Tier Policy.)

@Sword-Destiny
Copy link
Contributor Author

@thomcc

@Sword-Destiny
Copy link
Contributor Author

Is there any more questions?

library/std/src/sys/teeos/memchr.rs Outdated Show resolved Hide resolved
library/std/src/sys/teeos/condvar.rs Outdated Show resolved Hide resolved
library/std/src/sys/teeos/mod.rs Show resolved Hide resolved
@@ -0,0 +1,43 @@
pub struct RwLock {}
Copy link
Member

Choose a reason for hiding this comment

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

It is better to implement this as a Mutex than not supporting RwLock at all. Even though it won't allow concurrent reads, it will still allow the type to work correctly.

Copy link
Contributor Author

@Sword-Destiny Sword-Destiny Dec 6, 2023

Choose a reason for hiding this comment

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

I think I can use Cell for rwlock like unsupported/locks/rwlock.rs.

Copy link
Member

Choose a reason for hiding this comment

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

No, the Cell implementation is only correct for systems without thread support. You need to implement it using a mutex.

Copy link
Contributor Author

@Sword-Destiny Sword-Destiny Dec 6, 2023

Choose a reason for hiding this comment

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

Then I prefer not to support rwlock and tell users in the manual that we do not support rwlock, for it's really different from mutex and rwlock interfaces. And a little hard :-) , I will try.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I overestimated the problem

Copy link
Member

Choose a reason for hiding this comment

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

Can you change the code to wrap Mutex instead of duplicating all the logic around pthread_mutex_t?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I originally thought I needed to do it through libc's interface. Is this the effect you expected?

use crate::sys::locks::mutex::Mutex;

/// we do not supported rwlock, so use mutex to simulate rwlock.
/// it's useful because so many code in std will use rwlock.
pub struct RwLock {
    inner: Mutex,
}

impl RwLock {
    #[inline]
    pub const fn new() -> RwLock {
        RwLock { inner: Mutex::new() }
    }

    #[inline]
    pub fn read(&self) {
        unsafe { self.inner.lock() };
    }

    #[inline]
    pub fn try_read(&self) -> bool {
        unsafe { self.inner.try_lock() }
    }

    #[inline]
    pub fn write(&self) {
        unsafe { self.inner.lock() };
    }

    #[inline]
    pub unsafe fn try_write(&self) -> bool {
        unsafe { self.inner.try_lock() }
    }

    #[inline]
    pub unsafe fn read_unlock(&self) {
        unsafe { self.inner.unlock() };
    }

    #[inline]
    pub unsafe fn write_unlock(&self) {
        unsafe { self.inner.unlock() };
    }
}

library/std/src/sys/teeos/thread.rs Show resolved Hide resolved
@@ -0,0 +1,348 @@
//! copy from unix 100%, version 1.65
Copy link
Member

Choose a reason for hiding this comment

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

It seems the issue is due to pub(in crate::sys::unix) in the unix implementation. Maybe it would be better to just change that to pub(crate) to allow the code to be reused (timespec is very widespread outside "unix" targets).

@bors
Copy link
Contributor

bors commented Dec 6, 2023

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

@rustbot rustbot added the O-unix Operating system: Unix-like label Dec 6, 2023
@rust-log-analyzer

This comment has been minimized.

Signed-off-by: 袁浩 <yuanhao34@huawei.com>
@Amanieu
Copy link
Member

Amanieu commented Dec 7, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Dec 7, 2023

📌 Commit e353eb9 has been approved by Amanieu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Dec 7, 2023
@bors
Copy link
Contributor

bors commented Dec 7, 2023

⌛ Testing commit e353eb9 with merge 568f6a8...

@bors
Copy link
Contributor

bors commented Dec 7, 2023

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 568f6a8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 7, 2023
@bors bors merged commit 568f6a8 into rust-lang:master Dec 7, 2023
12 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 7, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (568f6a8): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.3% [-1.3%, -1.3%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-3.3% [-5.1%, -1.5%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -3.3% [-5.1%, -1.5%] 2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.9% [0.9%, 0.9%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.9% [0.9%, 0.9%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 675.253s -> 675.831s (0.09%)
Artifact size: 314.17 MiB -> 314.22 MiB (0.02%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. O-unix Operating system: Unix-like S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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