From: Jeff King <peff@peff.net>
To: Denton Liu <liu.denton@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH] Fix -Wmaybe-uninitialized warnings under -O0
Date: Sat, 4 Apr 2020 10:21:31 -0400 [thread overview]
Message-ID: <20200404142131.GA679473@coredump.intra.peff.net> (raw)
In-Reply-To: <20200404120743.GA636417@generichostname>
On Sat, Apr 04, 2020 at 08:07:43AM -0400, Denton Liu wrote:
> > for_each_string_list_item(...) {
> > ret = for_each_fullref_in(...);
>
> This loop is missing a bit of important context:
>
> if (ret)
> break;
>
Yes, I omitted it because it's not relevant to whether ret is
initialized or not (i.e., if we enter the loop then it always is.
But I think it is to the argument you make below.
> > 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.
I think the break on "ret" is better understood as "if we saw an error,
return it; otherwise keep going".
If we were given zero patterns, is that a noop success, or is it an
error? This should never happen because we cover that case earlier, so
it would be a bug in find_longest_prefixes. Perhaps:
diff --git a/ref-filter.c b/ref-filter.c
index b1812cb69a..b358567663 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1964,6 +1964,8 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter,
}
find_longest_prefixes(&prefixes, filter->name_patterns);
+ if (!prefixes.nr)
+ BUG("no common ref prefix?");
for_each_string_list_item(prefix, &prefixes) {
ret = for_each_fullref_in(prefix->string, cb, cb_data, broken);
is worth doing to document that assumption. Ideally that would then let
the compiler know that we'll always enter the loop, but it doesn't seem
to be quite the clever.
So I dunno. I think it probably doesn't matter between "0" and "-1",
because this case really shouldn't be reachable.
> 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.
Yeah, the code is correct in this case, and I don't think the
uninitialized case can be reached. But the subtle linkage here is that
patterns[0] being non-NULL means that find_longest_prefixes() will never
return a zero-length list, which means we'll always enter that loop at
least once.
> P.S. Do we want to quash the -O3 warnings as well?
They're probably worth looking at. I've periodically swept through and
fixed them, as recently as last September (try "git log --grep=-O3").
But new ones seem to have cropped up. I'm not sure if they were
introduced in the code or if the compiler got smarter (or dumber).
Just skimming the output, I see:
In function ‘ll_binary_merge’,
inlined from ‘ll_xdl_merge’ at ll-merge.c:115:10,
inlined from ‘ll_union_merge’ at ll-merge.c:151:9:
ll-merge.c:74:4: error: ‘%s’ directive argument is null [-Werror=format-overflow=]
74 | warning("Cannot merge binary files: %s (%s vs. %s)",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
75 | path, name1, name2);
| ~~~~~~~~~~~~~~~~~~~
This one seems likely to be accurate (and just uncovered by more
aggressive inlining). The union_merge function passes in NULL filenames,
and probably could trigger this warning on binary input.
trace2/tr2_dst.c: In function ‘tr2_dst_get_trace_fd.part.0’:
trace2/tr2_dst.c:296:10: error: ‘fd’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
296 | dst->fd = fd;
| ~~~~~~~~^~~~
trace2/tr2_dst.c:229:6: note: ‘fd’ was declared here
229 | int fd;
| ^~
This one looks like a false positive. The tr2 code does something
unusual for our code base: on error it returns errno. And I think the
compiler is not aware that errno would not be zero after a syscall
failure. It might be worth changing the return value to match our usual
pattern (return -1, and the caller can look in errno), which would
appease the compiler as a side effect.
revision.c: In function ‘do_add_index_objects_to_pending’:
revision.c:322:22: error: array subscript [1, 2147483647] is outside array bounds of ‘char[1]’ [-Werror=array-bounds]
322 | if (0 < len && name[len] && buf.len)
| ~~~~^~~~~
This one is confusing. I imagine the char[1] is the empty string we
frequently pass to add_pending_object() and friends. And the "len" here
comes from interpret_branch_name(name, ...). So somehow the compiler
doesn't quite know that the length we'll return is going to be less than
or equal to the string length we pass in.
I don't know how to fix that aside from this, which isn't great (btw, it
looks like a lot of this code could stand to switch to using ssize_t
instead of int; there are probably weird errors if you managed to
somehow feed a 2GB branch name).
diff --git a/revision.c b/revision.c
index 8136929e23..9bc398bec1 100644
--- a/revision.c
+++ b/revision.c
@@ -317,9 +317,10 @@ static void add_pending_object_with_path(struct rev_info *revs,
revs->no_walk = 0;
if (revs->reflog_info && obj->type == OBJ_COMMIT) {
struct strbuf buf = STRBUF_INIT;
- int len = interpret_branch_name(name, 0, &buf, 0);
+ size_t namelen = strlen(name);
+ int len = interpret_branch_name(name, namelen, &buf, 0);
- if (0 < len && name[len] && buf.len)
+ if (len == namelen && buf.len)
strbuf_addstr(&buf, name + len);
add_reflog_for_walk(revs->reflog_info,
(struct commit *)obj,
-Peff
next prev parent reply other threads:[~2020-04-04 14:21 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-01 7:30 [PATCH] Fix -Wmaybe-uninitialized warnings under -O0 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
2020-04-04 14:21 ` Jeff King [this message]
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=20200404142131.GA679473@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=liu.denton@gmail.com \
/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.
Code repositories for project(s) associated with this public 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).