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
