| Message ID | 20190905082459.26816-1-s-beyer@gmx.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix maybe-uninitialized warnings found by gcc 9 -flto | expand |
Am 05.09.19 um 10:24 schrieb Stephan Beyer: > Compiler heuristics for detection of potentially uninitialized variables > may change between compiler versions and enabling link-time optimization > may find new warnings. Indeed, compiling with gcc 9.2.1 and enabled > link-time optimization feature resulted in a few hits that are fixed by > this patch in the most naïve way. This allows to compile git using the > DEVELOPER=1 switch (which sets -Werror) and using the -flto flag. > > Signed-off-by: Stephan Beyer <s-beyer@gmx.net> > --- > builtin/am.c | 2 +- > builtin/pack-objects.c | 2 +- > bulk-checkin.c | 2 ++ > fast-import.c | 3 ++- > t/helper/test-read-cache.c | 2 +- > 5 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/builtin/am.c b/builtin/am.c > index 1aea657a7f..ab914fd46e 100644 > --- a/builtin/am.c > +++ b/builtin/am.c > @@ -1266,7 +1266,7 @@ static int get_mail_commit_oid(struct object_id *commit_id, const char *mail) > static void get_commit_info(struct am_state *state, struct commit *commit) > { > const char *buffer, *ident_line, *msg; > - size_t ident_len; > + size_t ident_len = 0; > struct ident_split id; > > buffer = logmsg_reencode(commit, NULL, get_commit_output_encoding()); Further context: ident_line = find_commit_header(buffer, "author", &ident_len); if (split_ident_line(&id, ident_line, ident_len) < 0) die(_("invalid ident line: %.*s"), (int)ident_len, ident_line); find_commit_header() can return NULL. split_ident_line() won't handle that well. So I think what's missing here is a NULL check. If the compiler is smart enough then that should silence the initialization warning. > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c > index 76ce906946..d0c03b0e9b 100644 > --- a/builtin/pack-objects.c > +++ b/builtin/pack-objects.c > @@ -1171,7 +1171,7 @@ static int add_object_entry(const struct object_id *oid, enum object_type type, > { > struct packed_git *found_pack = NULL; > off_t found_offset = 0; > - uint32_t index_pos; > + uint32_t index_pos = 0; > > display_progress(progress_state, ++nr_seen); > Further context: if (have_duplicate_entry(oid, exclude, &index_pos)) return 0; if (!want_object_in_pack(oid, exclude, &found_pack, &found_offset)) { /* The pack is missing an object, so it will not have closure */ if (write_bitmap_index) { if (write_bitmap_index != WRITE_BITMAP_QUIET) warning(_(no_closure_warning)); write_bitmap_index = 0; } return 0; } create_object_entry(oid, type, pack_name_hash(name), exclude, name && no_try_delta(name), index_pos, found_pack, found_offset); So we call have_duplicate_entry() and if it returns 0 then we might end up using index_pos. So when does it return 0? static int have_duplicate_entry(const struct object_id *oid, int exclude, uint32_t *index_pos) { struct object_entry *entry; entry = packlist_find(&to_pack, oid, index_pos); if (!entry) return 0; OK, it does that if packlist_find() returns NULL. When does it do that? struct object_entry *packlist_find(struct packing_data *pdata, const struct object_id *oid, uint32_t *index_pos) { uint32_t i; int found; if (!pdata->index_size) return NULL; i = locate_object_entry_hash(pdata, oid, &found); if (index_pos) *index_pos = i; if (!found) return NULL; So if the packing list is empty then it returns NULL without setting index_pos. Hmm. It does set it in all other cases, no matter if oid is found or not. Is it really a good idea to make that exception? I suspect always setting index_pos here would silence the compiler as well and fix the issue closer to its root. But I may be missing something, this code looks complicated. > diff --git a/bulk-checkin.c b/bulk-checkin.c > index 39ee7d6107..87fa28c227 100644 > --- a/bulk-checkin.c > +++ b/bulk-checkin.c > @@ -200,6 +200,8 @@ static int deflate_to_pack(struct bulk_checkin_state *state, > struct hashfile_checkpoint checkpoint; > struct pack_idx_entry *idx = NULL; > > + checkpoint.offset = 0; > + > seekback = lseek(fd, 0, SEEK_CUR); > if (seekback == (off_t) -1) > return error("cannot find the current offset"); Omitting further context, even though it would help, but this reply is long enough already. It seems the compiler got confused -- I can't see an execution path that would use an uninitialized offset. If idx is NULL then the function is exited early, and if it's not then offset is initialized. But perhaps I'm missing something. Anyway, my points are that simply initializing might not always be the best fix, and that more context would help reviewers of such a patch, but only if functions are reasonably short and it's not necessary to follow the rabbit into a call chain hole. Didn't check the other cases. René
Stephan Beyer <s-beyer@gmx.net> writes: > diff --git a/fast-import.c b/fast-import.c > index b44d6a467e..58f73f9105 100644 > --- a/fast-import.c > +++ b/fast-import.c > @@ -903,7 +903,8 @@ static int store_object( > struct object_entry *e; > unsigned char hdr[96]; > struct object_id oid; > - unsigned long hdrlen, deltalen; > + unsigned long hdrlen; > + unsigned long deltalen = 0; > git_hash_ctx c; > git_zstream s; [in my attempt to imitate Réne...] In this function, deltalen is used only when delta != NULL, i.e. if (delta) { s.next_in = delta; s.avail_in = deltalen; } else { s.next_in = (void *)dat->buf; s.avail_in = dat->len; } ... if (delta) { ... hdrlen = encode_in_pack_object_header(hdr, sizeof(hdr), OBJ_OFS_DELTA, deltalen); ... Could delta become non-NULL without deltalen getting set? We see these before all uses of delta/deltalen in this function. if (last && last->data.len && last->data.buf && last->depth < max_depth && dat->len > the_hash_algo->rawsz) { delta_count_attempts_by_type[type]++; delta = diff_delta(last->data.buf, last->data.len, dat->buf, dat->len, &deltalen, dat->len - the_hash_algo->rawsz); } else delta = NULL; If diff_delta() returns non-NULL without touching deltalen, we'd be in trouble. We see this in delta.h static inline void * diff_delta(const void *src_buf, unsigned long src_bufsize, const void *trg_buf, unsigned long trg_bufsize, unsigned long *delta_size, unsigned long max_delta_size) { struct delta_index *index = create_delta_index(src_buf, src_bufsize); if (index) { void *delta = create_delta(index, trg_buf, trg_bufsize, delta_size, max_delta_size); free_delta_index(index); return delta; } return NULL; } so the question is if create_delta() can return non-NULL without touching delta_size. In diff-delta.c::create_delta(), *delta_size is assigned once at the very end, when the function returns a pointer to an allocated memory 'out'. All the "return" statement other than that last one literally returns "NULL". So it seems that this is a case the compiler getting confused.
On Thu, Sep 05, 2019 at 07:08:49PM +0200, René Scharfe wrote: > Anyway, my points are that simply initializing might not always be the > best fix, and that more context would help reviewers of such a patch, > but only if functions are reasonably short and it's not necessary to > follow the rabbit into a call chain hole. I started looking at these, too, and came to the same conclusion. Some of these are pointing to real bugs, and just silencing the warnings misses the opportunity to find them. I'll comment on the ones I did look at. > Further context: > > ident_line = find_commit_header(buffer, "author", &ident_len); > > if (split_ident_line(&id, ident_line, ident_len) < 0) > die(_("invalid ident line: %.*s"), (int)ident_len, ident_line); > > find_commit_header() can return NULL. split_ident_line() won't handle > that well. So I think what's missing here is a NULL check. If the > compiler is smart enough then that should silence the initialization > warning. Yeah, this one is a segfault waiting to happen, I think, if the author line is missing. > > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c > > index 76ce906946..d0c03b0e9b 100644 > > --- a/builtin/pack-objects.c > > +++ b/builtin/pack-objects.c > > @@ -1171,7 +1171,7 @@ static int add_object_entry(const struct object_id *oid, enum object_type type, > > { > > struct packed_git *found_pack = NULL; > > off_t found_offset = 0; > > - uint32_t index_pos; > > + uint32_t index_pos = 0; > [...] > So if the packing list is empty then it returns NULL without setting > index_pos. Hmm. It does set it in all other cases, no matter if oid is > found or not. Is it really a good idea to make that exception? I > suspect always setting index_pos here would silence the compiler as well > and fix the issue closer to its root. Yeah, this is a weird one. That index_pos is actually a position in the current hash table. And in the first object we see in pack-objects' input, we definitely _do_ end up with a nonsense index_pos, which then gets passed to packlist_alloc(). But since we also need to grow the hash table during that allocation, we don't use index_pos at all. So setting index_pos to something known like "0" is kind of weird, in that we don't have a hash position at all (the table doesn't exist!). But it's probably the least bad thing. If we do that, it should happen in packlist_find(). I have to agree this whole "passing around index_pos" thing looks complicated. It seems like we're just saving ourselves one hash lookup on insertion (and it's not even an expensive hash, since we're reusing the oid). I suspect the whole thing would also be a lot simpler using one of our existing hash implementations. > Didn't check the other cases. The only other one I looked at was: >> int cmd__read_cache(int argc, const char **argv) >> { >>- int i, cnt = 1, namelen; >>+ int i, cnt = 1, namelen = 0; I actually saw this one the other day, because it triggered for me when compiling with SANITIZE=address. AFAICT it's a false positive. "name" is always NULL unless skip_prefix() returns true, in which case we always set "namelen". And we only look at "namelen" if "name" is non-NULL. This one doesn't even require LTO, because skip_prefix() is an inline function. I'm not sure why the compiler gets confused here. I don't mind initializing namelen to 0 to silence it, though (we already set name to NULL, so this would just match). -Peff
Stephan Beyer <s-beyer@gmx.net> writes: > diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c > index 7e79b555de..ef0963e2f4 100644 > --- a/t/helper/test-read-cache.c > +++ b/t/helper/test-read-cache.c > @@ -4,7 +4,7 @@ > > int cmd__read_cache(int argc, const char **argv) > { > - int i, cnt = 1, namelen; > + int i, cnt = 1, namelen = 0; > const char *name = NULL; > > if (argc > 1 && skip_prefix(argv[1], "--print-and-refresh=", &name)) { namelen = strlen(name); The above is the only assignment to namelen in this function, and namelen is used like so: if (name) { ... pos = index_name_pos(&the_index, name, namelen); So somebody does not realize that skip_prefix() returns true only when it touches name. But skip_prefix() is inline and visible to the compiler, and it is quite clear that name is only touched when the function returns non-zero. static inline int skip_prefix(const char *str, const char *prefix, const char **out) { do { if (!*prefix) { *out = str; return 1; } } while (*str++ == *prefix++); return 0; } So it looks like it is another case of compiler getting confused.
On Thu, Sep 05, 2019 at 10:56:12AM -0700, Junio C Hamano wrote: > Stephan Beyer <s-beyer@gmx.net> writes: > > > diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c > > index 7e79b555de..ef0963e2f4 100644 > > --- a/t/helper/test-read-cache.c > > +++ b/t/helper/test-read-cache.c > > @@ -4,7 +4,7 @@ > > > > int cmd__read_cache(int argc, const char **argv) > > { > > - int i, cnt = 1, namelen; > > + int i, cnt = 1, namelen = 0; > > const char *name = NULL; > > > > if (argc > 1 && skip_prefix(argv[1], "--print-and-refresh=", &name)) { > namelen = strlen(name); > > The above is the only assignment to namelen in this function, and > namelen is used like so: > > if (name) { > ... > pos = index_name_pos(&the_index, name, namelen); > > So somebody does not realize that skip_prefix() returns true only > when it touches name. But skip_prefix() is inline and visible to > the compiler, and it is quite clear that name is only touched when > the function returns non-zero. I said earlier that I wouldn't mind seeing "namelen = 0" here. But I think there is a much more direct solution: keeping the assignment and point of use closer together. That makes it more clear both to the compiler and to a human when we expect the variable to be valid. In fact, since it's only used once, we can drop the variable altogther. :) diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c index 7e79b555de..244977a29b 100644 --- a/t/helper/test-read-cache.c +++ b/t/helper/test-read-cache.c @@ -4,11 +4,10 @@ int cmd__read_cache(int argc, const char **argv) { - int i, cnt = 1, namelen; + int i, cnt = 1; const char *name = NULL; if (argc > 1 && skip_prefix(argv[1], "--print-and-refresh=", &name)) { - namelen = strlen(name); argc--; argv++; } @@ -24,7 +23,7 @@ int cmd__read_cache(int argc, const char **argv) refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL); - pos = index_name_pos(&the_index, name, namelen); + pos = index_name_pos(&the_index, name, strlen(name)); if (pos < 0) die("%s not in index", name); printf("%s is%s up to date\n", name,
Jeff King <peff@peff.net> writes: > I said earlier that I wouldn't mind seeing "namelen = 0" here. But I > think there is a much more direct solution: keeping the assignment and > point of use closer together. That makes it more clear both to the > compiler and to a human when we expect the variable to be valid. In > fact, since it's only used once, we can drop the variable altogther. :) Yeah, that sounds like a nice solution. > diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c > index 7e79b555de..244977a29b 100644 > --- a/t/helper/test-read-cache.c > +++ b/t/helper/test-read-cache.c > @@ -4,11 +4,10 @@ > > int cmd__read_cache(int argc, const char **argv) > { > - int i, cnt = 1, namelen; > + int i, cnt = 1; > const char *name = NULL; > > if (argc > 1 && skip_prefix(argv[1], "--print-and-refresh=", &name)) { > - namelen = strlen(name); > argc--; > argv++; > } > @@ -24,7 +23,7 @@ int cmd__read_cache(int argc, const char **argv) > > refresh_index(&the_index, REFRESH_QUIET, > NULL, NULL, NULL); > - pos = index_name_pos(&the_index, name, namelen); > + pos = index_name_pos(&the_index, name, strlen(name)); > if (pos < 0) > die("%s not in index", name); > printf("%s is%s up to date\n", name,
Am 05.09.19 um 19:53 schrieb Jeff King: >>> int cmd__read_cache(int argc, const char **argv) >>> { >>> - int i, cnt = 1, namelen; >>> + int i, cnt = 1, namelen = 0; > > I actually saw this one the other day, because it triggered for me when > compiling with SANITIZE=address. AFAICT it's a false positive. "name" is > always NULL unless skip_prefix() returns true, in which case we always > set "namelen". And we only look at "namelen" if "name" is non-NULL. > > This one doesn't even require LTO, because skip_prefix() is an inline > function. I'm not sure why the compiler gets confused here. Yes, that's curious. > I don't mind > initializing namelen to 0 to silence it, though (we already set name to > NULL, so this would just match). Pushing the strlen() call into the loop and getting rid of namelen should work as well -- and I'd be surprised if this had a measurable performance impact. René
René Scharfe <l.s.r@web.de> writes: > Am 05.09.19 um 19:53 schrieb Jeff King: >>>> int cmd__read_cache(int argc, const char **argv) >>>> { >>>> - int i, cnt = 1, namelen; >>>> + int i, cnt = 1, namelen = 0; >> >> I actually saw this one the other day, because it triggered for me when >> compiling with SANITIZE=address. AFAICT it's a false positive. "name" is >> always NULL unless skip_prefix() returns true, in which case we always >> set "namelen". And we only look at "namelen" if "name" is non-NULL. >> >> This one doesn't even require LTO, because skip_prefix() is an inline >> function. I'm not sure why the compiler gets confused here. > > Yes, that's curious. > >> I don't mind >> initializing namelen to 0 to silence it, though (we already set name to >> NULL, so this would just match). > > Pushing the strlen() call into the loop and getting rid of namelen should > work as well -- and I'd be surprised if this had a measurable performance > impact. Yeah, we are making strlen() call on a constant "name" in a loop over argv[]. I do not think it matters in this case, either.
Am 05.09.19 um 21:25 schrieb Junio C Hamano: > René Scharfe <l.s.r@web.de> writes: > >> Am 05.09.19 um 19:53 schrieb Jeff King: >>>>> int cmd__read_cache(int argc, const char **argv) >>>>> { >>>>> - int i, cnt = 1, namelen; >>>>> + int i, cnt = 1, namelen = 0; >>> >>> I actually saw this one the other day, because it triggered for me when >>> compiling with SANITIZE=address. AFAICT it's a false positive. "name" is >>> always NULL unless skip_prefix() returns true, in which case we always >>> set "namelen". And we only look at "namelen" if "name" is non-NULL. >>> >>> This one doesn't even require LTO, because skip_prefix() is an inline >>> function. I'm not sure why the compiler gets confused here. >> >> Yes, that's curious. >> >>> I don't mind >>> initializing namelen to 0 to silence it, though (we already set name to >>> NULL, so this would just match). >> >> Pushing the strlen() call into the loop and getting rid of namelen should >> work as well -- and I'd be surprised if this had a measurable performance >> impact. > > Yeah, we are making strlen() call on a constant "name" in a loop > over argv[]. I do not think it matters in this case, either. The loop count is either 1 or argv[1] interpreted as a number, i.e. it could be very high. Its body consists of an index load and writing a number to a file, though -- a strlen() call on the name of that file should go unnoticed amid that activity. (I didn't measure it, though.) René
René Scharfe <l.s.r@web.de> writes: > The loop count is either 1 or argv[1] interpreted as a number, i.e. it could > be very high. Its body consists of an index load and writing a number to a > file, though -- a strlen() call on the name of that file should go unnoticed > amid that activity. (I didn't measure it, though.) Yup, I didn't either but we reasoned along the same line.
On Thu, Sep 05, 2019 at 10:24:59AM +0200, Stephan Beyer wrote: > Compiler heuristics for detection of potentially uninitialized variables > may change between compiler versions and enabling link-time optimization > may find new warnings. Indeed, compiling with gcc 9.2.1 and enabled > link-time optimization feature resulted in a few hits that are fixed by > this patch in the most naïve way. This allows to compile git using the > DEVELOPER=1 switch (which sets -Werror) and using the -flto flag. Lots of discussion in this thread. Let's try to turn it into some patches. :) After the patches below, I can compile cleanly with gcc 9.2.1 using -flto with both -O2 and -O3 (some of the cases only seemed to trigger for me with -O3). I've ordered them in decreasing value. The first one is a real bugfix, the second is a related cleanup. The next 3 are appeasing the compiler, but I think are a good idea (but note I went more for root causes than your originals). The last one is perhaps more controversial, but IMHO worth doing. [1/6]: git-am: handle missing "author" when parsing commit [2/6]: pack-objects: use object_id in packlist_alloc() [3/6]: bulk-checkin: zero-initialize hashfile_checkpoint [4/6]: diff-delta: set size out-parameter to 0 for NULL delta [5/6]: test-read-cache: drop namelen variable [6/6]: pack-objects: drop packlist index_pos optimization builtin/am.c | 4 +++- builtin/pack-objects.c | 33 ++++++++++++++------------------- bulk-checkin.c | 2 +- diff-delta.c | 2 ++ pack-bitmap-write.c | 2 +- pack-bitmap.c | 2 +- pack-objects.c | 20 ++++++++++---------- pack-objects.h | 6 ++---- t/helper/test-read-cache.c | 5 ++--- 9 files changed, 36 insertions(+), 40 deletions(-) -Peff
On 9/6/19 12:48 AM, Jeff King wrote: > On Thu, Sep 05, 2019 at 10:24:59AM +0200, Stephan Beyer wrote: > >> Compiler heuristics for detection of potentially uninitialized variables >> may change between compiler versions and enabling link-time optimization >> may find new warnings. Indeed, compiling with gcc 9.2.1 and enabled >> link-time optimization feature resulted in a few hits that are fixed by >> this patch in the most naïve way. This allows to compile git using the >> DEVELOPER=1 switch (which sets -Werror) and using the -flto flag. > > Lots of discussion in this thread. Let's try to turn it into some > patches. :) I thought the same and just sent my version of it. :D Stephan
On Fri, Sep 06, 2019 at 12:53:49AM +0200, Stephan Beyer wrote: > On 9/6/19 12:48 AM, Jeff King wrote: > > On Thu, Sep 05, 2019 at 10:24:59AM +0200, Stephan Beyer wrote: > > > >> Compiler heuristics for detection of potentially uninitialized variables > >> may change between compiler versions and enabling link-time optimization > >> may find new warnings. Indeed, compiling with gcc 9.2.1 and enabled > >> link-time optimization feature resulted in a few hits that are fixed by > >> this patch in the most naïve way. This allows to compile git using the > >> DEVELOPER=1 switch (which sets -Werror) and using the -flto flag. > > > > Lots of discussion in this thread. Let's try to turn it into some > > patches. :) > > I thought the same and just sent my version of it. :D Yeah, I see that our mails crossed. :) I like some of my versions better, but I'm OK with either (or a mix-and-match). -Peff
On 9/6/19 12:58 AM, Jeff King wrote: > On Fri, Sep 06, 2019 at 12:53:49AM +0200, Stephan Beyer wrote: > >> On 9/6/19 12:48 AM, Jeff King wrote: >>> On Thu, Sep 05, 2019 at 10:24:59AM +0200, Stephan Beyer wrote: >>> >>>> Compiler heuristics for detection of potentially uninitialized variables >>>> may change between compiler versions and enabling link-time optimization >>>> may find new warnings. Indeed, compiling with gcc 9.2.1 and enabled >>>> link-time optimization feature resulted in a few hits that are fixed by >>>> this patch in the most naïve way. This allows to compile git using the >>>> DEVELOPER=1 switch (which sets -Werror) and using the -flto flag. >>> >>> Lots of discussion in this thread. Let's try to turn it into some >>> patches. :) >> >> I thought the same and just sent my version of it. :D > > Yeah, I see that our mails crossed. :) I like some of my versions > better, but I'm OK with either (or a mix-and-match). I took a quick glance at yours. I also noticed the issue you address in [PATCH 2/6], but I was unsure if this is the way to go (I'm only occasionally reading on this list). I would prefer your patch series, with maybe one exception... The thing is: I had *exactly* the same commit like your [PATCH 6/6] (except for the commit message and for the number), but I dropped it. Why? Because I had the feeling (no particular instance though) that the second locate_object_entry_hash() for each insertion *can* indeed take "too much" time. Also, I was wondering, if the "found = 1" case should be catched as a BUG("should not happen") or something. I don't care much, though. The performance impact should probably be checked carefully. Stephan
On Fri, Sep 06, 2019 at 01:10:46AM +0200, Stephan Beyer wrote: > I took a quick glance at yours. I also noticed the issue you address in > [PATCH 2/6], but I was unsure if this is the way to go (I'm only > occasionally reading on this list). I would prefer your patch series, > with maybe one exception... Yeah, we've been slowly removing these old "unsigned char *" references (see a7db4c193d98f for the latest round touching this same code -- I'm actually surprised I missed this one back then, as that's when packlist_find() got converted). > The thing is: I had *exactly* the same commit like your [PATCH 6/6] > (except for the commit message and for the number), but I dropped it. > Why? Because I had the feeling (no particular instance though) that the > second locate_object_entry_hash() for each insertion *can* indeed take > "too much" time. Also, I was wondering, if the "found = 1" case should > be catched as a BUG("should not happen") or something. > > I don't care much, though. The performance impact should probably be > checked carefully. I did measure it, like this: # Do the traversal separately. This would make any difference in the # pack-objects hash code stand out _more_, plus it makes it cheaper to # run multiple trials. git rev-list --objects --all >input # Make sure stderr is redirected to avoid progress, which again would # amplify any differences. git pack-objects --stdout --delta-base-offset <input >/dev/null 2>&1 Running on linux.git, I got: [before] Benchmark #1: git pack-objects --stdout --delta-base-offset <input >/dev/null 2>&1 Time (mean ± σ): 26.225 s ± 0.233 s [User: 24.089 s, System: 4.867 s] Range (min … max): 25.915 s … 26.555 s 10 runs [after] Benchmark #1: git pack-objects --stdout --delta-base-offset <input >/dev/null 2>&1 Time (mean ± σ): 26.129 s ± 0.170 s [User: 24.003 s, System: 4.958 s] Range (min … max): 25.974 s … 26.570 s 10 runs So actually faster after, though not statistically significant. ;) The BUG() on "found==1" might be worth doing. -Peff
diff --git a/builtin/am.c b/builtin/am.c index 1aea657a7f..ab914fd46e 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1266,7 +1266,7 @@ static int get_mail_commit_oid(struct object_id *commit_id, const char *mail) static void get_commit_info(struct am_state *state, struct commit *commit) { const char *buffer, *ident_line, *msg; - size_t ident_len; + size_t ident_len = 0; struct ident_split id; buffer = logmsg_reencode(commit, NULL, get_commit_output_encoding()); diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 76ce906946..d0c03b0e9b 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -1171,7 +1171,7 @@ static int add_object_entry(const struct object_id *oid, enum object_type type, { struct packed_git *found_pack = NULL; off_t found_offset = 0; - uint32_t index_pos; + uint32_t index_pos = 0; display_progress(progress_state, ++nr_seen); diff --git a/bulk-checkin.c b/bulk-checkin.c index 39ee7d6107..87fa28c227 100644 --- a/bulk-checkin.c +++ b/bulk-checkin.c @@ -200,6 +200,8 @@ static int deflate_to_pack(struct bulk_checkin_state *state, struct hashfile_checkpoint checkpoint; struct pack_idx_entry *idx = NULL; + checkpoint.offset = 0; + seekback = lseek(fd, 0, SEEK_CUR); if (seekback == (off_t) -1) return error("cannot find the current offset"); diff --git a/fast-import.c b/fast-import.c index b44d6a467e..58f73f9105 100644 --- a/fast-import.c +++ b/fast-import.c @@ -903,7 +903,8 @@ static int store_object( struct object_entry *e; unsigned char hdr[96]; struct object_id oid; - unsigned long hdrlen, deltalen; + unsigned long hdrlen; + unsigned long deltalen = 0; git_hash_ctx c; git_zstream s; diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c index 7e79b555de..ef0963e2f4 100644 --- a/t/helper/test-read-cache.c +++ b/t/helper/test-read-cache.c @@ -4,7 +4,7 @@ int cmd__read_cache(int argc, const char **argv) { - int i, cnt = 1, namelen; + int i, cnt = 1, namelen = 0; const char *name = NULL; if (argc > 1 && skip_prefix(argv[1], "--print-and-refresh=", &name)) {
Compiler heuristics for detection of potentially uninitialized variables may change between compiler versions and enabling link-time optimization may find new warnings. Indeed, compiling with gcc 9.2.1 and enabled link-time optimization feature resulted in a few hits that are fixed by this patch in the most naïve way. This allows to compile git using the DEVELOPER=1 switch (which sets -Werror) and using the -flto flag. Signed-off-by: Stephan Beyer <s-beyer@gmx.net> --- builtin/am.c | 2 +- builtin/pack-objects.c | 2 +- bulk-checkin.c | 2 ++ fast-import.c | 3 ++- t/helper/test-read-cache.c | 2 +- 5 files changed, 7 insertions(+), 4 deletions(-) -- 2.23.0.38.g7ab3f3815a