-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Separate L4Re from Linux code, add aarch64 and enable tests #4479
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
base: main
Are you sure you want to change the base?
Conversation
Some changes occurred in the Android module cc @maurer |
92284e5
to
40a082a
Compare
Hi @tgross35, this is the refactoring of the recent L4Re PR: #4383 (which is kept around just in case). I think the failure of the freebsd nightly checks is not my fault. The rest succeeds now. I did most of what you requested in your comment on the old PR, I just kept the linux/mod.rs file around since there is a massive part of code that is linux-only and is not supported by L4Re and it did not seem to make sense to put it all in shared.rs. Also, I put some more code that's shared from the sub modules (emscripten, android, linux, l4re) up in linux_like/mod.rs. In theory, I think there's potential to do that with more code, that's just the one that I would have put in shared.rs but realized that it could go into linux_like/mod.rs instead. |
26e9993
to
5b60e70
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry this has taken a while to get to, but thank you for all the changes here! The shape of this one looks much better. I have a handful of small comments but will need to take a deeper look at the big refactor portions again.
ci/run-docker.sh
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this isn't directly related to l4re, mind splitting it to a separate commit? (same PR is fine of course)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
libc-test/build.rs
Outdated
@@ -3990,6 +4007,10 @@ fn test_linux(target: &str) { | |||
}); | |||
|
|||
cfg.skip_struct(move |ty| { | |||
if ty.starts_with("l4_") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these anonymous on l4re?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually turned out to be not necessary. I removed this and similar lines.
libc-test/build.rs
Outdated
@@ -4169,6 +4190,12 @@ fn test_linux(target: &str) { | |||
}); | |||
|
|||
cfg.skip_const(move |name| { | |||
// L4Re requires a min stack size of 64k; that isn't defined in uClibc, but | |||
// somewhere in the core libraries. uClibc wants 16k, but that's not enough. | |||
if name == "PTHREAD_STACK_MIN" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be l4re && ...
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, done.
libc-test/build.rs
Outdated
@@ -3716,11 +3728,12 @@ fn test_linux(target: &str) { | |||
"libgen.h", | |||
"limits.h", | |||
"link.h", | |||
"linux/sysctl.h", | |||
[uclibc]: "linux/if_ether.h", | |||
[linux]: "linux/sysctl.h", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is probably safer to flip the logic here. That is, rather than [linux]
, make these [!l4re]
so any other OS that happens to reuse some of this function gets the same defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, that's better! I changed it.
libc-test/build.rs
Outdated
@@ -4920,7 +4949,9 @@ fn test_linux(target: &str) { | |||
|
|||
cfg.generate(src_hotfix_dir().join("lib.rs"), "main.rs"); | |||
|
|||
test_linux_like_apis(target); | |||
if linux { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, if !l4re
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
(*(self as *const siginfo_t as *const siginfo_sigfault)).si_addr | ||
} | ||
|
||
pub unsafe fn si_value(&self) -> crate::sigval { | ||
#[repr(C)] | ||
struct siginfo_si_value { | ||
_si_signo: c_int, | ||
_si_errno: c_int, | ||
_si_code: c_int, | ||
_si_timerid: c_int, | ||
_si_overrun: c_int, | ||
si_value: crate::sigval, | ||
} | ||
(*(self as *const siginfo_t as *const siginfo_si_value)).si_value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is preexisting, but mind updating to the slightly safer newer casting APIs? So ptr::from_ref(self).cast::<siginfo_sigfault>()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
// FIXME(1.0): this is actually a union | ||
pub struct sem_t { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ulibc was broken before this right? If so, might as well make it correct here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that was broken before. I fixed it now.
src/unix/linux_like/shared.rs
Outdated
@@ -0,0 +1,2127 @@ | |||
//! Shared definitions between Linux and L4Re | |||
|
|||
// TODO: check if there is more overlap with emscripten and android |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FIXME(linux)
if this comment is meant to be left in the codebase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used the FIXME. There are actually a lot of cases where the same struct that is shared between l4re and linux is also shared with one of the other targets (android or emscripten) and one might think about adding all these cases to linux_like/mod.rs instead (with the exception of the differing target as #[cfg..].
@rustbot author, for the above review and a rebase |
Reminder, once the PR becomes ready for a review, use |
Thanks for the comments! Unfortunately, I'll only be able to get to them beginning of next week but I'll make it a priority then! |
c2490f3
to
804f6f9
Compare
@tgross35 Should be all fixed now. CI runs through (the failing freebsd checks are again not due to my change). Anything else? |
@tgross35 any chance that we can get this into the next release? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A handful more requests here then I think this should be good to go.
If there is a bit more commit splitting that can be done here, that would be helpful for the sake of better history and slightly easier cherry picks. E.g. move emscripten+android items to linux_like
in one, add the new l4re
module in another, create shared
in a third.
src/unix/linux_like/l4re/mod.rs
Outdated
#[allow(deprecated)] | ||
#[deprecated(since = "0.2.64", note = "Not stable across OS versions")] | ||
pub const RLIMIT_NLIMITS: crate::__rlimit_resource_t = RLIM_NLIMITS; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is l4re-only, it can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. I removed the deprecation warning instead, since L4Re's uclibc configuration has the define and I did not find a deprecation warning there.
src/unix/linux_like/mod.rs
Outdated
#[cfg_attr( | ||
any( | ||
target_env = "musl", | ||
target_env = "ohos", | ||
target_env = "uclibc", | ||
target_pointer_width = "32" | ||
), | ||
repr(align(4)) | ||
)] | ||
#[cfg_attr( | ||
all( | ||
not(target_env = "musl"), | ||
not(target_env = "ohos"), | ||
not(target_env = "uclibc"), | ||
target_pointer_width = "64" | ||
), | ||
repr(align(8)) | ||
)] | ||
#[cfg(not(target_os = "android"))] | ||
pub struct pthread_rwlockattr_t { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
emscripten also needs 4-alignment right? Same for the other structs in this hunk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I missed that, thanks. I added it.
src/unix/linux_like/mod.rs
Outdated
@@ -349,7 +595,7 @@ cfg_if! { | |||
u64.hash(state); | |||
} | |||
} | |||
|
|||
} else if #[cfg(feature = "extra_traits")] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't equivalent right? These used to be emitted unconditionally, but now are only if the all(feature = "extra_traits", not(target_os = "l4re"))
condition above did not match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I split it up into two blocks.
#[cfg(not(target_os = "android"))] | ||
pub const PTHREAD_MUTEX_INITIALIZER: pthread_mutex_t = pthread_mutex_t { | ||
size: [0; crate::__SIZEOF_PTHREAD_MUTEX_T], | ||
}; | ||
#[cfg(not(target_os = "android"))] | ||
pub const PTHREAD_COND_INITIALIZER: pthread_cond_t = pthread_cond_t { | ||
size: [0; crate::__SIZEOF_PTHREAD_COND_T], | ||
}; | ||
#[cfg(not(any(target_os = "android", target_env = "uclibc")))] | ||
pub const PTHREAD_RWLOCK_INITIALIZER: pthread_rwlock_t = pthread_rwlock_t { | ||
size: [0; crate::__SIZEOF_PTHREAD_RWLOCK_T], | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weren't these emitted on Android before? Also, looks like they moved up in the file (used to be around 2717)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are present on android (in b32/mod.rs and b64/mod.rs) but implemented differently, therefore I left them out here. Before, they were in another file (emscripten/mod.rs and linux/mod.rs) afaics.
src/unix/linux_like/shared.rs
Outdated
@@ -0,0 +1,2127 @@ | |||
//! Shared definitions between Linux and L4Re |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe call this file linux_l4re_shared
to match the description?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/unix/linux_like/shared.rs
Outdated
|
||
// FIXME(linux): check if there is more overlap with emscripten and android | ||
|
||
use core::mem::size_of; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should already come via the prelude
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
src/unix/linux_like/shared.rs
Outdated
pub struct __c_anonymous_ifru_map { | ||
pub mem_start: c_ulong, | ||
pub mem_end: c_ulong, | ||
pub base_addr: c_ushort, | ||
pub irq: c_uchar, | ||
pub dma: c_uchar, | ||
pub port: c_uchar, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was in s!
rather than s_no_extra_traits
before right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, fixed.
impl PartialEq for dirent { | ||
fn eq(&self, other: &dirent) -> bool { | ||
self.d_ino == other.d_ino | ||
&& self.d_off == other.d_off | ||
&& self.d_reclen == other.d_reclen | ||
&& self.d_type == other.d_type | ||
&& self | ||
.d_name | ||
.iter() | ||
.zip(other.d_name.iter()) | ||
.all(|(a, b)| a == b) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this trait impl (and the others below it) in a separate module from the structs they go with?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because the types of the fields of dirent are different by os (but still they can share the trait implementation). But maybe that's overshooting the mark for code deduplication? Let me know if I shall put them together with their struct implementations again.
src/unix/linux_like/shared.rs
Outdated
pub const ABDAY_1: crate::nl_item = base; | ||
pub const ABDAY_2: crate::nl_item = base + 0x1; | ||
pub const ABDAY_3: crate::nl_item = base + 0x2; | ||
pub const ABDAY_4: crate::nl_item = base + 0x3; | ||
pub const ABDAY_5: crate::nl_item = base + 0x4; | ||
pub const ABDAY_6: crate::nl_item = base + 0x5; | ||
pub const ABDAY_7: crate::nl_item = base + 0x6; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were gated on gnu || musl || ohos
before. Is that not needed anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that's not needed anymore. uclibc has them too, just starting at a different number. So I introduced the base variable which is still different between uclibc and not uclibc.
80cafff
to
67e08ed
Compare
The L4Re code was previously attached to the Linux code which was not correct in many ways. This commit separates the L4Re code and enables the libc-tests and includes the fixes for the failing tests. Aarch64 is added as a second supported architecture (more to come).
The L4Re code was previously attached to the Linux code which was not correct in many ways. This commit separates the L4Re code and enables the libc-tests and includes the fixes for the failing tests. Aarch64 is added as a second supported architecture (more to come).
Sources
L4Re-adapted version of uclibc: https://github.com/kernkonzept/l4re-core/tree/master/uclibc/lib.
Checklist
libc-test/semver
have been updated*LAST
or*MAX
areincluded (see #3131)
cd libc-test && cargo test --target mytarget
);especially relevant for platforms that may not be checked in CI