Require C11 #12660

devreal · 2024-07-09T21:12:13Z

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?

bosilca

Not for 5.x

devreal · 2024-07-09T22:45:38Z

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 · 2024-07-09T23:53:45Z

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

opal/util/Makefile.am

jsquyres · 2024-07-10T18:36:21Z

config/opal_setup_cc.m4

+                AC_MSG_NOTICE([using $flag to ensure C11 support])
+                break
+            fi
+        done


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

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.

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.

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.

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.

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 · 2024-07-16T20:34:45Z

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?

devreal · 2024-07-17T14:36:12Z

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:

Drop support for RHEL 7.9 and its prehistoric default compiler. According to [2] RHEL 7.9 is on extended support until 2028.
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.
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 · 2024-07-17T14:44:28Z

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 · 2024-07-17T15:32:42Z

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

devreal · 2024-09-10T16:21:44Z

It looks like the Mac OS X environment is borked. It fails to find strsep (even though <string.h> is included) and fails on some internal header:

/Applications/Xcode_15.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/signal.h:69:42: error: use of undeclared identifier 'NSIG'
extern __const char *__const sys_signame[NSIG];
                                         ^

rhc54 · 2024-09-10T16:49:28Z

Take a gander at @wenduwan comment in the slack "general" thread - looks like GitHub is making some significant changes to the Mac "actions" environment.

devreal · 2024-09-10T18:12:49Z

@rhc54 Thanks for the pointer. According to https://github.blog/changelog/2024-08-19-notice-of-upcoming-deprecations-and-breaking-changes-in-github-actions-runners/ the macos-12 are phased out but we're using macos-14-arm64 so I don't think we're affected here.

hjelmn · 2024-10-08T15:54:59Z

Awesome! +1000

github-actions · 2024-10-08T20:26:34Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

1a86e0b: Disable silent-rules on CI

check_cherry_pick: contains a cherry pick message that refers to a commit that exists, but is in an as-yet unmerged pull request: d4410ad

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

github-actions · 2024-10-09T13:14:07Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

1a86e0b: Disable silent-rules on CI

check_cherry_pick: contains a cherry pick message that refers to a commit that exists, but is in an as-yet unmerged pull request: d4410ad

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

devreal · 2024-10-09T15:28:17Z

I removed the verbose output and rebased onto current main. All tests pass now. I think this is good to go so please re-review.

bwbarrett · 2024-12-16T20:45:20Z

@devreal Do you still want to pull these changes into main for the next major release? I think we're all on the same page that we want to do this.

devreal · 2024-12-16T21:19:45Z

Thanks for the nudge @bwbarrett. I rebased, hope all tests pass this time.

devreal · 2024-12-17T16:20:00Z

All tests passed. Please rereview and merge if happy with it.

hppritcha · 2024-12-17T16:25:25Z

could you squash down to fewer commits - maybe one?

Error out if the compiler does not support C11. Removes support for `__thread` since we now rely on C11 `_Thread_local`. *Auxilliary fixes required to build with C11:* Replace typeof() with size_t in loop header All uses of the `PREPARE_REQUESTS_WITH_NO_FREE` macro map to size_t. Add feature test macro _POSIX_C_SOURCE to enable fileno() Flex prior to 2.6.6 uses fileno() but does not define the needed feature test macros. See westes/flex#263 for details. Add feature test macro to enable pthread_mutexattr_settype() Enable nanosleep with _POSIX_C_SOURCE 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. Reduce_local test: Add feature test macros 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. Include ompi_config.h in reduce_local.c Signed-off-by: Joseph Schuchart <[email protected]>

devreal · 2024-12-17T16:39:22Z

Sorry, forgot about that. Squashed down to one commit and kept the details of the changes in the commit message.

bwbarrett · 2024-12-17T16:43:10Z

I actually disagree with Howard on the squash, but what's done is done.

bwbarrett · 2024-12-17T16:55:36Z

test/datatype/reduce_local.c

-        __typeof__(b) _b = (b); \
-        _a < _b ? _a : _b;      \
-    })
+#define max(a, b) (a) > (b) ? (a) : (b)


max() isn't used, and min() looks like it isn't used in a way where this implementation breaks, so I'm ok with it.

But why ? The original code was correct, compliant with C11. Why changing it to a worst version ?

typeof is not C11 and the generic types aren't needed here.

Signed-off-by: Jeff Squyres <[email protected]>

jsquyres

I totally missed that you asked where we should notify users.

I took the liberty of pushing a commit to this PR with the corresponding docs changes (including a new v6.0.x changelog which features a single line so far: OMPI now requires a C11-compliant compiler).

devreal · 2024-12-17T17:16:00Z

Thanks @jsquyres!

@janjust The Nvidia CI seems to be running low on disk space.

bosilca

Too many unnecessary things not related to the scope of this PR were added inside.

bosilca · 2024-12-17T17:58:38Z

test/datatype/reduce_local.c

-        __typeof__(b) _b = (b); \
-        _a < _b ? _a : _b;      \
-    })
+#define max(a, b) (a) > (b) ? (a) : (b)


