Re: [PATCH v2 2/2] ci: compile "linux-gcc-default" job with -Og

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

>   pack-mtimes.c: In function ‘load_pack_mtimes_file’:
>   Error: pack-mtimes.c:89:25: ‘mtimes_size’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>      89 |                         munmap(data, mtimes_size);
>         |                         ^~~~~~~~~~~~~~~~~~~~~~~~~
>   cc1: all warnings being treated as errors
>   make: *** [Makefile:2757: pack-mtimes.o] Error 1
>   make: *** Waiting for unfinished jobs....

The use on line 89 is guarded with "if (data)" and data can become
non-NULL only after mtimes_size is computed, so this is benign.

They have excuse for a false positive because the warning is about
"maybe" uninitialized, but that does not help our annoyance factor
X-<.

>   pack-revindex.c: In function ‘load_revindex_from_disk’:
>   Error: pack-revindex.c:260:25: ‘revindex_size’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>     260 |                         munmap(data, revindex_size);
>         |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>   cc1: all warnings being treated as errors
>   make: *** [Makefile:2757: pack-revindex.o] Error 1
>   cat: exit.status: No such file or directory

This follows exactly the same pattern established by the other one
(or perhaps the other one copied from here).  It is another false
positive.

I am not sure what the right fix would be.  For example, if we were
interested in avoiding to incur too much resources for revindex, we
might do something like this

--- i/pack-revindex.c
+++ w/pack-revindex.c
@@ -258,6 +258,8 @@ static int load_revindex_from_disk(char *revindex_name,
 	if (ret) {
 		if (data)
 			munmap(data, revindex_size);
+		fprintf(stderr, "would have fit %d revindex in 10MB\n",
+			10 * 1024 * 1024 / revindex_size);
 	} else {
 		*len_p = revindex_size;
 		*data_p = (const uint32_t *)data;

without even guarding with "if (data)".

If we "initialize" revindex_size to a meaningless dummy value like 0
like the attached would _hide_ such a real bug from the compiler, so
I dunno.

@@ -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 = 0;
 	struct revindex_header *hdr;
 
 	if (git_env_bool(GIT_TEST_REV_INDEX_DIE_ON_DISK, 0))