-Wall

-Wall

[Corinna Vinschen]

Hi folks,

if you saw my today's pushes, you're aware that I only found a bug
because I used -Wall.  I fixed the bug and a few less crucial warnings.

I did NOT fix the warnings in code we took verbatim from some BSDs,
which often contain unused variables, or in some cases expressions which
are deemed to profit from extra paranthesis, e. g.

  if (a >= 0 ^ b == 0)

For that reason, I'd like to suggest to add -Wall by default to the
build flags for newlib, just as it is already for ages in the Cygwin
tree.

Anybody having a strong opinion, pro or contra?


Corinna

Re: -Wall

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 827 bytes --]

On 19 Jan 2024 13:55, Corinna Vinschen wrote:
> if you saw my today's pushes, you're aware that I only found a bug
> because I used -Wall.  I fixed the bug and a few less crucial warnings.
> 
> I did NOT fix the warnings in code we took verbatim from some BSDs,
> which often contain unused variables, or in some cases expressions which
> are deemed to profit from extra paranthesis, e. g.
> 
>   if (a >= 0 ^ b == 0)
> 
> For that reason, I'd like to suggest to add -Wall by default to the
> build flags for newlib, just as it is already for ages in the Cygwin
> tree.
> 
> Anybody having a strong opinion, pro or contra?

enabling -Wall by default sounds fine.  if you wanted to include any -Werror,
that would require more effort/tooling like putting it all behind a new flag
like --enable-werror.
-mike

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: -Wall

[Joel Sherrill]

