Skip to content

Conversation

@farao
Copy link

@farao farao commented Jun 9, 2025

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

  • Relevant tests in libc-test/semver have been updated
  • 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 Jun 9, 2025

Some changes occurred in the Android module

cc @maurer

@farao farao force-pushed the revert branch 11 times, most recently from 92284e5 to 40a082a Compare June 12, 2025 16:33
@farao
Copy link
Author

farao commented Jun 13, 2025

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.

@farao farao force-pushed the revert branch 2 times, most recently from 26e9993 to 5b60e70 Compare June 23, 2025 09:15
Copy link
Contributor

@tgross35 tgross35 left a 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.

@tgross35
Copy link
Contributor

tgross35 commented Jul 7, 2025

@rustbot author, for the above review and a rebase

@rustbot
Copy link
Collaborator

rustbot commented Jul 7, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rustbot
Copy link
Collaborator

rustbot commented Nov 20, 2025

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@farao farao force-pushed the revert branch 5 times, most recently from 61f844b to b64b370 Compare November 20, 2025 12:39
@farao
Copy link
Author

farao commented Nov 20, 2025

@rustbot ready

use crate::prelude::*;

pub type l4_umword_t = c_ulong; // Unsigned machine word.
pub type pthread_t = *mut c_void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does this come from? From https://github.com/search?q=repo%3Akernkonzept%2Fl4re-core+%2Ftypedef.*pthread_t%3B%2F&type=code, it seems like they are all c_ulong, like upstream uclibc. (Not that it really matters too much.)

Copy link
Author

Choose a reason for hiding this comment

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

I will try to investigate (since changing it brings me a warning). I'd like to leave it for another PR if that's ok.

Comment on lines +224 to +248
#[cfg_attr(any(target_pointer_width = "32",), repr(align(4)))]
#[cfg_attr(not(any(target_pointer_width = "32",)), repr(align(8)))]
pub struct pthread_mutexattr_t {
#[doc(hidden)]
size: [u8; crate::__SIZEOF_PTHREAD_MUTEXATTR_T],
}

#[cfg_attr(any(target_pointer_width = "32"), repr(align(4)))]
#[cfg_attr(all(target_pointer_width = "64"), repr(align(8)))]
pub struct pthread_rwlockattr_t {
#[doc(hidden)]
size: [u8; crate::__SIZEOF_PTHREAD_RWLOCKATTR_T],
}

#[repr(align(4))]
pub struct pthread_condattr_t {
#[doc(hidden)]
size: [u8; crate::__SIZEOF_PTHREAD_CONDATTR_T],
}

#[repr(align(4))]
pub struct pthread_barrierattr_t {
#[doc(hidden)]
size: [u8; crate::__SIZEOF_PTHREAD_BARRIERATTR_T],
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm looking at the definitions here https://github.com/kernkonzept/l4re-core/blob/e1bf5beeb2933af404b03ca1571e82f33c3385c2/libc/uclibc-ng/libpthread/include/bits/pthreadtypes.h and they don't seem to match up at all - is that the wrong place?

Copy link
Author

@farao farao Nov 21, 2025

Choose a reason for hiding this comment

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

No, the definitions are different. I thought it might have been a concious design decision in the linux part of the code to commonize the pthread structs by just putting an array of bytes into them so I kept this (especially because some time in the near future we might support other libcs). But I'm also happy to choose the actual struct definitions if you prefer this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh no it wasn't a deviant decision, that's actually how they're defined in the linux libs https://github.com/bminor/glibc/blob/f9e61cd446d45016e20b6fe85ab87364ebdbec1b/sysdeps/nptl/bits/pthreadtypes.h#L67-L72. I would indeed prefer to match the struct definitions (with all fields private). It's much easier to make sure the fields line up than to add manually compute the size/align, and I don't know that the sizes are likely to always stay the same across platforms anyway.

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).
@farao
Copy link
Author

farao commented Nov 21, 2025

@rustbot ready

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

Still some leftovers in the pthread definitions, and I would like the uclibc pthread defs to match source rather than using the array. But after this, LGTM!

Comment on lines +224 to +248
#[cfg_attr(any(target_pointer_width = "32",), repr(align(4)))]
#[cfg_attr(not(any(target_pointer_width = "32",)), repr(align(8)))]
pub struct pthread_mutexattr_t {
#[doc(hidden)]
size: [u8; crate::__SIZEOF_PTHREAD_MUTEXATTR_T],
}

#[cfg_attr(any(target_pointer_width = "32"), repr(align(4)))]
#[cfg_attr(all(target_pointer_width = "64"), repr(align(8)))]
pub struct pthread_rwlockattr_t {
#[doc(hidden)]
size: [u8; crate::__SIZEOF_PTHREAD_RWLOCKATTR_T],
}