But why ? The original code was correct, compliant with C11. Why changing it to a worst version ?

bosilca · 2024-12-17T17:59:19Z

ompi/mca/vprotocol/pessimist/vprotocol_pessimist_wait.c

@@ -20,7 +20,7 @@ static int vprotocol_pessimist_request_no_free(ompi_request_t **req) {
 }

 #define PREPARE_REQUESTS_WITH_NO_FREE(count, requests) do { \
-    for (typeof(count) i = 0; i < count; i++)     \
+    for (size_t i = 0; i < count; i++)     \


How is this related to the scope of this PR (aka. require C11) ?

typeof is not C11.

bosilca · 2024-12-17T17:59:24Z

opal/class/opal_fifo.h

@@ -224,7 +224,7 @@ static inline opal_list_item_t *opal_fifo_pop_atomic(opal_fifo_t *fifo)
        if (++attempt == 5) {
            /* deliberately suspend this thread to allow other threads to run. this should
             * only occur during periods of contention on the lifo. */
-            _opal_lifo_release_cpu();
+            opal_lifo_release_cpu();


How is this related to the scope of this PR (aka. require C11) ?

This had to be outlined to limit the scope of the _POSIX_C_SOURCE feature test macro needed for nanosleep.

bosilca · 2024-12-17T17:59:49Z

opal/class/opal_lifo.c

@@ -31,3 +35,16 @@ static void opal_lifo_construct(opal_lifo_t *lifo)
 }

 OBJ_CLASS_INSTANCE(opal_lifo_t, opal_object_t, opal_lifo_construct, NULL);
+
+
+void opal_lifo_release_cpu(void)


How is this related to the scope of this PR (aka. require C11) ?

See above, outlined for the _POSIX_C_SOURCE feature test macro.

devreal added the Target: main label Jul 9, 2024

devreal requested review from jsquyres and bosilca July 9, 2024 21:12

bosilca approved these changes Jul 9, 2024

View reviewed changes

devreal force-pushed the require-c11 branch from ab7724c to 25df677 Compare July 9, 2024 22:19

jsquyres reviewed Jul 10, 2024

View reviewed changes

config/opal_setup_cc.m4 Outdated Show resolved Hide resolved

devreal force-pushed the require-c11 branch 3 times, most recently from 8714b5e to c6ba95b Compare July 10, 2024 14:32

jsquyres reviewed Jul 10, 2024

View reviewed changes

opal/util/Makefile.am Show resolved Hide resolved

jsquyres reviewed Jul 10, 2024

View reviewed changes

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

devreal force-pushed the require-c11 branch from 4ec6936 to 58dc607 Compare July 16, 2024 20:46

devreal changed the title ~~Require and limit features to C11~~ Require C11 Jul 17, 2024

devreal mentioned this pull request Sep 9, 2024

ci: remove rhel7 #12805

Merged

devreal force-pushed the require-c11 branch from 58dc607 to f925b01 Compare September 10, 2024 15:47

wenduwan force-pushed the require-c11 branch from f925b01 to 13e8ddb Compare September 10, 2024 20:23

janjust mentioned this pull request Oct 8, 2024

Replace alignment_of_long_double with C11 alignof #12658

Open

devreal force-pushed the require-c11 branch from edd9a7c to b21fc7a Compare October 9, 2024 14:07

devreal requested a review from bosilca October 9, 2024 15:28

devreal force-pushed the require-c11 branch from b21fc7a to cdb4649 Compare December 16, 2024 21:18

devreal force-pushed the require-c11 branch from cdb4649 to eaea24f Compare December 17, 2024 16:38

bwbarrett approved these changes Dec 17, 2024

View reviewed changes

docs: Note that Open MPI now requires a C11-compliant compiler

e0c2f0b

Signed-off-by: Jeff Squyres <[email protected]>

jsquyres approved these changes Dec 17, 2024

View reviewed changes

bosilca reviewed Dec 17, 2024

View reviewed changes

Require C11 #12660

Are you sure you want to change the base?

Require C11 #12660

Conversation

devreal commented Jul 9, 2024

bosilca left a comment

Choose a reason for hiding this comment

devreal commented Jul 9, 2024

bosilca commented Jul 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devreal commented Jul 16, 2024

devreal commented Jul 17, 2024

wenduwan commented Jul 17, 2024

devreal commented Jul 17, 2024

devreal commented Sep 10, 2024

rhc54 commented Sep 10, 2024

devreal commented Sep 10, 2024

hjelmn commented Oct 8, 2024

github-actions bot commented Oct 8, 2024

github-actions bot commented Oct 9, 2024

devreal commented Oct 9, 2024

bwbarrett commented Dec 16, 2024

devreal commented Dec 16, 2024

devreal commented Dec 17, 2024

hppritcha commented Dec 17, 2024

devreal commented Dec 17, 2024

bwbarrett commented Dec 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsquyres left a comment

Choose a reason for hiding this comment

devreal commented Dec 17, 2024

bosilca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment