-flto and -Werror

-flto and -Werror

[Matthias Klose]

Using -flto exposes some new warnings in code, as seen in the both build logs
below, for upstream elfutils and systemd.  I have seen others.  These upstreams
enable -Werror by default, but probably don't see these warnings turning to
errors themself, because the LTO flags are usually injected by the packaging tools.

e.g.
https://launchpadlibrarian.net/536740411/buildlog_ubuntu-hirsute-ppc64el.systemd_248.666.gd4d7127d94+21.04.20210503043716_BUILDING.txt.gz
e.g.
https://launchpadlibrarian.net/536683989/buildlog_ubuntu-hirsute-amd64.elfutils_0.183.43.g92980edc+21.04.20210502190301_BUILDING.txt.gz

showing:

../src/shared/efi-loader.c: In function ‘efi_get_reboot_to_firmware’:
../src/shared/efi-loader.c:168:16: error: ‘b’ may be used uninitialized in this
function [-Werror=maybe-uninitialized]

i386_lex.c: In function ‘i386_restart’:
i386_lex.c:1816:25: error: potential null pointer dereference
[-Werror=null-dereference]
 1816 |         b->yy_bs_column = 0;

A coworker worked out by review that these warnings are false positives.  Now
the first option already has the *maybe* in it's name, the second option gives
this hint in the message (*potentially*).  Now getting the complaint that
-Werror isn't usable with -flto anymore.

Would it make sense to mark warnings with a high potential of false positives,
which are not turned into errors with -Werror? And only turn these into errors
with a new option, e.g. -Wall-errors?

Matthias

Re: -flto and -Werror

[David Brown]

On 04/05/2021 14:39, Matthias Klose wrote:
> Using -flto exposes some new warnings in code, as seen in the both build logs
> below, for upstream elfutils and systemd.  I have seen others.  These upstreams
> enable -Werror by default, but probably don't see these warnings turning to
> errors themself, because the LTO flags are usually injected by the packaging tools.
> 
> e.g.
> https://launchpadlibrarian.net/536740411/buildlog_ubuntu-hirsute-ppc64el.systemd_248.666.gd4d7127d94+21.04.20210503043716_BUILDING.txt.gz
> e.g.
> https://launchpadlibrarian.net/536683989/buildlog_ubuntu-hirsute-amd64.elfutils_0.183.43.g92980edc+21.04.20210502190301_BUILDING.txt.gz
> 
> showing:
> 
> ../src/shared/efi-loader.c: In function ‘efi_get_reboot_to_firmware’:
> ../src/shared/efi-loader.c:168:16: error: ‘b’ may be used uninitialized in this
> function [-Werror=maybe-uninitialized]
> 
> i386_lex.c: In function ‘i386_restart’:
> i386_lex.c:1816:25: error: potential null pointer dereference
> [-Werror=null-dereference]
>  1816 |         b->yy_bs_column = 0;
> 
> A coworker worked out by review that these warnings are false positives.  Now
> the first option already has the *maybe* in it's name, the second option gives
> this hint in the message (*potentially*).  Now getting the complaint that
> -Werror isn't usable with -flto anymore.
> 
> Would it make sense to mark warnings with a high potential of false positives,
> which are not turned into errors with -Werror? And only turn these into errors
> with a new option, e.g. -Wall-errors?
> 
> Matthias
> 

I don't think that would make sense.  Compiling with -Werror is only
appropriate if you have a specific compiler version and a specific set
of warning flags - otherwise new warnings (either from different flags,
or a different compiler version) may cause your build to fail.  That's
the price you pay for the benefits of static error analysis and for
using -Werror to ensure that your code is checked against your set of
warnings.

Personally, I use -Werror in my own builds - but like most
warning-related flags, it is a flag you use during development (and thus
the solution in this case is probably to fix the possible issue in the
code), not for a bundle of known-good code for distribution and builds.

(That's my opinion on such flags.  Other opinions are available
elsewhere :-) )

David

Re: -flto and -Werror

[Martin Sebor]

On 5/4/21 6:39 AM, Matthias Klose wrote:
> Using -flto exposes some new warnings in code, as seen in the both build logs
> below, for upstream elfutils and systemd.  I have seen others.  These upstreams
> enable -Werror by default, but probably don't see these warnings turning to
> errors themself, because the LTO flags are usually injected by the packaging tools.
> 
> e.g.
> https://launchpadlibrarian.net/536740411/buildlog_ubuntu-hirsute-ppc64el.systemd_248.666.gd4d7127d94+21.04.20210503043716_BUILDING.txt.gz
> e.g.
> https://launchpadlibrarian.net/536683989/buildlog_ubuntu-hirsute-amd64.elfutils_0.183.43.g92980edc+21.04.20210502190301_BUILDING.txt.gz
> 
> showing:
> 
> ../src/shared/efi-loader.c: In function ‘efi_get_reboot_to_firmware’:
> ../src/shared/efi-loader.c:168:16: error: ‘b’ may be used uninitialized in this
> function [-Werror=maybe-uninitialized]
> 
> i386_lex.c: In function ‘i386_restart’:
> i386_lex.c:1816:25: error: potential null pointer dereference
> [-Werror=null-dereference]
>   1816 |         b->yy_bs_column = 0;
> 
> A coworker worked out by review that these warnings are false positives.  Now
> the first option already has the *maybe* in it's name, the second option gives
> this hint in the message (*potentially*).  Now getting the complaint that
> -Werror isn't usable with -flto anymore.
> 
> Would it make sense to mark warnings with a high potential of false positives,
> which are not turned into errors with -Werror? And only turn these into errors
> with a new option, e.g. -Wall-errors?

I'm not in favor of adding a new option quite like that for a few
reasons but I wonder if the problem you're raising would be better
solved slightly differently.

-Wmaybe-uninitialized has a relatively higher rate of false positives
than most other warnings.  It's controlled by its own option for that
reason, so a mechanism for keeping it from causing errors with -Werror
already exists (-Wno-error=maybe-uninitialized).

-Wnull-dereference is disabled by default and not included in either
-Wall or -Wextra, and it too can be prevented from causing errors by
simply using -Wno-error=null-dereference (when it's enabled).

In general, all warnings have a nonzero rate of false positives,
especially the "late" ones that depend on optimization, so whether
one rate is high is not a question that can be answered objectively.
I would expect adding a new option with the proposed effect to lead
to the same protracted and never-resolved debates about which warning
should be included in -Wall, which ones in -Wextra, and which ones
not enabled at all.

That being said, I think introducing some general mechanism for
controlling all "maybe" kinds of flow-sensitive warnings like
-Wmaybe-uninitialized would be useful, and extending the same
heuristic to other warnings (the "if there exists a path from
the function entry to the problem site" aspect).

Independently, if that would help solve the problem you're pointing
out, I would also support adding a general option to control warnings
issued during the LTO stage so that they could be prevented from
casing errors while those issued earlier still could.  Say -Werror=lto
(or something like that).

Martin