[-- Attachment #1: Type: text/plain, Size: 1808 bytes --]

On Fri, Jan 19, 2024 at 9:39 AM Mike Frysinger <vapier@gentoo.org> wrote:

> On 19 Jan 2024 13:55, Corinna Vinschen wrote:
> > if you saw my today's pushes, you're aware that I only found a bug
> > because I used -Wall.  I fixed the bug and a few less crucial warnings.
> >
> > I did NOT fix the warnings in code we took verbatim from some BSDs,
> > which often contain unused variables, or in some cases expressions which
> > are deemed to profit from extra paranthesis, e. g.
> >
> >   if (a >= 0 ^ b == 0)
> >
> > For that reason, I'd like to suggest to add -Wall by default to the
> > build flags for newlib, just as it is already for ages in the Cygwin
> > tree.
> >
> > Anybody having a strong opinion, pro or contra?
>
> enabling -Wall by default sounds fine.  if you wanted to include any
> -Werror,
> that would require more effort/tooling like putting it all behind a new
> flag
> like --enable-werror.
>

Based on the warnings from BSD code, I hope you think -Werror should be
off by default until the warnings are cleaned up.

For RTEMS, we try not to modify code imported from third parties. We have
a rule that you can only add code inside an ifdef __rtems__. For example,
if
we wanted to eliminate those unused variable warnings, we would likely do
something like this in third party code:

#ifndef __rtems__
  int unused_variable;
#endif

And, if being really diligent, post a patch or ticket to the upstream
project.
Our goal is to make it easier to update these files in the future.

FWIW we have over 1M lines of FreeBSD code in rtems-libbsd which is
a port of the FreeBSD networking, USB, nvme, etc. and the commands
to configure the stack (ifconfig, route, netstat, ping, etc) This has worked
pretty well for us.

--joel


> -mike
>

Re: -Wall

[Liviu Ionescu]

> On 19 Jan 2024, at 14:55, Corinna Vinschen <vinschen@redhat.com> wrote:
> 
> ... I'd like to suggest to add -Wall by default to the
> build flags for newlib, just as it is already for ages in the Cygwin
> tree.
> 
> Anybody having a strong opinion, pro or contra?

For my own code I run all tests with `-Werror -Wall` and as many extra `-fxxx` options I can (trying to emulate the LLVM/clang `-feverything`). This usually requires a combination of code edits and pragmas to silence some warnings, which may be tedious, but it gives me some peace of mind when my code is integrated in various build environments.

However, when I make binary distributions using someone else code (*), I find compiler warnings less useful (read annoying), since there is not much I can do to fix them.

For the newlib configure, if not already available, I suggest you implement `--enable-warnings`/`--disable-warnings` and `--enable-werror`/`--disable-werror`.

The default are usually disabled, but are not that relevant; for my builds I generally use explicit `--disable-warnings --disable-werror` when available.

But for your CI tests, sure, enable `-Wall` and try to fix the code to clear all warnings, using the latest toolchains.


Regards,

Liviu


(*) I maintain the xPack binary cross-platform distributions of arm-none-eabi-gcc and riscv-none-elf-gcc, which also include newlib.

Re: -Wall

[Liviu Ionescu]

> On 19 Jan 2024, at 18:23, Liviu Ionescu <ilg@livius.net> wrote:
> 
>  as many extra `-fxxx` options I can (trying to emulate the LLVM/clang `-feverything

oops! actually `-Weverything` :-(

Liviu

Re: -Wall

[brian.inglis]

On 2024-01-19 05:55, Corinna Vinschen wrote:
> Hi folks,
> 
> if you saw my today's pushes, you're aware that I only found a bug
> because I used -Wall.  I fixed the bug and a few less crucial warnings.
> 
> I did NOT fix the warnings in code we took verbatim from some BSDs,
> which often contain unused variables, or in some cases expressions which
> are deemed to profit from extra paranthesis, e. g.
> 
>    if (a >= 0 ^ b == 0)
> 
> For that reason, I'd like to suggest to add -Wall by default to the
> build flags for newlib, just as it is already for ages in the Cygwin
> tree.
> 
> Anybody having a strong opinion, pro or contra?

++

I also like:

	-Wextra -Wformat=2 -Wformat-overflow=2 -Werror=format-security

to get more useful warnings, and error if there are security issues like totally 
variable format strings; use `info gcc W...` for descriptions; YL/100kmMV

I have also found the following work well for development with recent gcc:

	-fanalyzer -fsanitize-recover=all
	-fstack-check -fstack-protector-all
	--param=ssp-buffer-size=4

but may be inappropriate for production builds; use `info gcc f...` for 
descriptions.

-- 
Take care. Thanks, Brian Inglis              Calgary, Alberta, Canada

La perfection est atteinte                   Perfection is achieved
non pas lorsqu'il n'y a plus rien à ajouter  not when there is no more to add
mais lorsqu'il n'y a plus rien à retirer     but when there is no more to cut
                                  -- Antoine de Saint-Exupéry

Re: -Wall

[brian.inglis]

On 2024-01-20 11:17, brian.inglis@systematicsw.ab.ca wrote:
> On 2024-01-19 05:55, Corinna Vinschen wrote:
>> if you saw my today's pushes, you're aware that I only found a bug
>> because I used -Wall.  I fixed the bug and a few less crucial warnings.
>> I did NOT fix the warnings in code we took verbatim from some BSDs,
>> which often contain unused variables, or in some cases expressions which
>> are deemed to profit from extra paranthesis, e. g.
>>    if (a >= 0 ^ b == 0)
>> For that reason, I'd like to suggest to add -Wall by default to the
>> build flags for newlib, just as it is already for ages in the Cygwin
>> tree.
>> Anybody having a strong opinion, pro or contra?

> ++
> I also like:
>      -Wextra -Wformat=2 -Wformat-overflow=2 -Werror=format-security
> to get more useful warnings, and error if there are security issues like totally 
> variable format strings; use `info gcc W...` for descriptions; YL/100kmMV
> I have also found the following work well for development with recent gcc:
>      -fanalyzer -fsanitize-recover=all
>      -fstack-check -fstack-protector-all
>      --param=ssp-buffer-size=4
> but may be inappropriate for production builds; use `info gcc f...` for 
> descriptions.

Linux added -Wstringop-overflow to diagnose provable buffer overflows, except 
with GCC == 11 which mishandles the test:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=610347effc2ecb5ededf5037e82240b151f883ab

For extra Linux warning flags W=1/2/3 available see:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/Makefile.extrawarn

-- 
Take care. Thanks, Brian Inglis              Calgary, Alberta, Canada

La perfection est atteinte                   Perfection is achieved
non pas lorsqu'il n'y a plus rien à ajouter  not when there is no more to add
mais lorsqu'il n'y a plus rien à retirer     but when there is no more to cut
                                 -- Antoine de Saint-Exupéry

Re: -Wall

[Corinna Vinschen]

On Jan 23 22:38, brian.inglis@systematicsw.ab.ca wrote:
> On 2024-01-20 11:17, brian.inglis@systematicsw.ab.ca wrote:
> > On 2024-01-19 05:55, Corinna Vinschen wrote:
> > > if you saw my today's pushes, you're aware that I only found a bug
> > > because I used -Wall.  I fixed the bug and a few less crucial warnings.
> > > I did NOT fix the warnings in code we took verbatim from some BSDs,
> > > which often contain unused variables, or in some cases expressions which
> > > are deemed to profit from extra paranthesis, e. g.
> > >    if (a >= 0 ^ b == 0)
> > > For that reason, I'd like to suggest to add -Wall by default to the
> > > build flags for newlib, just as it is already for ages in the Cygwin
> > > tree.
> > > Anybody having a strong opinion, pro or contra?
> 
> > ++
> > I also like:
> >      -Wextra -Wformat=2 -Wformat-overflow=2 -Werror=format-security
> > to get more useful warnings, and error if there are security issues like
> > totally variable format strings; use `info gcc W...` for descriptions;
> > YL/100kmMV
> > I have also found the following work well for development with recent gcc:
> >      -fanalyzer -fsanitize-recover=all
> >      -fstack-check -fstack-protector-all
> >      --param=ssp-buffer-size=4
> > but may be inappropriate for production builds; use `info gcc f...` for
> > descriptions.
> 
> Linux added -Wstringop-overflow to diagnose provable buffer overflows,
> except with GCC == 11 which mishandles the test:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=610347effc2ecb5ededf5037e82240b151f883ab
> 
> For extra Linux warning flags W=1/2/3 available see:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/Makefile.extrawarn

This is going a tad bit too far.  I was just asking if we should add
-Wall unconditionally.

I'm fully aware that this doesn't cover all possible warnings an
over-protective compiler can generate, but this is, IMHO, a medium
sized compromise.

Ok or not?


Corinna

Re: -Wall

[Liviu Ionescu]

> On 24 Jan 2024, at 13:19, Corinna Vinschen <vinschen@redhat.com> wrote:
> 
> I was just asking if we should add
> -Wall unconditionally.
> 
> Ok or not?

unconditionally: not, because there is nothing for me (as a third party package maintainer) to do when I see those warnings
behind `--enable-warnings`: definitely yes

Liviu

Re: -Wall

[Corinna Vinschen]

On Jan 24 13:25, Liviu Ionescu wrote:
> 
> 
> > On 24 Jan 2024, at 13:19, Corinna Vinschen <vinschen@redhat.com> wrote:
> > 
> > I was just asking if we should add
> > -Wall unconditionally.
> > 
> > Ok or not?
> 
> unconditionally: not, because there is nothing for me (as a third party package maintainer) to do when I see those warnings
> behind `--enable-warnings`: definitely yes

Behind --disable-warnings, if so.  -Wall should at least be default.


Corinna

Re: -Wall

[Liviu Ionescu]

> On 19 Jan 2024, at 14:55, Corinna Vinschen <vinschen@redhat.com> wrote:
> 
> ... I only found a bug because I used -Wall ...

For completeness, please note that `-Wall` does not enable **all* warnings, GCC has additional warnings, that might be useful to spot other bugs.

Below is an excerpt from my CMake scripts.

I did not check recent GCC releases, they probably include even more warnings.

Personally I'd use as many of them when `--enable-warnings`.

Regards,

Liviu


```
if("${CMAKE_C_COMPILER_VERSION}" VERSION_GREATER_EQUAL "7.0.0")

# message(VERBOSE "Adding GCC 7 warnings...")

list(APPEND ${variable_name}

# ---------------------------------------------------------------------
# Common GNU C & C++.

-Waggregate-return
-Wcast-align
-Wcast-qual
-Wconversion
-Wdouble-promotion
-Wduplicated-branches
-Wduplicated-cond
-Wextra
-Wfloat-conversion
-Wfloat-equal
-Wformat-nonliteral
-Wformat-overflow=2
-Wformat-security
-Wformat-signedness
-Wformat-truncation=2
-Wformat-y2k
-Wformat=2
-Wlogical-op
-Wmissing-declarations
# Older GCC may include non-existent SDK folders.
$<$<NOT:$<PLATFORM_ID:Darwin>>:-Wmissing-include-dirs>
-Wnull-dereference
-Wpacked
-Wpadded
-Wpointer-arith
-Wredundant-decls
-Wshadow
-Wshift-overflow=2
-Wsign-conversion
-Wswitch-default
-Wswitch-enum
-Wundef
-Wuninitialized
-Wvla

# ---------------------------------------------------------------------
# GNU C only.

$<$<COMPILE_LANGUAGE:C>:-Wbad-function-cast>
$<$<COMPILE_LANGUAGE:C>:-Wc++-compat>
$<$<COMPILE_LANGUAGE:C>:-Wduplicate-decl-specifier>
$<$<COMPILE_LANGUAGE:C>:-Wmissing-prototypes>
$<$<COMPILE_LANGUAGE:C>:-Wnested-externs>
$<$<COMPILE_LANGUAGE:C>:-Wold-style-definition>
$<$<COMPILE_LANGUAGE:C>:-Wstrict-prototypes>

# ---------------------------------------------------------------------
# GNU C++ only.

# inherits the "cxx11" ABI tag that 'std::string'
# $<$<COMPILE_LANGUAGE:CXX>:-Wabi-tag>

$<$<COMPILE_LANGUAGE:CXX>:-Wctor-dtor-privacy>
$<$<COMPILE_LANGUAGE:CXX>:-Wnoexcept>
$<$<COMPILE_LANGUAGE:CXX>:-Wnon-virtual-dtor>
$<$<COMPILE_LANGUAGE:CXX>:-Wold-style-cast>
$<$<COMPILE_LANGUAGE:CXX>:-Woverloaded-virtual>
$<$<COMPILE_LANGUAGE:CXX>:-Wplacement-new=2>
$<$<COMPILE_LANGUAGE:CXX>:-Wregister>
$<$<COMPILE_LANGUAGE:CXX>:-Wsign-promo>
$<$<COMPILE_LANGUAGE:CXX>:-Wstrict-null-sentinel>
$<$<COMPILE_LANGUAGE:CXX>:-Wsuggest-final-methods>
$<$<COMPILE_LANGUAGE:CXX>:-Wsuggest-final-types>
$<$<COMPILE_LANGUAGE:CXX>:-Wsuggest-override>
$<$<COMPILE_LANGUAGE:CXX>:-Wuseless-cast>
$<$<COMPILE_LANGUAGE:CXX>:-Wzero-as-null-pointer-constant>
)

endif()

if("${CMAKE_C_COMPILER_VERSION}" VERSION_GREATER_EQUAL "8.0.0")

# message(VERBOSE "Adding GCC 8 warnings...")

list(APPEND ${variable_name}

$<$<COMPILE_LANGUAGE:CXX>:-Wextra-semi>

)

endif()

if("${CMAKE_C_COMPILER_VERSION}" VERSION_GREATER_EQUAL "9.0.0")

# message(VERBOSE "Adding GCC 9 warnings...")

list(APPEND ${variable_name}

# None so far.
)

endif()

if("${CMAKE_C_COMPILER_VERSION}" VERSION_GREATER_EQUAL "10.0.0")

# message(VERBOSE "Adding GCC 10 warnings...")

list(APPEND ${variable_name}

# ---------------------------------------------------------------------
# Common GNU C & C++.

-Warith-conversion

# ---------------------------------------------------------------------
# GNU C++ only.

$<$<COMPILE_LANGUAGE:CXX>:-Wcomma-subscript>
$<$<COMPILE_LANGUAGE:CXX>:-Wredundant-tags>
$<$<COMPILE_LANGUAGE:CXX>:-Wvolatile>
)

# RISC-V 10.1 fails with `note: replace the class-key with 'struct'`.
if(NOT "${CMAKE_C_COMPILER_VERSION}" VERSION_EQUAL "10.1.0")

list(APPEND ${variable_name}

$<$<COMPILE_LANGUAGE:CXX>:-Wmismatched-tags>
)

endif()

endif()

elseif("${CMAKE_C_COMPILER_ID}" MATCHES ".*Clang")

message(VERBOSE "Adding clang warnings...")

list(APPEND ${variable_name}

# For clang things are much easier.
-Weverything
$<$<C_COMPILER_ID:ARMClang>:-Wno-poison-system-directories>
)

endif()
```