Skip to content

Fix a few minor compiler/standards issues (clang-16 porting) (for main branch)#767

Merged
val-ms merged 5 commits intoCisco-Talos:mainfrom
val-ms:clang-16-porting-pr-747-for-main
Nov 23, 2022
Merged

Fix a few minor compiler/standards issues (clang-16 porting) (for main branch)#767
val-ms merged 5 commits intoCisco-Talos:mainfrom
val-ms:clang-16-porting-pr-747-for-main

Conversation

@val-ms
Copy link
Contributor

@val-ms val-ms commented Nov 18, 2022

Copy of #747 forward-ported for main branch.

In addition, this PR:

  • Fixes a few new strict prototype issues.
  • Enables these warnings as errors by default at the top level CMake to prevent accidentally introducing more of these in the future.

The description from #747 is as follows...


The next major version of clang has threatened to enable some strictness flags by default:

https://bugs.gentoo.org/show_bug.cgi?id=clang-16-porting

Specifically,

  • -Werror=implicit-function-declaration
  • -Werror=implicit-int
  • -Werror=strict-prototypes

Whether or not they all wind up on by default, the problems they catch are easy to fix and usually beneficial for e.g. further compiler diagnostics. Here I've fixed what I had to in order to build v0.103.7.

For the autotools build system, the following autoconf fix (and an autoreconf -fi) is also required:

https://git.savannah.gnu.org/gitweb/?p=autoconf.git;a=commit;h=8b5e2016c7ed2d67f31b03a3d2e361858ff5299b

Otherwise, the checks that are emitted will unduly fail.

orlitzky and others added 3 commits November 17, 2022 16:17
On Linux, thpool.c uses syscall() from unistd.h, but that function is
not defined without _GNU_SOURCE:

  c-thread-pool/thpool.c: In function 'jobqueue_pull':
  c-thread-pool/thpool.c:474:105: error: implicit declaration of function
  'syscall' [-Werror=implicit-function-declaration]

In general that's not great, because it hinders some compiler diagnostics,
but it will also cause problems down the road if (for example) clang-16
decides to enable -Werror=implicit-function-declaration by default.

This commit changes the _POSIX_C_SOURCE definition at the top of
thpool.c to _GNU_SOURCE, as in the syscall(2) man page.
Prototypes (or the declarations themselves, if there is no
corresponding prototype) for functions that take no arguments are
required by the C standard to specify (void) as their argument list;
for example,

  regex_pcre.h:79:1: error: function declaration isn't a prototype
  [-Werror=strict-prototypes]
     79 | cl_error_t cli_pcre_init_internal();

Future versions of clang may become strict about this, and there's no
harm in conforming to the standard right now, so we fix all such
instances in this commit.
@lgtm-com
Copy link

lgtm-com bot commented Nov 18, 2022

This pull request introduces 2 alerts and fixes 1 when merging 74f65ed into 39fbbef - view on LGTM.com

new alerts:

  • 2 for Wrong type of arguments to formatting function

fixed alerts:

  • 1 for Implicit function declaration

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. It looks like GitHub code scanning with CodeQL is already set up for this repo, so no further action is needed 🚀. For more information, please check out our post on the GitHub blog.

@val-ms val-ms force-pushed the clang-16-porting-pr-747-for-main branch from 74f65ed to b142a17 Compare November 18, 2022 20:00
@val-ms
Copy link
Contributor Author

val-ms commented Nov 18, 2022

@ragusaa I just removed that commit which did this:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index f2c95c35f..74c09bbbb 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -851,6 +851,9 @@ else()
     extract_valid_c_flags(WARNCFLAGS
         -Wall -Wextra
         -Wformat-security
+        -Werror=implicit-function-declaration
+        -Werror=implicit-int
+        -Werror=strict-prototypes
     )
     if(ENABLE_ALL_THE_WARNINGS)
         extract_valid_c_flags(WARNCFLAGS

As discussed, we can figure out how to add this in our CI instead, to prevent introducing new warnings.

The change to define _GNU_SOURCE means that the compiler knows that
syscall returns a long, not an int -- so then it warns about the wrong
format characters for printing that value.
@lgtm-com
Copy link

lgtm-com bot commented Nov 18, 2022

This pull request fixes 1 alert when merging c3855e2 into 39fbbef - view on LGTM.com

fixed alerts:

  • 1 for Implicit function declaration

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. It looks like GitHub code scanning with CodeQL is already set up for this repo, so no further action is needed 🚀. For more information, please check out our post on the GitHub blog.

@ragusaa
Copy link
Contributor

ragusaa commented Nov 22, 2022

This fails to build when running those warnings as errors with clang-14. The two suggestions I have are

  1. Add the following to the top of crypto.c
    #if !defined(_WIN32) #define _GNU_SOURCE 1 #endif

  2. Change the following line in CMakeLists.txt
    if(CMAKE_COMPILER_IS_GNUCXX)
    to
    if(CMAKE_COMPILER_IS_GNUCXX OR (${CMAKE_CXX_COMPILER_ID} STREQUAL "Clang"))

Building with these will fail for Clang because _GNU_SOURCE is not
defined.
    -Werror=implicit-function-declaration
    -Werror=implicit-int
    -Werror=strict-prototypes

Also added fix to use "MATCHES" instead of "EQUALS" in FindLLVM where
the same sort of logic is used.
@val-ms
Copy link
Contributor Author

val-ms commented Nov 22, 2022

This fails to build when running those warnings as errors with clang-14. The two suggestions I have are

1. Add the following to the top of crypto.c
   `#if !defined(_WIN32) #define _GNU_SOURCE 1 #endif`

2. Change the following line in CMakeLists.txt
   `if(CMAKE_COMPILER_IS_GNUCXX)`
   to
   `if(CMAKE_COMPILER_IS_GNUCXX OR (${CMAKE_CXX_COMPILER_ID} STREQUAL "Clang"))`

I went with a variation on the second option. I think it is fixed now.

@lgtm-com
Copy link

lgtm-com bot commented Nov 22, 2022

This pull request fixes 1 alert when merging 9ab74bd into 6ea6ee5 - view on LGTM.com

fixed alerts:

  • 1 for Implicit function declaration

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. It looks like GitHub code scanning with CodeQL is already set up for this repo, so no further action is needed 🚀. For more information, please check out our post on the GitHub blog.

@val-ms val-ms merged commit e8a1fa6 into Cisco-Talos:main Nov 23, 2022
@val-ms val-ms deleted the clang-16-porting-pr-747-for-main branch November 23, 2022 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants