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

linux: add missing tls bindings #4296

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mbyx
Copy link

@mbyx mbyx commented Mar 4, 2025

Description

Adds missing TLS bindings for linux as mentioned in #3149. Tests should pass as long as the kernel used for testing implements them.

Sources

Previously only a part of the header had been implemented, now everything has been bound except for placeholder values (__TLS_INFO_MAX, TLS_INFO_MAX).
https://github.com/torvalds/linux/blob/99fa936e8e4f117d62f229003c9799686f74cebc/include/uapi/linux/tls.h

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
    I was unable to test locally due to errors similar to Unable to run tests in libc-test using musl (Alpine Linux 3.18.2) #3305.

@rustbot
Copy link
Collaborator

rustbot commented Mar 4, 2025

r? @tgross35

rustbot has assigned @tgross35.
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

@tgross35
Copy link
Contributor

tgross35 commented Mar 4, 2025

Permalink to the headers in their current state https://github.com/torvalds/linux/blob/99fa936e8e4f117d62f229003c9799686f74cebc/include/uapi/linux/tls.h (you can get these from the triple dot menu at the top right of the file view).

The CI failure just needs you to sort libc-test/semver/linux.txt, FreeBSD is spurious.

@mbyx
Copy link
Author

mbyx commented Mar 4, 2025

The test that failed gives the same errors that I experienced when testing my commit locally. Is it the CI system or something wrong with my implementation? It also cancelled the rest, so I can't check if the problem will occur for them all.

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.

LGTM and the CI problem should be fixed with #4311. Could you rebase and squash?

@mbyx mbyx force-pushed the missing-tls-bindings branch from ab54eca to fc84ede Compare March 10, 2025 12:51
@tgross35
Copy link
Contributor

Hm, are these headers just not available on loongarch musl for some reason? Cc @heiher in case you know something.

@heiher
Copy link
Contributor

heiher commented Mar 11, 2025

Hm, are these headers just not available on loongarch musl for some reason? Cc @heiher in case you know something.

Thanks for your CC. I have pushed a PR (#4320) for that, and I believe it has fixed this issue.

@mbyx mbyx force-pushed the missing-tls-bindings branch from fc84ede to f7235c3 Compare March 15, 2025 12:34
@mbyx mbyx force-pushed the missing-tls-bindings branch from f7235c3 to e8d564e Compare March 22, 2025 15:46
@tgross35
Copy link
Contributor

This should work now, @mbyx could you squash please?

@mbyx mbyx force-pushed the missing-tls-bindings branch 3 times, most recently from 30edaf5 to e0153ff Compare March 23, 2025 05:49
@mbyx
Copy link
Author

mbyx commented Mar 23, 2025

Sorry, but this is my first time having to squash, I think I'm not doing it properly, could you please guide me?

sort semver/linux.txt properly
@mbyx mbyx force-pushed the missing-tls-bindings branch from e0153ff to bdcb3eb Compare March 23, 2025 14:25
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