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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 12, 2024, 22:11 UTC
Message-ID: <xmqqo785olpp.fsf@gitster.g>
In-Reply-To: <xmqqed946auc.fsf@gitster.g>

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

Show 28 quoted lines

> We can squelch the warning by initializing the variable to a
> meaningless value, like 0, but I consider that such a change makes
> the resulting code a lot worse than the original, and that is not
> because I do not want an extra and meaningless assignment emitted
> [*2*] in the resulting binary whose performance impact cannot be
> measured by anybody.
>
> It is bad because it defeats efforts by compilers that understand
> the data flow a bit better to diagnose a true "used uninitialized"
> bugs we may introduce in the future.  Adding such a workaround goes
> against the longer-term health of the codebase.
>
> For example, I could add another use of mtimes_size that is not
> guarded by "if (data)" right next to it, i.e.
>
>         @@ -87,6 +87,7 @@ static int load_pack_mtimes_file(char *mtimes_file,
>                 if (ret) {
>                         if (data)
>                                 munmap(data, mtimes_size);
>         +		printf("debug %d\n", (int)mtimes_size);
>                 } else {
>                         *len_p = mtimes_size;
>                         *data_p = data;
>
> gcc-13 does notice that we are now truly using the variable
> uninitialized and flags it for us (gcc-12 also does notice but the
> important difference is that gcc-13 refrained from warning at the
> safe use of the variable that is guarded by "if (data)").

By the way, I do not know if any compiler gives us such a feature, but if the trick to squelch a false positive were finer grained, I would have been much more receptive to the idea of building with different optimization level, allowing a bit more false positives.

The workaround everybody jumps at is to initialize the variable to a meaningless value (like 0) and I have explained why it is suboptimal already. But if we can tell less intelligent compilers "we know our use of this variable AT THIS POINT is safe", e.g. by annotating the above snippet of the code, perhaps like this:

                if (ret) {
                        if (data)
				/* -Wno-uninitialized (mtimes_size) */
                                munmap(data, mtimes_size);
			printf("debug %d\n", (int)mtimes_size);

then it would be clear to the compiler that understand the annotation that inside that "if (data)" block, we know that the use of mtimes_size is not using an uninitialized variable.

For the use of the same variable on the next "debug" line, because it is outside of that "if (data)" block, the annotation should have no effect, and the compiler is free to do its own analysis and we will accept if it finds mtimes_size can be used uninitialized there. Any new use added for the same variable will not be masked by a meaningless initialization if we can use such a "workaround" to squelch false positives.