Skip to content

autotools: stop setting -std=gnu89 with --enable-warnings#12346

Closed
vszakats wants to merge 11 commits intocurl:masterfrom
vszakats:test-autotools-warn-without-std
Closed

autotools: stop setting -std=gnu89 with --enable-warnings#12346
vszakats wants to merge 11 commits intocurl:masterfrom
vszakats:test-autotools-warn-without-std

Conversation

@vszakats
Copy link
Member

@vszakats vszakats commented Nov 16, 2023

Do not alter the C standard when building with --enable-warnings when
building with gcc.

On one hand this alters warning results compared to a default build.
On the other, it may produce different binaries, which is unexpected.

Also fix new warnings that appeared after removing -std=gnu89:

  • include: fix public curl headers to use the correct printf mask for
    CURL_FORMAT_CURL_OFF_T and CURL_FORMAT_CURL_OFF_TU with mingw-w64
    and Visual Studio 2013 and newer. This fixes the printf mask warnings
    in examples and tests. E.g. [1]

  • conncache: fix printf format string [2].

  • http2: fix potential null pointer dereference [3].
    (seen on Slackware with gcc 11.)

  • libssh: fix printf format string in SFTP code [4].
    Also make MSVC builds compatible with old CRT versions.

  • libssh2: fix printf format string in SFTP code for MSVC.
    Applying the same fix as for libssh above.

  • unit1395: fix argument is null and related issues [5]:

    • stop calling strcmp() with NULL to avoid undefined behaviour.
    • fix checking results if some of them were NULL.
    • do not pass NULL to printf %s.
  • ci: keep a build job with -std=gnu89 to continue testing for
    C89-compliance. We can apply this to other gcc jobs as needed.
    Ref: b23ce2c (2022-09-23) curl-compilers.m4: for gcc + want warnings, set gnu89 standard #9542

[1] https://dev.azure.com/daniel0244/curl/_build/results?buildId=18581&view=logs&jobId=ccf9cc6d-2ef1-5cf2-2c09-30f0c14f923b
[2] https://github.com/curl/curl/actions/runs/6896854263/job/18763831142?pr=12346#step:6:67
[3] https://github.com/curl/curl/actions/runs/6896854253/job/18763839238?pr=12346#step:30:214
[4] https://github.com/curl/curl/actions/runs/6896854253/job/18763838007?pr=12346#step:29:895
[5] https://github.com/curl/curl/actions/runs/6896854253/job/18763836775?pr=12346#step:33:1689

Closes #12346

@vszakats vszakats marked this pull request as draft November 16, 2023 21:45
@vszakats vszakats force-pushed the test-autotools-warn-without-std branch 3 times, most recently from 75a5954 to 56318f9 Compare November 17, 2023 01:48
@vszakats vszakats marked this pull request as ready for review November 17, 2023 02:04
- stop calling `strcmp()` with NULL to avoid undefined behaviour.
- fix checking results if some of them were NULL.
- do not pass NULL to printf `%s`.

```
In file included from ../../tests/libtest/test.h:43,
                 from curlcheck.h:24,
                 from unit1395.c:24:
unit1395.c: In function ‘test’:
../../lib/curl_printf.h:44:18: error: ‘%s’ directive argument is null [-Werror=format-overflow=]
   44 | # define fprintf curl_mfprintf
unit1395.c:93:7: note: in expansion of macro ‘fprintf’
   93 |       fprintf(stderr, "Test %u: '%s' gave '%s' instead of NULL\n",
      |       ^~~~~~~
```
Ref: https://github.com/curl/curl/actions/runs/6896854253/job/18763836775?pr=12346#step:33:1689
```
vssh/libssh.c: In function ‘myssh_statemach_act’:
vssh/libssh.c:1162:29: error: format ‘%llu’ expects argument of type ‘long long unsigned int’, but argument 2 has type ‘uint64_t’ {aka ‘long unsigned int’} [-Werror=format=]
 1162 |         char *tmp = aprintf("statvfs:\n"
      |                             ^~~~~~~~~~~~
......
 1169 |                             statvfs->f_bsize, statvfs->f_frsize,
      |                             ~~~~~~~~~~~~~~~~
      |                                    |
      |                                    uint64_t {aka long unsigned int}
vssh/libssh.c:1163:42: note: format string is defined here
 1163 |                             "f_bsize: %llu\n" "f_frsize: %llu\n"
      |                                       ~~~^
      |                                          |
      |                                          long long unsigned int
      |                                       %lu
```
Ref: https://github.com/curl/curl/actions/runs/6896854253/job/18763838007?pr=12346#step:29:895
```
In file included from curl_setup.h:656,
                 from conncache.c:26:
conncache.c: In function ‘Curl_conncache_add_conn’:
conncache.c:246:22: error: format ‘%ld’ expects argument of type ‘long int’, but argument 3 has type ‘curl_off_t’ {aka ‘long long int’} [-Werror=format=]
  246 |   DEBUGF(infof(data, "Added connection %ld. "
      |                      ^~~~~~~~~~~~~~~~~~~~~~~~
  247 |                "The cache now contains %zu members",
  248 |                conn->connection_id, connc->num_conn));
      |                ~~~~~~~~~~~~~~~~~~~
      |                    |
      |                    curl_off_t {aka long long int}
curl_setup_once.h:286:19: note: in definition of macro ‘DEBUGF’
  286 | #define DEBUGF(x) x
      |                   ^
conncache.c:246:10: note: in expansion of macro ‘infof’
  246 |   DEBUGF(infof(data, "Added connection %ld. "
      |          ^~~~~
conncache.c:246:42: note: format string is defined here
  246 |   DEBUGF(infof(data, "Added connection %ld. "
      |                                        ~~^
      |                                          |
      |                                          long int
      |                                        %lld
```
Ref: https://github.com/curl/curl/actions/runs/6896854263/job/18763831142?pr=12346#step:6:67
```
http2.c: In function 'nw_out_writer':
http2.c:374:14: error: null pointer dereference [-Werror=null-dereference]
  374 |   nwritten = Curl_conn_cf_send(cf->next, data, (const char *)buf, buflen, err);
      |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```