#[repr(align(4))]
pub struct pthread_condattr_t {
#[doc(hidden)]
size: [u8; crate::__SIZEOF_PTHREAD_CONDATTR_T],
}

#[repr(align(4))]
pub struct pthread_barrierattr_t {
#[doc(hidden)]
size: [u8; crate::__SIZEOF_PTHREAD_BARRIERATTR_T],
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh no it wasn't a deviant decision, that's actually how they're defined in the linux libs https://github.com/bminor/glibc/blob/f9e61cd446d45016e20b6fe85ab87364ebdbec1b/sysdeps/nptl/bits/pthreadtypes.h#L67-L72. I would indeed prefer to match the struct definitions (with all fields private). It's much easier to make sure the fields line up than to add manually compute the size/align, and I don't know that the sizes are likely to always stay the same across platforms anyway.

Comment on lines -1490 to +1112
size: [u8; crate::__SIZEOF_PTHREAD_COND_T],
pub size: [u8; crate::__SIZEOF_PTHREAD_COND_T],
Copy link
Contributor

Choose a reason for hiding this comment

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

Please no!

Comment on lines 1082 to 1110
#[cfg_attr(
all(
any(target_env = "musl", target_env = "ohos"),
any(target_env = "musl", target_env = "ohos", target_os = "emscripten"),
target_pointer_width = "32"
),
repr(align(4))
)]
#[cfg_attr(
all(
any(target_env = "musl", target_env = "ohos"),
any(target_env = "musl", target_env = "ohos", target_os = "emscripten"),
target_pointer_width = "64"
),
repr(align(8))
)]
#[cfg_attr(
all(
not(any(target_env = "musl", target_env = "ohos")),
not(any(target_env = "musl", target_env = "ohos", target_os = "emscripten")),
target_arch = "x86"
),
repr(align(4))
)]
#[cfg_attr(
all(
not(any(target_env = "musl", target_env = "ohos")),
not(any(target_env = "musl", target_env = "ohos", target_os = "emscripten")),
not(target_arch = "x86")
),
repr(align(8))
)]
pub struct pthread_cond_t {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please make sure to self-review these pthread diffs? Again this has some changes for emscripten that aren't relevant.

Comment on lines 1125 to 1132
target_arch = "powerpc",
target_arch = "sparc",
target_arch = "x86_64",
target_arch = "x86"
target_arch = "x86",
target_os = "emscripten",
)
),
repr(align(4))
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto, drop this

Comment on lines 1144 to 1165
target_arch = "powerpc",
target_arch = "sparc",
target_arch = "x86_64",
target_arch = "x86"
target_arch = "x86",
target_os = "emscripten",
))
),
repr(align(8))
)]
pub struct pthread_mutex_t {
#[doc(hidden)]
size: [u8; crate::__SIZEOF_PTHREAD_MUTEX_T],
pub size: [c_char; crate::__SIZEOF_PTHREAD_MUTEX_T],
}

#[cfg_attr(
all(
target_pointer_width = "32",
any(
target_os = "emscripten",
target_arch = "mips",
target_arch = "mips32r6",
target_arch = "arm",
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Comment on lines 1178 to 1184
any(
target_pointer_width = "64",
not(any(
target_os = "emscripten",
target_arch = "mips",
target_arch = "mips32r6",
target_arch = "arm",
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Comment on lines 1193 to 1199
repr(align(8))
)]
pub struct pthread_rwlock_t {
size: [u8; crate::__SIZEOF_PTHREAD_RWLOCK_T],
pub size: [u8; crate::__SIZEOF_PTHREAD_RWLOCK_T],
}

#[cfg_attr(
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

tgross35 added a commit to tgross35/rust-libc that referenced this pull request Dec 2, 2025
This is imperfect but should get things closer to what is available on
l4re's uclibc fork. Based on work by Marius Melzer [rust-lang#4479].

[rust-lang#4479]: rust-lang#4479

(backport <rust-lang#4836>)
(cherry picked from commit 34d8ce7)
github-merge-queue bot pushed a commit that referenced this pull request Dec 2, 2025
This is imperfect but should get things closer to what is available on
l4re's uclibc fork. Based on work by Marius Melzer [#4479].

[#4479]: #4479

(backport <#4836>)
(cherry picked from commit 34d8ce7)
github-merge-queue bot pushed a commit that referenced this pull request Dec 2, 2025
This is imperfect but should get things closer to what is available on
l4re's uclibc fork. Based on work by Marius Melzer [#4479].

[#4479]: #4479

(backport <#4836>)
(cherry picked from commit 34d8ce7)
github-merge-queue bot pushed a commit that referenced this pull request Dec 2, 2025
This is imperfect but should get things closer to what is available on
l4re's uclibc fork. Based on work by Marius Melzer [#4479].

[#4479]: #4479

(backport <#4836>)
(cherry picked from commit 34d8ce7)
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