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

Require C11 #12660

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Require C11 #12660

wants to merge 6 commits into from

Conversation

devreal
Copy link
Contributor

@devreal devreal commented Jul 9, 2024

Error out if the compiler does not support C11 and limit the standard to C11 if the compiler accepts a standard flag. This limit prevents us from using features of newer standard versions.

Also removes support for __thread since we now rely on C11 _Thread_local.

Please check my m4 code, not an expert on this.

See #12658 (review) for notes from the RM discussion and the PR that initiated this.

There will likely be more changes over time replacing pre-C11 features with C11 features where possible but I wanted to keep this manageable.

@jsquyres where should I put a note about this for users?

Copy link
Member

@bosilca bosilca left a comment

Choose a reason for hiding this comment

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

Not for 5.x

@devreal
Copy link
Contributor Author

devreal commented Jul 9, 2024

Mhh, CI fails with this error:

  LEX      keyval_lex.c
  CC       keyval_lex.lo
keyval_lex.c: In function ‘opal_util_keyval_yy_init_buffer’:
keyval_lex.c:1848:48: error: implicit declaration of function ‘fileno’ [-Werror=implicit-function-declaration]
 1848 |         b->yy_is_interactive = file ? (isatty( fileno(file) ) > 0) : 0;
      |                                                ^~~~~~
cc1: some warnings being treated as errors

I tried adding POSIX feature test macros but that doesn't seem to help. There seems to be a fix in flex from 7 years ago: westes/flex#263 Maybe that just hasn't trickled down to the distributions yet... I cannot reproduce this problem on my Mac, unfortunately.

@bosilca
Copy link
Member

bosilca commented Jul 9, 2024

Add -D _POSIX_C_SOURCE=20240101L" to libopalutilkeyval_la_CFLAGS` in opal/util/keyval/Makefile.am ?

I guess you will need to so the same for all lex/yacc outputs

config/opal_setup_cc.m4 Outdated Show resolved Hide resolved
@devreal devreal force-pushed the require-c11 branch 3 times, most recently from 8714b5e to c6ba95b Compare July 10, 2024 14:32
AC_MSG_NOTICE([using $flag to ensure C11 support])
break
fi
done
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to put a check here to ensure that some flag was found and that C11 is, indeed, supported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do error out if C11 is not supported (the block above). This is just adding the C11 flag as a restriction (GCC will eventually default to C23 so we could accidentally sneak in C23 code that). If we don't find a flag for some reason we can just move on.

Copy link
Member

Choose a reason for hiding this comment

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

Ah -- so there's no case where an additional flag is needed, but we can't find that flag? If that's true, then this is good enough.

I ask because $opal_cv_c11_flag_required is set to true if we try to compile a C11 program (i.e., checking __STDC_VERSION__) with no additional flags and it fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The OPAL_PROG_CC_C11 macro sets the -std=c11 parameter if it's needed for C11 support and then tries to pick the right flag. If we don't find a suitable flag we error out. If C11 support is available without a flag then we get here and try to find one to restrict ourselves. So this block is somewhat optional and we I'm ok with removing it if we don't want to restrict ourselves to C11.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, gotcha. Yes, if this is redundant code, let's ditch it.

Per your other point: I can't think of a reason to restrict ourselves to C11 -- can you? If neither of us can think of one, I think we're clear to remove this block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mhh, I was thinking we should protect ourselves from unintentionally introducing C23 code once compilers default to that but maybe that risk is low. I removed that part of configure.

@devreal devreal force-pushed the require-c11 branch 3 times, most recently from 9a54aae to 4ec6936 Compare July 15, 2024 14:52
@devreal
Copy link
Contributor Author

devreal commented Jul 16, 2024

Not sure why CI fails. All tests seem green but at the end it's marked as failed. Can someone please point me to the error?

Error out if the compiler does not support C11.

Also removes support for `__thread` since we now rely on C11 `_Thread_local`.

Signed-off-by: Joseph Schuchart <[email protected]>
All uses of the `PREPARE_REQUESTS_WITH_NO_FREE` macro map to size_t.

Signed-off-by: Joseph Schuchart <[email protected]>
Flex prior to 2.6.6 uses fileno() but does not define the needed
feature test macros.
See westes/flex#263 for details.

Signed-off-by: Joseph Schuchart <[email protected]>
Move calls to nanosleep out of headers and add the feature test macro
_POSIX_C_SOURCE to enable its use. This should not cause
any significant overheads.

Signed-off-by: Joseph Schuchart <[email protected]>
Needed for strsep(), posix_memalign(), and getopt().

Also fix the min/max macros. There is no need to cache the variables
as these macros are used without producing side-effects.

Signed-off-by: Joseph Schuchart <[email protected]>
@devreal devreal changed the title Require and limit features to C11 Require C11 Jul 17, 2024
@devreal
Copy link
Contributor Author

devreal commented Jul 17, 2024

Got it. Tests show green even if they failed in Jenkins... The culprit is our tests on RHEL 7.9, which still ships GCC 4.8. GCC added support for _Atomic, _Thread_local, and _Generic in 4.9 [1] and we are currently testing for that as part of our C11 test. We are not using _Atomic by default and _Generic is used in one place, optionally (the displacement/size wrappers for collectives). This PR makes _Thread_local the only option for TLS and removes the detection of __thread. All other features of C11 appear to be available in GCC 4.8, including __STDC_VERSION__ == 201112L.

I see three options:

  1. Drop support for RHEL 7.9 and its prehistoric default compiler. According to [2] RHEL 7.9 is on extended support until 2028.
  2. Separate out detection of _Atomic, _Generic, and _Thread_local, bring back the fallback to __thread, and require all other features of C11 by checking for __STDC_VERSION__ == 201112L (since GCC 4.7). That will give us access to some of the things C11 introduced without checking for all features explicitly, including _Static_assert, _Noreturn, stdalign.h, and anonymous structs.
  3. Drop this PR and bring it back post 2028.

[1] https://gcc.gnu.org/wiki/C11Status
[2] https://en.wikipedia.org/wiki/Red_Hat_Enterprise_Linux

@wenduwan
Copy link
Contributor

Drop support for RHEL 7.9 and its prehistoric default compiler.

The extended supported is a paid subscription IIUC so I don't think we can strictly secure the test environment. FYI EFA is dropping support for CentOS 7 and RHEL 7.

@devreal
Copy link
Contributor Author

devreal commented Jul 17, 2024

That's an important data point, thanks @wenduwan. I put this topic on the list for the next devel call.

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.

None yet

4 participants