Ref: https://github.com/curl/curl/actions/runs/6896854253/job/18763839238?pr=12346#step:30:214
E.g.:
```
ftpgetinfo.c: In function 'main':
ftpgetinfo.c:78:16: error: ISO C does not support the 'I' printf flag [-Werror=format=]
   78 |         printf("filesize %s: %" CURL_FORMAT_CURL_OFF_T " bytes\n",
      |                ^~~~~~~~~~~~~~~~
ftpgetinfo.c:78:16: error: format '%d' expects argument of type 'int', but argument 3 has type 'curl_off_t' {aka 'long long int'} [-Werror=format=]
   78 |         printf("filesize %s: %" CURL_FORMAT_CURL_OFF_T " bytes\n",
      |                ^~~~~~~~~~~~~~~~
   79 |                filename, filesize);
      |                          ~~~~~~~~
      |                          |
      |                          curl_off_t {aka long long int}
In file included from ../../include/curl/curl.h:54,
                 from ftpgetinfo.c:27:
../../include/curl/system.h:205:42: note: format string is defined here
  205 | #  define CURL_FORMAT_CURL_OFF_T     "I64d"
```
Ref: https://dev.azure.com/daniel0244/curl/_build/results?buildId=18581&view=logs&jobId=ccf9cc6d-2ef1-5cf2-2c09-30f0c14f923b
Caveat: mingw old, I have no info if `inttypes.h` is supported there.
No longer necessary after fixing curl public headers.
@vszakats vszakats force-pushed the test-autotools-warn-without-std branch from 56318f9 to 915fb74 Compare November 17, 2023 13:12
@vszakats
Copy link
Member Author

vszakats commented Nov 17, 2023

Looked up the history of this option. Added in b23ce2c (last year) via #9542, with the goal of testing for C89 compliance. I think this would be better achieved by passing -std=gnu89 via CFLAGS in specific CI jobs where we want to test for this.

The original goal of this option was to test C89-compliance.
Keep that feature, but limited to a specific CI job. We can extended
to other gcc jobs as needed.

Don't add them to these though, as in these revealed actual issues by
building without `-std=gnu89`:

- openssl3-O3
- event-based
- Slackware-openssl-with-gssapi-gcc
- mingw-w64 builds
@github-actions github-actions bot added the CI Continuous Integration label Nov 17, 2023
@vszakats
Copy link
Member Author

Restored -std=gnu89 as CFLAGS in the linux / openssl3 CI job.

@vszakats
Copy link
Member Author

vszakats commented Nov 17, 2023

This old old-mingw header package (from 2010) does contain inttypes.h:
mingwrt-3.18-mingw32-dev.tar.gz via https://sourceforge.net/projects/mingw/files/MinGW/Base/mingwrt/mingwrt-3.18/

It means old-mingw likely continues to work fine with public headers after this change, though I haven't actually tried.

@vszakats vszakats force-pushed the test-autotools-warn-without-std branch 2 times, most recently from 71385a3 to 547fb72 Compare November 18, 2023 22:53
@vszakats
Copy link
Member Author

vszakats commented Nov 18, 2023

I couldn't replicate the same issue with libssh2 [1]. I cannot even grasp it for libssh, as it seems rooted from the fact that uint64_t somehow becomes long unsigned int "uint64_t {aka long unsigned int}" in that CI job.

Anyway, for libssh2 the MSVC-issue seems to apply too:

curl/lib/vssh/libssh2.c

Lines 1965 to 1971 in a9fd0d0

