| Message ID | xmqqjziw3arr.fsf@gitster.g (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] DONTAPPLY: -Og fallout workaround | expand |
Junio C Hamano <gitster@pobox.com> writes: > These "workarounds" are to mark variables that are used after > initialized, but some compilers with lower optimization levels > cannot see and report "used uninitialized". > > This set targets "gcc-12 -Og". For the reason why this is a wrong > thing to do for longer-term code health, see > > https://lore.kernel.org/git/xmqqed946auc.fsf@gitster.g/ > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > > * Even though I said I won't do the actual patch, since I had to > gauge the extent of damage, I ended up doing so anyways. > > As I explained already, the size of this patch, i.e. number of > places that need the workaround, does not really matter. What > is horrible is how each of these workaround will hide real bugs > we may introduce in the future from the compilers. > > builtin/branch.c | 2 +- > builtin/fast-import.c | 4 ++-- > builtin/repack.c | 2 +- > fetch-pack.c | 2 +- > http-backend.c | 2 +- > http.c | 2 +- > pack-mtimes.c | 2 +- > pack-revindex.c | 2 +- > refs/packed-backend.c | 2 +- > reftable/stack.c | 2 +- > remote-curl.c | 4 ++-- > t/helper/test-ref-store.c | 2 +- > trailer.c | 4 ++-- > 13 files changed, 16 insertions(+), 16 deletions(-) And depending on the version of compilers, apparently even this is not enough. I do not offhand know what GitHub CI is running for linux-gcc-default (ubuntu-latest), but this gets flagged for using (try to guess which one without looking at the answer below) ... static int parse_count(const char *arg) { int count; if (strtol_i(arg, 10, &count) < 0) die("'%s': not an integer", arg); return count; } ... count uninitilaized, since the compiler does not realize that strtol_i() always touches "count" unless the function returns negative, and die() never returns. Exactly the same pattern continues. So, unless we disable -Werror, let's not continue this experiment with -Og or -Os as the damage seems to be far greater than the benefit (which I haven't seen any, but that is largely due to timezone differences---I asked "what's the real bug you found with this" a few hours ago that is past EOB in Europe).
On Mon, Jun 10, 2024 at 01:05:00PM -0700, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > These "workarounds" are to mark variables that are used after > > initialized, but some compilers with lower optimization levels > > cannot see and report "used uninitialized". > > > > This set targets "gcc-12 -Og". For the reason why this is a wrong > > thing to do for longer-term code health, see > > > > https://lore.kernel.org/git/xmqqed946auc.fsf@gitster.g/ > > > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > > --- > > > > * Even though I said I won't do the actual patch, since I had to > > gauge the extent of damage, I ended up doing so anyways. > > > > As I explained already, the size of this patch, i.e. number of > > places that need the workaround, does not really matter. What > > is horrible is how each of these workaround will hide real bugs > > we may introduce in the future from the compilers. > > > > builtin/branch.c | 2 +- > > builtin/fast-import.c | 4 ++-- > > builtin/repack.c | 2 +- > > fetch-pack.c | 2 +- > > http-backend.c | 2 +- > > http.c | 2 +- > > pack-mtimes.c | 2 +- > > pack-revindex.c | 2 +- > > refs/packed-backend.c | 2 +- > > reftable/stack.c | 2 +- > > remote-curl.c | 4 ++-- > > t/helper/test-ref-store.c | 2 +- > > trailer.c | 4 ++-- > > 13 files changed, 16 insertions(+), 16 deletions(-) > > And depending on the version of compilers, apparently even this is > not enough. I do not offhand know what GitHub CI is running for > linux-gcc-default (ubuntu-latest), but this gets flagged for using > (try to guess which one without looking at the answer below) ... > > static int parse_count(const char *arg) > { > int count; > > if (strtol_i(arg, 10, &count) < 0) > die("'%s': not an integer", arg); > return count; > } > > ... count uninitilaized, since the compiler does not realize that > strtol_i() always touches "count" unless the function returns > negative, and die() never returns. Exactly the same pattern > continues. > > So, unless we disable -Werror, let's not continue this experiment > with -Og or -Os as the damage seems to be far greater than the > benefit (which I haven't seen any, but that is largely due to > timezone differences---I asked "what's the real bug you found with > this" a few hours ago that is past EOB in Europe). The real bug that "-Og" would have been able to detect was reported by Peff via [1]. In this case it wasn't "-Og" that detected it, but Coverity did. But it would have been detected if we had a job that compiled with "-Og". But now that I see the full picture of this with different compiler options I have to agree that this is not really worth it. Especially not given that Coverity is able to detect such cases, even though that only happens retroactively after a topic has landed. Let's drop this experiment. Patrick [1]: 20240605100728.GA3440281@coredump.intra.peff.net
Patrick Steinhardt <ps@pks.im> writes: > The real bug that "-Og" would have been able to detect was reported by > Peff via [1]. In this case it wasn't "-Og" that detected it, but > Coverity did. But it would have been detected if we had a job that > compiled with "-Og". Thanks. It is curious that you singled out "-Og" here, but with the usual "-O2" optimization level, it does not seem to get noticed. > But now that I see the full picture of this with different compiler > options I have to agree that this is not really worth it. Especially not > given that Coverity is able to detect such cases, even though that only > happens retroactively after a topic has landed. > > Let's drop this experiment. OK, but let's keep the CFLAGS_APPEND one ;-)
On Tue, Jun 11, 2024 at 10:30:24AM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > But now that I see the full picture of this with different compiler > > options I have to agree that this is not really worth it. Especially not > > given that Coverity is able to detect such cases, even though that only > > happens retroactively after a topic has landed. > > > > Let's drop this experiment. > > OK, but let's keep the CFLAGS_APPEND one ;-) Sure, let's do that. I assume you'll just cherry-pick that single commit? Patrick
On Wed, Jun 12, 2024 at 06:42:28AM +0200, Patrick Steinhardt wrote: > On Tue, Jun 11, 2024 at 10:30:24AM -0700, Junio C Hamano wrote: > > Patrick Steinhardt <ps@pks.im> writes: > > > But now that I see the full picture of this with different compiler > > > options I have to agree that this is not really worth it. Especially not > > > given that Coverity is able to detect such cases, even though that only > > > happens retroactively after a topic has landed. > > > > > > Let's drop this experiment. > > > > OK, but let's keep the CFLAGS_APPEND one ;-) > > Sure, let's do that. I assume you'll just cherry-pick that single > commit? Ah, just spotted that you already did. Thanks! Patrick
diff --git a/builtin/branch.c b/builtin/branch.c index 48cac74f97..1f4de92e05 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -504,7 +504,7 @@ static void print_current_branch_name(void) int flags; const char *refname = refs_resolve_ref_unsafe(get_main_ref_store(the_repository), "HEAD", 0, NULL, &flags); - const char *shortname; + const char *shortname = shortname; if (!refname) die(_("could not resolve HEAD")); else if (!(flags & REF_ISSYMREF)) diff --git a/builtin/fast-import.c b/builtin/fast-import.c index ea5ae5196d..6d53e42011 100644 --- a/builtin/fast-import.c +++ b/builtin/fast-import.c @@ -1866,7 +1866,7 @@ static void parse_original_identifier(void) static int parse_data(struct strbuf *sb, uintmax_t limit, uintmax_t *len_res) { - const char *data; + const char *data = data; strbuf_reset(sb); if (!skip_prefix(command_buf.buf, "data ", &data)) @@ -2807,7 +2807,7 @@ static void parse_new_commit(const char *arg) static void parse_new_tag(const char *arg) { static struct strbuf msg = STRBUF_INIT; - const char *from; + const char *from = from; char *tagger; struct branch *s; struct tag *t; diff --git a/builtin/repack.c b/builtin/repack.c index 7608430a37..e4752aae3f 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -1106,7 +1106,7 @@ static int write_cruft_pack(const struct pack_objects_args *args, static const char *find_pack_prefix(const char *packdir, const char *packtmp) { - const char *pack_prefix; + const char *pack_prefix = pack_prefix; if (!skip_prefix(packtmp, packdir, &pack_prefix)) die(_("pack prefix %s does not begin with objdir %s"), packtmp, packdir); diff --git a/fetch-pack.c b/fetch-pack.c index eba9e420ea..f8f318742f 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1517,7 +1517,7 @@ static void receive_shallow_info(struct fetch_pack_args *args, process_section_header(reader, "shallow-info", 0); while (packet_reader_read(reader) == PACKET_READ_NORMAL) { - const char *arg; + const char *arg = arg; struct object_id oid; if (skip_prefix(reader->line, "shallow ", &arg)) { diff --git a/http-backend.c b/http-backend.c index 5b4dca65ed..eecba3b44f 100644 --- a/http-backend.c +++ b/http-backend.c @@ -259,7 +259,7 @@ static void http_config(void) static struct rpc_service *select_service(struct strbuf *hdr, const char *name) { - const char *svc_name; + const char *svc_name = svc_name; struct rpc_service *svc = NULL; int i; diff --git a/http.c b/http.c index 2dea2d03da..c45f91fa4e 100644 --- a/http.c +++ b/http.c @@ -2156,7 +2156,7 @@ static int update_url_from_redirect(struct strbuf *base, const char *asked, const struct strbuf *got) { - const char *tail; + const char *tail = tail; size_t new_len; if (!strcmp(asked, got->buf)) diff --git a/pack-mtimes.c b/pack-mtimes.c index cdf30b8d2b..49598d4749 100644 --- a/pack-mtimes.c +++ b/pack-mtimes.c @@ -29,7 +29,7 @@ static int load_pack_mtimes_file(char *mtimes_file, int fd, ret = 0; struct stat st; uint32_t *data = NULL; - size_t mtimes_size, expected_size; + size_t mtimes_size = mtimes_size, expected_size; struct mtimes_header header; fd = git_open(mtimes_file); diff --git a/pack-revindex.c b/pack-revindex.c index fc63aa76a2..7120bed5b1 100644 --- a/pack-revindex.c +++ b/pack-revindex.c @@ -206,7 +206,7 @@ static int load_revindex_from_disk(char *revindex_name, int fd, ret = 0; struct stat st; void *data = NULL; - size_t revindex_size; + size_t revindex_size = revindex_size; struct revindex_header *hdr; if (git_env_bool(GIT_TEST_REV_INDEX_DIE_ON_DISK, 0)) diff --git a/refs/packed-backend.c b/refs/packed-backend.c index c4c1e36aa2..25321a5758 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -677,7 +677,7 @@ static struct snapshot *create_snapshot(struct packed_ref_store *refs) /* If the file has a header line, process it: */ if (snapshot->buf < snapshot->eof && *snapshot->buf == '#') { - char *tmp, *p, *eol; + char *tmp, *p = p, *eol; struct string_list traits = STRING_LIST_INIT_NODUP; eol = memchr(snapshot->buf, '\n', diff --git a/reftable/stack.c b/reftable/stack.c index 737591125e..aa1851fd73 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -1231,7 +1231,7 @@ struct segment suggest_compaction_segment(uint64_t *sizes, size_t n, uint8_t factor) { struct segment seg = { 0 }; - uint64_t bytes; + uint64_t bytes = bytes; size_t i; if (!factor) diff --git a/remote-curl.c b/remote-curl.c index 827bd848ae..4f9808c63f 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -1265,7 +1265,7 @@ static void parse_fetch(struct strbuf *buf) int alloc_heads = 0, nr_heads = 0; do { - const char *p; + const char *p = p; if (skip_prefix(buf->buf, "fetch ", &p)) { const char *name; struct ref *ref; @@ -1428,7 +1428,7 @@ static void parse_push(struct strbuf *buf) int ret; do { - const char *arg; + const char *arg = arg; if (skip_prefix(buf->buf, "push ", &arg)) strvec_push(&specs, arg); else diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c index ad24300170..7d3870fe76 100644 --- a/t/helper/test-ref-store.c +++ b/t/helper/test-ref-store.c @@ -66,7 +66,7 @@ static unsigned int arg_flags(const char *arg, const char *name, static const char **get_store(const char **argv, struct ref_store **refs) { - const char *gitdir; + const char *gitdir = gitdir; if (!argv[0]) { die("ref store required"); diff --git a/trailer.c b/trailer.c index 72e5136c73..5056c16644 100644 --- a/trailer.c +++ b/trailer.c @@ -508,11 +508,11 @@ static int git_trailer_config(const char *conf_key, const char *value, const struct config_context *ctx UNUSED, void *cb UNUSED) { - const char *trailer_item, *variable_name; + const char *trailer_item = trailer_item, *variable_name; struct arg_item *item; struct conf_info *conf; char *name = NULL; - enum trailer_info_type type; + enum trailer_info_type type = type; int i; if (!skip_prefix(conf_key, "trailer.", &trailer_item))
These "workarounds" are to mark variables that are used after initialized, but some compilers with lower optimization levels cannot see and report "used uninitialized". This set targets "gcc-12 -Og". For the reason why this is a wrong thing to do for longer-term code health, see https://lore.kernel.org/git/xmqqed946auc.fsf@gitster.g/ Signed-off-by: Junio C Hamano <gitster@pobox.com> --- * Even though I said I won't do the actual patch, since I had to gauge the extent of damage, I ended up doing so anyways. As I explained already, the size of this patch, i.e. number of places that need the workaround, does not really matter. What is horrible is how each of these workaround will hide real bugs we may introduce in the future from the compilers. builtin/branch.c | 2 +- builtin/fast-import.c | 4 ++-- builtin/repack.c | 2 +- fetch-pack.c | 2 +- http-backend.c | 2 +- http.c | 2 +- pack-mtimes.c | 2 +- pack-revindex.c | 2 +- refs/packed-backend.c | 2 +- reftable/stack.c | 2 +- remote-curl.c | 4 ++-- t/helper/test-ref-store.c | 2 +- trailer.c | 4 ++-- 13 files changed, 16 insertions(+), 16 deletions(-)