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@xxxxxxx>
> ---
> 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é
