From: Jakub Jelinek <jakub@redhat.com>
To: Ingo Molnar <mingo@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
Colin King <colin.king@canonical.com>,
Ingo Molnar <mingo@redhat.com>,
linux-kernel@vger.kernel.org, Richard Henderson <rth@twiddle.net>,
Dan Carpenter <dan.carpenter@oracle.com>
Subject: Re: Q: why didn't GCC warn about this uninitialized variable? (was: Re: [PATCH] perf tests: initialize sa.sa_flags)
Date: Thu, 3 Mar 2016 13:55:42 +0100 [thread overview]
Message-ID: <20160303125542.GD3017@tucnak.redhat.com> (raw)
In-Reply-To: <20160303121944.GB2484@gmail.com>
On Thu, Mar 03, 2016 at 01:19:44PM +0100, Ingo Molnar wrote:
> struct sigaction sa;
>
> ...
>
> sigfillset(&sa.sa_mask);
> sa.sa_sigaction = segfault_handler;
> sigaction(SIGSEGV, &sa, NULL);
>
> ... which uninitialized sa.sa_flags field GCC merrily accepted as proper C code,
> despite us turning on essentially _all_ GCC warnings for the perf build that exist
> under the sun:
GCC -W{,maybe-}uninitialized warning works only on SSA form, so in order to
get that warning, either it needs to be some scalar that is uninitialized,
or Scalar Replacement of Aggregates needs to be able to turn the structure
into independent scalars. Neither is the case here, as you take address of
the struct when passing its address to sigaction, and that call can't be
inlined nor is in any other way visible to the compiler, so that it could
optimize both the caller and sigaction itself at the same time.
Even if GCC added infrastructure for tracking which bits/bytes in
aggregates are or might be uninitialized at which point, generally,
struct XYZ abc;
foo (&abc);
is so common pattern that warning here that the fields are uninitialized
would have extremely high false positive ratio.
Even if as somebody mentioned that the argument is const struct sigaction *
rather than struct sigaction *, that doesn't change really anything,
you can cast away the constness and still write into it in the other
function.
Furthermore, in many APIs, only a subset of fields need to be initialized
unconditionally, and other fields might need to be initialized only
conditionally, depending on those always initialized fields, other
parameters to functions, etc.
So, in order to warn here, we'd need some assurance (new attribute on
sigaction function?) that when it is called, it has to have all named fields in
the pointed to structures initialized (perhaps attribute like nonnull, which
can either apply to all pointer arguments, or selected ones) at the entry of
the function and not initializing them all is a bug.
So, this really isn't as trivial as you might think it is.
Jakub
next prev parent reply other threads:[~2016-03-03 12:55 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-02 12:55 [PATCH] perf tests: initialize sa.sa_flags Colin King
2016-03-02 12:59 ` Peter Zijlstra
2016-03-02 13:03 ` Arnaldo Carvalho de Melo
2016-03-02 13:21 ` Peter Zijlstra
2016-03-02 13:23 ` Arnaldo Carvalho de Melo
2016-03-03 12:19 ` Q: why didn't GCC warn about this uninitialized variable? (was: Re: [PATCH] perf tests: initialize sa.sa_flags) Ingo Molnar
2016-03-03 12:25 ` Q: why didn't GCC warn about this uninitialized variable? Colin Ian King
2016-03-03 12:31 ` Måns Rullgård
2016-03-03 12:43 ` Ingo Molnar
2016-03-03 12:49 ` Joe Perches
2016-03-03 12:55 ` Jakub Jelinek [this message]
2016-03-03 13:24 ` Q: why didn't GCC warn about this uninitialized variable? (was: Re: [PATCH] perf tests: initialize sa.sa_flags) Ingo Molnar
2016-03-03 13:46 ` Jakub Jelinek
2016-03-03 14:04 ` Ingo Molnar
2016-03-03 13:47 ` Ingo Molnar
2016-03-03 14:19 ` Jakub Jelinek
2016-03-03 14:40 ` Ingo Molnar
2016-03-03 14:53 ` Ingo Molnar
2016-03-03 15:04 ` Ingo Molnar
2016-03-02 13:02 ` [PATCH] perf tests: initialize sa.sa_flags Arnaldo Carvalho de Melo
2016-03-05 8:20 ` [tip:perf/core] perf tests: Initialize sa.sa_flags tip-bot for Colin Ian King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160303125542.GD3017@tucnak.redhat.com \
--to=jakub@redhat.com \
--cc=acme@kernel.org \
--cc=colin.king@canonical.com \
--cc=dan.carpenter@oracle.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=rth@twiddle.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox