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

Fix Os/Posix/Task.cpp compilation with musl #3145

Merged
merged 1 commit into from
Jan 21, 2025
Merged

Conversation

celskeggs
Copy link
Contributor

Related Issue(s) see below
Has Unit Tests (y/n) no
Documentation Included (y/n) no

Change Description

Limit the use of pthread_attr_setaffinity_np to platforms that we expect to support it. This is a non-POSIX function and should not be relied on for any generic POSIX code. This change adds back previously removed preprocessor defines to determine whether it can be used.

Rationale

As discussed in #1507, musl does not support pthread_attr_setaffinity_np, since it does not promise to support non-POSIX APIs. It appears that this was fixed in #1517, but subsequently broken in #2672. I was not able to find any reason explained on #2672 why it was changed.

Notably, before this pull request, the following directive was used:

#if TGT_OS_TYPE_LINUX && __GLIBC__

After the pull request, this directive was used instead:

#ifdef _GNU_SOURCE

This is unfortunately not an adequate replacement. C++ programs compiled by clang automatically have the _GNU_SOURCE macro declared, even when using a non-GNU standard like --std=c++14. It appears that this is based on g++'s behavior, which is to support libstdc++ as described at https://stackoverflow.com/a/11680100. In other words, using musl does not result in _GNU_SOURCE remaining undefined, because _GNU_SOURCE is defined by the compiler, not the standard library. However, it is true that if _GNU_SOURCE is NOT defined (e.g. if -U_GNU_SOURCE is passed on the command line), the API will not be published. So it may be necessary to check for _GNU_SOURCE being defined, but it is not sufficient.

Therefore, I propose to combine these checks as follows:

#if defined(TGT_OS_TYPE_LINUX) && defined(__GLIBC__) && defined(_GNU_SOURCE)

Testing/Review Recommendations

Someone who has a glibc-based project that relies on CPU affinity should verify that this doesn't break their path. (I will also want to do a re-test myself for the musl case; I've only tested with #ifdef __GLIBC__, not with the full line that I'm now proposing.)

Future Work

It is unclear whether musl will add support for this API in the future. The explanation of why they don't is given at https://musl.openwall.narkive.com/28kFmu49/pthread-attr-g-s-etaffinity-np. To summarize, implementing the API appears to require unbounded storage, and they don't think it's useful, because the affinity can be changed once the thread is started (by the thread itself). It is not clear to me whether this is actually adequate for F Prime's use case (for systems that actually require CPU affinity), but maybe musl is not appropriate for hard real-time environments anyway? I'm not sure what the right thing is for musl to do. Really, if the function is truly necessary, an appropriate variant should be added to a new version of the POSIX standard.

Other projects appear to deal with this problem by using a configure script to check whether pthread_attr_setaffinity_np is defined or not. We could consider doing that too, since checking for features is part of what CMake is supposed to do when generating build scripts. However, it isn't clear to me whether a configuration check would add any value over the solution in this PR, and it would certainly be more complicated. So I will hold off on recommending that as a long-term solution.

// pthread_attr_setaffinity_np is a non-POSIX function. Notably, it is not available on musl.
// Limit its use to builds that involve glibc, on Linux, with _GNU_SOURCE defined.
// That's the circumstance in which we expect this feature to work.
#if defined(TGT_OS_TYPE_LINUX) && defined(__GLIBC__) && defined(_GNU_SOURCE)

Check notice

Code scanning / CodeQL

Conditional compilation Note

Use of conditional compilation must be kept to a minimum.
@LeStarch LeStarch merged commit ac2c994 into nasa:devel Jan 21, 2025
36 checks passed
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