search

nginx / unit

NGINX Unit - universal web app server - a lightweight and versatile open source server that simplifies the application stack by natively executing application code across eight different programming language runtimes.
https://unit.nginx.org
Apache License 2.0
5.27k stars 324 forks source link

Explicitly build with -std=gnu11 (C11 with GNU extensions) #1012

Closed ac000 closed 3 months ago

ac000 commented 7 months ago

Currently Unit doesn't specify any specific C standard for compiling and will thus be compiled under whatever the compiler happens to default to.

For most recent'ish compilers that will be gnu11/17 (C11/17 + GNU extensions).

For some of the oldest still-supported systems, e.g RHEL/CentOS 7 and Debian 8 that will be gnu90 (with GCC 4.8.x).

Up until now this hasn't really been an issue and we have been able to use some C99 features that are implemented as GNU extensions in older compilers, e.g

However there are a couple of C99 features that aren't GNU extensions that would be handy to be able to use, i.e

However, if we are going to switch up to C99, then perhaps we should just leap frog to C11 instead (the Linux Kernel did in fact make the switch from gnu89 to gnu11 in March '22).

GCC 4.8 as in CentOS 7 & Debian 8 has some support for C11, so while we can make full use of C99, we couldn't yet make full use of C11, but in a few years when those systems go EOL we will no longer have that restriction and in the meantime we can restrict ourselves to the supported set of features (or implement fallbacks where appropriate).

It can only be a benefit that we would be compiling Unit consistently under the same language standard.

This will also help give the impression that Unit is a modern C code base.

ac000 commented 7 months ago

Putting this here so we don't loose it seeing as we are deprecating ReviewBoard.

ac000 commented 4 months ago

Rebased with master

$ git range-diff 4275a1e5...af1b255c
  -:  -------- >   1:  a922f9a6 Update third-party components for the Java module.
  -:  -------- >   2:  9a36de84 Go: Use Homebrew include paths
  -:  -------- >   3:  846a7f48 .mailmap: Set correct address for Danielle
  -:  -------- >   4:  d9f5f1fb Ruby: Handle response field arrays
  -:  -------- >   5:  88854cf1 Ruby: Prevent a possible integer underflow
  -:  -------- >   6:  49aee676 HTTP: added TSTR validation flag to the rewrite option.
  -:  -------- >   7:  263460d9 Docs: replaced the slack community links with GitHub Discussions
  -:  -------- >   8:  7e03a6cc Go: Add missing +build and go:build comments
  -:  -------- >   9:  6ee5d555 .mailmap: Fix up Taryn's email address
  -:  -------- >  10:  b04455f6 Updated security.txt
  -:  -------- >  11:  e95a91cb .mailmap: Add a few more entries
  -:  -------- >  12:  5a833793 Tests: pathlib used where appropriate
  -:  -------- >  13:  a1e00b4e White space formatting fixes
  -:  -------- >  14:  4e08f495 Tests: added Ruby tests with array in header values
  -:  -------- >  15:  02d1984c HTTP: Remove short read check in nxt_http_static_buf_completion()
  -:  -------- >  16:  af6833a1 Tools: setup-unit: -hh: Add missing documentation for 'restart'
  -:  -------- >  17:  034b6394 Tools: setup-unit: -hh: The advanced commands aren't experimental
  -:  -------- >  18:  ba56e50e Tools: setup-unit: -hh: Add short-cut for the advanced help
  -:  -------- >  19:  6452ca11 Node.js: fixed "httpVersion" variable format
  -:  -------- >  20:  37abe2e4 HTTP: refactored out nxt_http_request_access_log().
  -:  -------- >  21:  4c91bebb HTTP: enhanced access log with conditional filtering.
  -:  -------- >  22:  dcbff27d Docs: Update changes.xml for conditional access logging
  -:  -------- >  23:  ad364507 Tests: "if" option in access logging.
  -:  -------- >  24:  9919b50a Isolation: Add a new nxt_cred_t type
  -:  -------- >  25:  f7c9d3a8 Isolation: Use an appropriate type for storing uid/gids
  -:  -------- >  26:  eba7378d Configuration: Use the NXT_CONF_VLDT_REQUIRED flag for procmap
  -:  -------- >  27:  990fbe70 Configuration: Remove procmap validation code
  -:  -------- >  28:  ecd57392 Configuration: Add nxt_conf_get_string_dup()
  -:  -------- >  29:  bb376c68 Simplify, by calling nxt_conf_get_string_dup()
  -:  -------- >  30:  46cef09f Configuration: Don't corrupt abstract socket names
  -:  -------- >  31:  9e986704 Configuration: Fix validation of "processes"
  -:  -------- >  32:  3a2687bb Packages: added Ubuntu 23.10 "mantic" support.
  -:  -------- >  33:  8ebe04fd contrib: Bump libunit-wasm to 0.3.0.
  -:  -------- >  34:  ca1bc062 contrib: updated njs to 0.8.2.
  -:  -------- >  35:  bad2c181 Packages: Added Fedora 39 support.
  -:  -------- >  36:  661e918a Docker: added python3.12 to versions
  -:  -------- >  37:  2b0d93d1 Docker: Generated Dockerfile for Unit 1.31.1.
  -:  -------- >  38:  fbeb2065 fix: Take options as well as requestListener (#1091)
  -:  -------- >  39:  c3af21e9 Docker: Switch to github
  -:  -------- >  40:  baff936b Packages: Move dist target to git archive
  -:  -------- >  41:  34b3a812 Add nxt_file_chown()
  -:  -------- >  42:  b500c36d Allow to set the permissions of the Unix domain control socket
  -:  -------- >  43:  2bd3b418 Docs: Update man page for new --control-* options
  -:  -------- >  44:  1dca8602 Tools: disambiguate unitc control socket detection
  -:  -------- >  45:  756feafd Node.js: Use console.warn instead of stderr.write
  -:  -------- >  46:  ea239635 Docker: Switch python3.12 to using github
  -:  -------- >  47:  0c983530 Node.js: Build/install fix
  -:  -------- >  48:  30b410e4 Avoid a segfault in nxt_conn_io_sendbuf()
  -:  -------- >  49:  62894ae7 Var: Refactored nxt_var_ref_get()
  -:  -------- >  50:  01fd121c Var: Refactored nxt_http_unknown_var_ref()
  -:  -------- >  51:  63507c49 Var: Make nxt_var_cache_value() more general
  -:  -------- >  52:  46554015 Var: Introduced nxt_var_get()
  -:  -------- >  53:  63ad4deb NJS: Simplified nxt_js_call()
  -:  -------- >  54:  33c6c4d4 NJS: variable access support
  -:  -------- >  55:  183a1e9d Docker: redirect logs to stderr
  -:  -------- >  56:  5570d807 Packages: fixed a path to python 3.12 example app
  -:  -------- >  57:  53648ed5 Tools: Fix typo in tools/README.md
  -:  -------- >  58:  d24ae5a9 Add additional replace rules for node:* modules
  -:  -------- >  59:  bd0abdf0 Docker: Shallow clone the Unit repo
  -:  -------- >  60:  914cd4e3 .mailmap: Map some more personal addresses
  -:  -------- >  61:  d52a9361 Docker: Update versions of Go, Node, PHP, Ruby
  -:  -------- >  62:  2765522b Tests: NJS request variables
  -:  -------- >  63:  cca2c46e Tools: setup-unit: Use trap(1) to handle cleanup
  -:  -------- >  64:  d6ed0003 Tools: setup-unit: De-duplicate code
  -:  -------- >  65:  e9a0c49d Tools: setup-unit: Pass --fail-with-body to curl(1)
  -:  -------- >  66:  565a8ed0 Tools: setup-unit: ctl edit: Print file name on error
  -:  -------- >  67:  bc093ab3 Tools: setup-unit: Fix error message
  -:  -------- >  68:  6aa5ef63 Tools: setup-unit: ctl edit: Append suffix to tmp file name
  -:  -------- >  69:  f71ead5f Updated copyright notice.
  -:  -------- >  70:  697a5850 Python: bytearray body support for ASGI module.
  -:  -------- >  71:  56d3a1a7 Add GitHub Actions
  -:  -------- >  72:  bca44630 .mailmap: Map Dylan's GitHub address
  -:  -------- >  73:  f2e64475 Wasm-wc: Register a new Wasm component model language module type
  -:  -------- >  74:  f0782722 Wasm-wc: Add core configuration data structure
  -:  -------- >  75:  20ada4b5 Wasm-wc: Core of initial Wasm component model language module support
  -:  -------- >  76:  a9345dd4 Add a .rustfmt.toml file
  -:  -------- >  77:  79c81772 Wasm-wc: Run src/lib.rs through rustfmt
  -:  -------- >  78:  ac3a54d6 Wasm-wc: Improve request buffer handling
  -:  -------- >  79:  98f808af Wasm-wc: Upgrade to wasmtime 17
  -:  -------- >  80:  60eb6c43 Wasm-wc: Allow to use the 'reactor' adaptor again
  -:  -------- >  81:  8d030139 Wasm-wc: Add Cargo.lock
  -:  -------- >  82:  07a0c9a3 Wasm-wc: Wire up the language module to the config system
  -:  -------- >  83:  da44dc00 Fix alignment of wasm options text in auto/help
  -:  -------- >  84:  4e6d7e87 Wasm-wc: Wire it up to the build system
  -:  -------- >  85:  7702293d Docker: Bump rust version to 1.76.0
  -:  -------- >  86:  1297f6f0 Docker: Add wasm-wasi-component to the wasm target
  -:  -------- >  87:  4c558697 Docker: Re-generate Dockerfile.wasm
  -:  -------- >  88:  7883acc6 Tests: Ruby hook tests unstable for version older 3.0
  -:  -------- >  89:  99da2f3c Tests: check for the AddressSanitizer flag during discovery
  -:  -------- >  90:  dbd9d25f Tests: skip some of TLS reconfiguration tests under AddressSanitizer
  -:  -------- >  91:  cabea47d Tests: renamed test_python_procman.py since it's not Python-specific
  -:  -------- >  92:  3f805bc6 Packages: added wasm-wasi-component module packaging for deb-based distros
  -:  -------- >  93:  7a640556 Packages: added wasm-wasi-component module packaging for rpm-based distros
  -:  -------- >  94:  7b13c306 Wasm-wc: Add nxt_unit.o as a dependency in the auto script
  -:  -------- >  95:  d54af163 Wasm-wc: Use the cargo build output as the make target dependency
  -:  -------- >  96:  2f3c7c2c Update third-party java components to their recent versions
  -:  -------- >  97:  e2cab032 Remove debug from builds and tests
  -:  -------- >  98:  faa7e792 Packages: Pass CFLAGS to compile wasm modules on all packaging targets
  -:  -------- >  99:  9d02b906 Add PHP 8.2 and 8.1 to test matrix
  -:  -------- > 100:  d5665f49 Update setup-go to v5
  -:  -------- > 101:  4d25c612 Edited changes.xml for the 1.32.0 release
  -:  -------- > 102:  ace553dc Generated Dockerfiles for Unit 1.32.0
  -:  -------- > 103:  08811700 Added version 1.32.0 CHANGES
  -:  -------- > 104:  e67d7433 Version bump
  -:  -------- > 105:  23e807de Wasm-wc: use more common uname switch to get operating system name
  1:  4275a1e5 = 106:  af1b255c Build with -std=gnu11 (C11 with GNU extensions)
ac000 commented 4 months ago 
$ git range-diff af1b255c...0c9c0b90
1:  af1b255c ! 1:  0c9c0b90 Build with -std=gnu11 (C11 with GNU extensions)
    @@ Commit message
         Currently Unit doesn't specify any specific C standard for compiling and
         will thus be compiled under whatever the compiler happens to default to.

    -    For most recent'ish compilers that will be gnu11/17 (C11/17 + GNU
    -    extensions).
    +    Current releases of GCC and Clang (13.x & 17.x respectively at the time
    +    of writing) default to gnu17 (C17 + GNU extensions).

    -    For some of the oldest still-supported systems, e.g RHEL/CentOS 7 and
    -    Debian 8 that will be gnu90 (with GCC 4.8.x).
    +    Our oldest still-supported system is RHEL/CentOS 7, that comes with GCC
    +    4.8.5 which defaults to gnu90.

         Up until now this hasn't really been an issue and we have been able to
         use some C99 features that are implemented as GNU extensions in older
    @@ Commit message

         However, if we are going to switch up to C99, then perhaps we should
         just leap frog to C11 instead (the Linux Kernel did in fact make the
    -    switch from gnu89 to gnu11 in March '22).
    +    switch from gnu89 to gnu11 in March '22). C17 is perhaps still a little
    +    new and is really just C11 + errata.

    -    GCC 4.8 as in CentOS 7 & Debian 8 has *some* support for C11, so while
    -    we can make full use of C99, we couldn't yet make full use of C11, but
    -    in a few years when those systems go EOL we will no longer have that
    +    GCC 4.8 as in RHEL 7 has *some* support for C11, so while we can make
    +    full use of C99, we couldn't yet make full use of C11, However RHEL 7 is
    +    EOL on June 30th 2024, after which we will no longer have that
         restriction and in the meantime we can restrict ourselves to the
         supported set of features (or implement fallbacks where appropriate).

    @@ Commit message
         Suggested-by: Andrew Clayton <a.clayton@nginx.com>
         Acked-by: Andrew Clayton <a.clayton@nginx.com>
         [ Andrew wrote the commit message ]
    -    Signed-off-by: Alejandro Colomar <alx@nginx.com>
    +    Signed-off-by: Alejandro Colomar <alx@kernel.org>
         Link: <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e8c07082a810fbb9db303a2b66b66b8d7e588b53>
    +    Link: <https://www.ibm.com/blog/announcement/ibm-is-announcing-red-hat-enterprise-linux-7-is-going-end-of-support-on-30-june-2024/>
    +    Cc: Dan Callahan <d.callahan@f5.com>
         Signed-off-by: Andrew Clayton <a.clayton@nginx.com>

      ## auto/cc/test ##
alejandro-colomar commented 4 months ago

BTW, once RHEL 7 dies, maybe you can jump to -std=gnu17 directly. Is there any other alive version that doesn't support gnu17?

ac000 commented 4 months ago

BTW, once RHEL 7 dies, maybe you can jump to -std=gnu17 directly. Is there any other alive version that doesn't support gnu17?

I checked what ships with RHEL 8, GCC 8, and that seems to support C17, didn't check *BSD etc, however from the GCC documentation we have this

A version with corrections integrated was prepared in 2017 and published in 2018 as ISO/IEC 9899:2018; it is known as C17 and is supported with -std=c17 or -std=iso9899:2017; the corrections are also applied with -std=c11, and the only difference between the options is the value of __STDC_VERSION__.

So with GCC 8+, -std=c11/gnu11 will basically also give you c17/gnu17.

So -std=gnu11 seems like a good choice, we won't limit slightly older compilers but will also generally get the benefits of C17.

I haven't checked if this is the case with clang yet...

ac000 commented 4 months ago

Rebased with master

$ git range-diff 0c9c0b90...f0f6ef6e
 1:  0c9c0b90 <  -:  -------- Build with -std=gnu11 (C11 with GNU extensions)
 -:  -------- >  1:  8ff606fb Configuration: Fix check in nxt_conf_json_parse_value()
 -:  -------- >  2:  4eb008bb Remove unused nxt_vector_t API
 -:  -------- >  3:  353d2d05 Var: Remove a dead assignment in nxt_var_interpreter()
 -:  -------- >  4:  c2f7f296 Avoid potential NULL pointer dereference in nxt_router_temp_conf()
 -:  -------- >  5:  8032ce31 Test with root access in GitHub workflows
 -:  -------- >  6:  0cee7d1a Add GitHub workflow for wasm-wasi-component
 -:  -------- >  7:  63bc3882 .mailmap: Map Dylan's 2nd GitHub address
 -:  -------- >  8:  f6899af6 Var: Fix cacheable issue for njs variable access
 -:  -------- >  9:  5511593d Remove support for Microsoft's Visual C++ compiler
 -:  -------- > 10:  0c2d7786 Remove support for Intel's icc compiler
 -:  -------- > 11:  e79e4635 Remove support for IBM's XL C compiler
 -:  -------- > 12:  9cd11133 Remove support for Sun's Sun Studio/SunPro C compiler
 -:  -------- > 13:  806e209d Remove -W from compiler flags
 -:  -------- > 14:  1dcb5383 Expand the comment about -Wstrict-overflow on GCC
 -:  -------- > 15:  0b5223e1 Disable strict-aliasing in clang by default
 -:  -------- > 16:  c1e3f02f Compile with -fno-strict-overflow
 -:  -------- > 17:  280a978d Add initial infrastructure for pretty printing make output
 -:  -------- > 18:  5d831af0 Hook up make pretty printing to the Unit core and tests
 -:  -------- > 19:  da335bec Pretty print the Java language module compiler output
 -:  -------- > 20:  574528f7 Pretty print the Perl language module compiler output
 -:  -------- > 21:  0a0dcf91 Pretty print the PHP language module compiler output
 -:  -------- > 22:  caaa1d28 Pretty print the Python language module compiler output
 -:  -------- > 23:  133f75fd Pretty print the Ruby language module compiler output
 -:  -------- > 24:  b763ba7e Pretty print the wasm language module compiler output
 -:  -------- > 25:  15072fbd Enable optional 'debuggable' builds
 -:  -------- > 26:  d23812b8 Allow to disable -Werror at 'make' time
 -:  -------- > 27:  f55fa70c Add a help target to the root Makefile
 -:  -------- > 28:  a171b399 Add an EXTRA_CFLAGS make variable
 -:  -------- > 29:  f0f6ef6e Build with -std=gnu11 (C11 with GNU extensions)
hongzhidao commented 4 months ago

Hi, I'm impartial to the change of switching from C89 to GNU11 and open to the team's decision. Thanks.

ac000 commented 4 months ago

I'm impartial to the change of switching from C89 to GNU11 and open to the team's decision.

This comment shows exactly why we need to set a standard!

... switching from C89 to GNU11

Oh, but we're not switching from C89.

I'm curious why you think that?

(Yes, we had to limit ourselves to C90, well gnu90, due to GCC 4.8 on RHEL 7 defaulting to -std=gnu90, which fortunately did allow us to use some C99 features...)

Currently we don't specify any standard. So when you compile Unit you will compile with whatever standard the compiler you're using defaults to.

Oh and GCC and Clang default to -std=gnuX not -std=cX

Let me ask you this. What compiler do you use on a day to day basis to compile Unit currently? and I'll tell what standard it's using.

GCC 5 (2014-12-23) switched to -std=gnu11 by default. So if your using a version of GCC or Clang from within the last ~10 years, there's a good chance you're already using at least gnu11. This patch just explicitly sets it.

Current versions of GCC (and probably Clang) default to -std=gnu17 which is basically C11 + errata.

hongzhidao commented 3 months ago

What compiler do you use on a day to day basis to compile Unit currently?

gcc version 8.5.0 20210514 (Red Hat 8.5.0-4) (GCC)

I'm not a fan of introducing such changes for now since the other way is to change things that have been working well when there is something we have to solve, it would be easier to check if the change makes sense. I'm not familiar with modern C compilers, that's why I said I'm impartial on it.

ac000 commented 3 months ago

What compiler do you use on a day to day basis to compile Unit currently? 

gcc version 8.5.0 20210514 (Red Hat 8.5.0-4) (GCC)

OK, so you do realise that you are compiling with -std=gnu11 already right? You understand that right? (well at least now hopefully anyway).

And to be absolutely clear here, I'll quote the GCC 8.5 manual

The default, if no C language dialect options are given, is -std=gnu11.

(Technically you're getting gnu17 due to GCC treating c11/gnu11 and c17/gnu17 the same, with the only difference being if you explicitly specify -std=c11/gnu11 then __STDC_VERSION__ returns 201112 rather than 201710)

Which is why your arguments make no sense to me as you're arguing against what you're already using...

I'm not a fan of introducing such changes for now since the other way is to change things that have been working well when there is

Change what? You're already using -std=gnu11 what did you have to change?.

All this patch does is to codify that, so it is clear to developers what C standard they should write to and will prevent people from trying to use too new features that we are not ready to support.

It also means Unit is targeting the same C standard on all systems we support which has got to be a good thing.

something we have to solve, it would be easier to check if the change

Again, as it stands you are already using -std=gnu11, What do think you have been using?

I've written the rationale for all this in the PR and commit message.

makes sense. I'm not familiar with modern C compilers, that's why I

I'm not sure what your definition of modern is, but I'd say you're already using modern C compilers.

said I'm impartial on it.

Great, if you're impartial to it, then there's no need to block it.

ac000 commented 3 months ago

Ultimately this change won't affect you, you can on carry just as you are and not give it a moments thought if you like.

Other people will see 'Oh Unit targets C11, OK, I won't waste my time trying to use C23 features...' and if they try, the compiler will let them know.

callahad commented 3 months ago

@ac000 Your tone here is unwarranted. I expect us all to hold ourselves to the highest possible standard in public communications in accordance with our code of conduct.

callahad commented 3 months ago

I'd argue in favor of this patch, as it makes our compiler target explicit rather than implicit.

Per @ac000's research, right now we're all using gnu11* by default, but defaults could change in the future. Pinning the version now protects us from the risk of gcc or clang changing their target.

*well, gnu17 is the real default, but it's very nearly identical to gnu11.

hongzhidao commented 3 months ago

Got it, thanks for the explanation.

ac000 commented 3 months ago

Rebased with master

$ git range-diff f0f6ef6e...b65e49c5
 -:  -------- >  1:  6b138571 Wasm-wc: Bump the mio crate from 0.8.10 to 0.8.11
 -:  -------- >  2:  2e615250 Add dependabot.yml
 -:  -------- >  3:  0d99744d Router: match when pattern and tested string are both zero length
 -:  -------- >  4:  fdc46759 NJS: avoiding arithmetic ops with NULL pointer in r->args
 -:  -------- >  5:  8844d33c Fixed undefined behaviour in left shift of int value
 -:  -------- >  6:  7dcd6c0e Avoiding arithmetic ops with NULL pointer in nxt_http_arguments_parse
 -:  -------- >  7:  264b3755 Avoiding arithmetic ops with NULL pointer in nxt_port_mmap_get
 -:  -------- >  8:  c9461a6b Initialize port_impl only when it is needed
 -:  -------- >  9:  dd701fb4 Avoiding arithmetic ops with NULL pointer in nxt_unit_mmap_get
 -:  -------- > 10:  abcfc4cd Fix the security-alert email link in the README
 -:  -------- > 11:  9993814d NJS: loader should be registered using njs_vm_set_module_loader()
 -:  -------- > 12:  0716b0c7 Tests: NJS cacheable variables with access log
 -:  -------- > 13:  dc16a7bc Add a repostatus badge to the README
 -:  -------- > 14:  7472a2ca Add a GitHub workflow status badge for our CI to the README
 1:  f0f6ef6e = 15:  b65e49c5 Build with -std=gnu11 (C11 with GNU extensions)