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

From: Jeff King <peff@peff.net>
Date: Jun 13, 2024, 10:15 UTC
Message-ID: <20240613101522.GC817573@coredump.intra.peff.net>
In-Reply-To: <xmqqo785olpp.fsf@gitster.g>

On Wed, Jun 12, 2024 at 03:11:30PM -0700, Junio C Hamano wrote:

Show 28 quoted lines

> 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.

I agree that such an annotation is much more focused. It's still not foolproof, though (e.g., we might chance earlier code so that the data/mtimes_size correlation is no longer true).

I think you could do it with:

			if (data)
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wuninitialized"
				munmap(data, mtimes_size);
#pragma GCC diagnostic pop

which is...ugly. There's a _Pragma() operator, too, which I think would let you make a macro like:

			if (data)
				SUPPRESS("-Wuninitialized", munmap(data, mtimes_size));

which is maybe slightly less horrific? Still pretty magical though.

But if the alternative is to do none of that, and just continue to avoid looking for warnings with -Os, I prefer that.

-Peff