char *tmp = aprintf("statvfs:\n"
"f_bsize: %llu\n" "f_frsize: %llu\n"
"f_blocks: %llu\n" "f_bfree: %llu\n"
"f_bavail: %llu\n" "f_files: %llu\n"
"f_ffree: %llu\n" "f_favail: %llu\n"
"f_fsid: %llu\n" "f_flag: %llu\n"
"f_namemax: %llu\n",

Any thoughts to include this MSVC fix for libssh2 too?:

diff --git a/lib/vssh/libssh2.c b/lib/vssh/libssh2.c
index 543b6fb43..4eb5cf63f 100644
--- a/lib/vssh/libssh2.c
+++ b/lib/vssh/libssh2.c
@@ -1962,13 +1962,23 @@ static CURLcode ssh_statemach_act(struct Curl_easy *data, bool *block)
         break;
       }
       else if(rc == 0) {
+        #ifdef _MSC_VER
+        #define LIBSSH2_VFS_SIZE_MASK "I64u"
+        #else
+        #define LIBSSH2_VFS_SIZE_MASK "llu"
+        #endif
         char *tmp = aprintf("statvfs:\n"
-                            "f_bsize: %llu\n" "f_frsize: %llu\n"
-                            "f_blocks: %llu\n" "f_bfree: %llu\n"
-                            "f_bavail: %llu\n" "f_files: %llu\n"
-                            "f_ffree: %llu\n" "f_favail: %llu\n"
-                            "f_fsid: %llu\n" "f_flag: %llu\n"
-                            "f_namemax: %llu\n",
+                            "f_bsize: %" LIBSSH2_VFS_SIZE_MASK "\n"
+                            "f_frsize: %" LIBSSH2_VFS_SIZE_MASK "\n"
+                            "f_blocks: %" LIBSSH2_VFS_SIZE_MASK "\n"
+                            "f_bfree: %" LIBSSH2_VFS_SIZE_MASK "\n"
+                            "f_bavail: %" LIBSSH2_VFS_SIZE_MASK "\n"
+                            "f_files: %" LIBSSH2_VFS_SIZE_MASK "\n"
+                            "f_ffree: %" LIBSSH2_VFS_SIZE_MASK "\n"
+                            "f_favail: %" LIBSSH2_VFS_SIZE_MASK "\n"
+                            "f_fsid: %" LIBSSH2_VFS_SIZE_MASK "\n"
+                            "f_flag: %" LIBSSH2_VFS_SIZE_MASK "\n"
+                            "f_namemax: %" LIBSSH2_VFS_SIZE_MASK "\n",
                             statvfs.f_bsize, statvfs.f_frsize,
                             statvfs.f_blocks, statvfs.f_bfree,
                             statvfs.f_bavail, statvfs.f_files,

Applying the same fix as used with libssh.
@vszakats vszakats force-pushed the test-autotools-warn-without-std branch from 547fb72 to a972c26 Compare November 18, 2023 23:13
@vszakats
Copy link
Member Author

vszakats commented Nov 19, 2023

To make libssh/libssh2 masks the same and future-proof, I'm thinking of moving from:
libssh:

        #ifdef _MSC_VER
        #define LIBSSH_VFS_SIZE_MASK "I64u"
        #else
        #define LIBSSH_VFS_SIZE_MASK PRIu64
        #endif

libssh2:

        #ifdef _MSC_VER
        #define LIBSSH2_VFS_SIZE_MASK "I64u"
        #else
        #define LIBSSH2_VFS_SIZE_MASK "llu"
        #endif

to this for both:

        #ifdef PRIu64
        #define LIBSSH_VFS_SIZE_MASK PRIu64
        #elif defined(_MSC_VER)
        #define LIBSSH_VFS_SIZE_MASK "I64u"
        #else
        #define LIBSSH_VFS_SIZE_MASK "llu"
        #endif

This would use the official uint64_t mask whenever available from inttypes.h either in via libssh.h (next release will include it for all targets) or curl/system.h. And fall back to the universal MSVC value, or the universal standard one for others.

...well, maybe just for libssh. For libssh2 the 64-bit type libssh2_uint64_t isn't stricly mapped to uint64_t:
https://github.com/libssh2/libssh2/blob/8c320a93a48775b74f40415e46f84bf68b4d5ae8/include/libssh2.h#L128-L145

UPDATE: Talked myself out of this. Once libssh gets a release with universal inttypes.h, we can revisit this.

@vszakats vszakats closed this in 413a0fe Nov 20, 2023
@vszakats vszakats deleted the test-autotools-warn-without-std branch November 20, 2023 22:29
vszakats added a commit to vszakats/curl that referenced this pull request Dec 17, 2023
Avoid stomping the libssh and libssh2 macro namespaces by prefixing
these with `CURL_`.

Follow-up to 413a0fe curl#12346

Closes #xxxxx
vszakats added a commit that referenced this pull request Dec 17, 2023
Avoid using the libssh and libssh2 macro namespaces by prefixing
these local macro names with `CURL_`.

Follow-up to 413a0fe #12346

Reviewed-by: Daniel Stenberg
Closes #12544
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

4 participants