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

Switch from gnu99 to gnu11 #13339

Closed
wants to merge 1 commit into from
Closed

Switch from gnu99 to gnu11 #13339

wants to merge 1 commit into from

Conversation

szubersk
Copy link
Contributor

Motivation and Context

To keep up with the times
https://www.phoronix.com/scan.php?page=news_item&px=Linux-Kernel-C89-To-C11

Description

--std=gnu99 -> --std=gnu11

How Has This Been Tested?

$ make

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@gmelikov
Copy link
Member

It may give some problems in future with pre 5.18 kernels, I think. Until now we could build master branch with really old kernels. Should we wait several years? Or are there really strong pros?

@rincebrain
Copy link
Contributor

rincebrain commented Apr 17, 2022

+1 to waiting a while - as much as I'd love to have new conveniences of newer C standards, until we decide to cut off support for pre-C11 kernels, we're gonna need to target the least common denominator.

(A CI build target with -std=gnu11 might not be a bad idea, though I'd be pretty surprised if it caught much in practice...)

@szubersk
Copy link
Contributor Author

szubersk commented Apr 17, 2022

A good news is that no compatibility was broken.
I just tested following (vanilla) kernels:

  • linux-3.10.108
  • linux-4.9.310
  • linux-4.14.275
  • linux-4.19.238

and 4.X series compiles well (except for a couple of unrelated to C11 tweaks needed for autoconf).

3.10 doesn't compile due to:

/home/debian/zfs-portable/include/os/linux/kernel/linux/blkdev_compat.h:309:2: error: #error "Unsupported kernel: no usable disk change check"
  309 | #error "Unsupported kernel: no usable disk change check"

On top of that 3.10 kernels need:

  • -fno-pie in CFLAGS to support gcc 6.0+
  • patch for arch/x86/tools/relocs.c
- Elf_Addr per_cpu_load_addr;
+ static Elf_Addr per_cpu_load_addr;
  • patch for fault_in_multipages_readable() in include/linux/pagemap.h
+    (void)c;

I'm not sure if it is worth to investigate vanilla 3.10 further. RHEL/CentOS have a lot of patches backported to their 3.X kernel lines and those are anywhere near the vanilla counterparts.

Bottom line: I couldn't find a single problem related to the newer standard.

@szubersk szubersk marked this pull request as ready for review April 17, 2022 21:13
@gmelikov
Copy link
Member

What's with the case - pre 5.18 kernel with buildin zfs with gnu11 code changes?

@szubersk
Copy link
Contributor Author

szubersk commented Apr 18, 2022

Good catch, George. Looks like I completely missed the built-in compilation mode.

On the other hand OpenZFS now uses c99 and Linux uses c89. Those two somehow mix together. Maybe there's hope still...

EDIT01
https://www.kernel.org/doc/html/latest/kbuild/makefiles.html#compilation-flags

@rincebrain
Copy link
Contributor

I know you can override flags in Kbuild - I've done it a lot for playing with SIMD things, among other things.

I am worried that, for example, GCC I believe only claims to be near feature parity for C11 with 4.9 and up, and, say, RHEL7 ships and builds their kernels with gcc 4.8.x, so using any feature they don't have yet would go boom.

@szubersk
Copy link
Contributor Author

I just checked --enable-linux-builtin using Linux 4.9 and it works well (Kbuild flags took care of everything as Rich mentioned).

https://gcc.gnu.org/wiki/C11Status says that specifically 3 features of C11 are not supported in GCC 4.8.5 so we lose the customer base provided

  • we use those features, and
  • users stay on GCC 4.8.X

This affects RHEL/CentOS 7 users that want to stay vanilla and don't want to take advantage of SCLo/EPEL/other supplementary repos. I can't estimate how many people it would be. They always have a way out of vanilla, they are not cut off.

@behlendorf
Copy link
Contributor

It's always been my experience that C standards committee has done an excellent job prioritizing compatibility. And since the OpenZFS code does it best to avoid the grey areas, I'd expect this to Just Work. It's nice to see your testing and the CI results confirm that. That said, I think we want to hold off making this change.

My big concern is cross-platform compatibility. We want to retain the ability to easily add this OpenZFS source, with minimal modification, to the FreeBSD repository. My understanding is they use C99 so we'd still only want to use C99 features in the common code. Given that constraint, it makes sense to have the compiler enforce this for us. Additionally, the illumos and MacOS platforms do pull from this tree, moving to something newer could cause them some heartburn*.

It's good to know this works on Linux, but unless we have a real compelling reason to move to C11 it's seems preferable to me to stick with C99. At least for now.

  • We used to have this problem in reverse, where FreeBSD and Illumos allowed C99 and Linux required C89. This made porting changes to Linux more difficult since we needed to removing any C99'isms. This was eventually resolved by building the OpenZFS code as C99 which is compatible with the kernel's C89 code.

@behlendorf behlendorf added the Status: Design Review Needed Architecture or design is under discussion label Apr 21, 2022
@szubersk
Copy link
Contributor Author

Thanks Brian for the constructive criticism. Linux landscape seems unaffected, I'll check the impact on illumos and FreeBSD.
Maybe there's still hope.

@behlendorf
Copy link
Contributor

I believe the consensus was to wait a few years and then revisit this, so for the moment I'm going to close it out. @szubersk thanks for investigating this.

@behlendorf behlendorf closed this Sep 16, 2022
@szubersk szubersk deleted the szubersk-c11 branch October 4, 2022 00:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Design Review Needed Architecture or design is under discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants