git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Denton Liu <liu.denton@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH] Fix -Wmaybe-uninitialized warnings under -O0
Date: Sat, 4 Apr 2020 08:07:43 -0400	[thread overview]
Message-ID: <20200404120743.GA636417@generichostname> (raw)
In-Reply-To: <20200403140447.GA639586@coredump.intra.peff.net>

Hi Peff,

On Fri, Apr 03, 2020 at 10:04:47AM -0400, Jeff King wrote:
> On Wed, Apr 01, 2020 at 10:06:43AM -0400, Denton Liu wrote:
> 
> > > So why does your version behave differently? And if this is a temporary
> > > state for a buggy version of gcc (that may be fixed in the next point
> > > release), is it worth changing our source code to appease it?
> > 
> > A correction to the earlier message... It seems like I wasn't reporting
> > the correct settings. I was actually compiling with -Og, not -O0
> > (whoops!).
> > 
> > I tested it with gcc-8 and it seems like it also reports the same
> > problem. Also, -O1 reports warnings as well.
> 
> Ah, OK, I can reproduce easily with -Og (up through gcc-10). Most of
> them don't trigger with -O1; just the one in ref-filter.c.
> 
> That one's interesting. We have:
> 
>   int ret = 0;
>   ...
>   if (...)
>          ...
>   else
>          ret = for_each_fullref_in_pattern(...);
>   ...
>   return ret;
> 
> So we'd either have 0 or an assigned return. But the bug is actually in
> for_each_fullref_in_pattern(), which does this:
> 
>   int ret; /* uninitialized! */
> 
>   /* a bunch of early return conditionals */
>   if (...)
>     return ...;
> 
>   for_each_string_list_item(...) {
>     ret = for_each_fullref_in(...);

This loop is missing a bit of important context:

	if (ret)
		break;

>   }
> 
>   return ret;
> 
> but that will return an uninitialized value when there are no patterns.
> I doubt we have such a case, but that may explain why -O0 does not
> complain (it assumes "in_pattern" will return a useful value) and -O2
> does not (it is able to figure out that it always does), but -O1 only
> inlines part of it.
> 
> Curiously, -Og _does_ find the correct function.
> 
> Your patch silences it, but is it doing the right thing? It sets "ret =
> 0", but we haven't actually iterated anything. Should it be an error
> instead?

I understood the semantics of for_each_fullref_in_pattern() (in the
non-early return case) to be "for each item, iterate and compute a
value; if that value is non-zero return it. If not found, return zero".
The missing context above is important since, without it, we lose the
semantics.

If I'm understanding the above correctly then, studying this function in
a vacuum, it would make sense to assign a default value of 0 since if
the for operates on an empty list, the function should consider the item
vacuously not found and we should return 0 as a result.

This was the type of analysis I applied to the other changes. I'll admit
that I studied the other functions in a vacuum as well since these
seemed to be superficial warnings (since they aren't triggered with
-O{0,2}) which indicated to me that the code was correct except for some
"cosmetic" errors. I dunno, perhaps this is my inexperience showing
through.

Thanks,

Denton

P.S. Do we want to quash the -O3 warnings as well?

> -Peff

  parent reply	other threads:[~2020-04-04 12:07 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-01  7:30 Denton Liu
     [not found] ` <CAPUEspgBkmxszgBee8C9hZnEwqztf-XKEj7LB_jWVFJaJCge0w@mail.gmail.com>
2020-04-01  9:05   ` Denton Liu
2020-04-01  9:52 ` Jeff King
2020-04-01 14:06   ` Denton Liu
2020-04-03 14:04     ` Jeff King
2020-04-03 14:38       ` Jeff King
2020-04-04 12:07       ` Denton Liu [this message]
2020-04-04 14:21         ` Jeff King
2021-05-05  8:40           ` [PATCH] trace2: refactor to avoid gcc warning under -O3 Ævar Arnfjörð Bjarmason
2021-05-05  9:47             ` Junio C Hamano
2021-05-05 13:34               ` Jeff King
2021-05-05 14:38             ` Johannes Schindelin
2021-05-06  1:26               ` Junio C Hamano
2021-05-06 20:29                 ` Johannes Schindelin
2021-05-06 21:10                   ` Junio C Hamano
2021-05-11 14:34                     ` Johannes Schindelin
2021-05-11 18:00                       ` Jeff King
2021-05-11 20:58                         ` Junio C Hamano
2021-05-11 21:07                           ` Jeff King
2021-05-11 21:33                             ` Junio C Hamano
2021-05-11  7:03             ` Junio C Hamano
2021-05-11 13:04             ` [PATCH v2] " Ævar Arnfjörð Bjarmason
2021-05-11 16:40               ` Jeff Hostetler
2021-05-11 17:54               ` Jeff King
2021-05-11 18:08                 ` Jeff King
2021-05-11 21:09                   ` Junio C Hamano
2021-05-20  0:20                   ` Junio C Hamano
2021-05-20 11:05                     ` [PATCH v3] " Ævar Arnfjörð Bjarmason
2021-05-20 13:13                       ` Jeff King
2021-05-20 22:08                         ` Junio C Hamano
2021-05-21  9:34                           ` Jeff 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

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200404120743.GA636417@generichostname \
    --to=liu.denton@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    --subject='Re: [PATCH] Fix -Wmaybe-uninitialized warnings under -O0' \
    /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

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).