Re: [PATCH 1/2] config.mak.dev: workaround gcc 12 bug affecting "pedantic" CI job

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Apr 15 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Fri, Apr 15 2022, Carlo Arenas wrote:
>>
>>>> > +# https://bugzilla.redhat.com/show_bug.cgi?id=2075786
>>>> > +ifneq ($(filter gcc12,$(COMPILER_FEATURES)),)
>>>> > +DEVELOPER_CFLAGS += -Wno-error=stringop-overread
>>>> > +endif
>>>>
>>>> What I meant with "just set -Wno-error=stringop-overread on gcc12 for
>>>> dir.(o|s|sp)?" was that you can set this per-file:
>>>
>>> of course, but that change goes in the Makefile and therefore affects
>>> ...
>> I mean it can go in config.mak.dev, it doesn't need to be in the
>> Makefile itself.
>> ...
>>>>         dir.sp dir.s dir.o: EXTRA_CPPFLAGS += -Wno-error=stringop-overread
>>>
>>> I know at least one developer that will then rightfully complain that
>>> the git build doesn't work in AIX with xl after this.
>>
>> Yes, it would break if it were in the Makfile, but not if it's in
>> config.mak.dev.
>
> I do not think you can blame Carlo for poor reading/comprehension in
> this case---I too (mis)read what you wrote, and didn't realize that
> you were suggesting to add the "for these target, EXTRA_CPPFLAGS
> additionally gets this value" inside the ifneq/endif Carlo added to
> hold the DEVELOPER_CFLAGS thing.

Indeed, I don't think I would have understood myself, I didn't mean to
imply any fault (except my own for not elaborating). Just claifying that
we can use that trick.

I.e. my own config.mak has had this (or a form thereof) for a while:

	http.sp http.s http.o: EXTRA_CPPFLAGS += -Wno-error=dangling-pointer=
	dir.sp dir.s dir.o: EXTRA_CPPFLAGS += -Wno-error=stringop-overread 

> For now, let's stick to the simpler form, though.

Sure, works for me.

Note though that one important difference between this solution and the
patch I had for http.c is that the patch will fix things for all builds,
whereas a config.mak.dev change (whether it's Carlos's global addition,
or my per-file) can only do so for cases where DEVELOPER=1.

Although in practice that's probably fine, anyone turning on -Werror is
likely to do so through DEVELOPER=1, and fore those that don't it's
"just a warning".

So yeah, it's probably better to do that for now.

But FWIW I did write up and test the below monstrosity as a replacement
just now, it's guaranteed to work with/without DEVELOPER, and squashes
only that specific warning, and only on GCC:
	
	diff --git a/dir.c b/dir.c
	index f2b0f242101..e7a5acb126f 100644
	--- a/dir.c
	+++ b/dir.c
	@@ -3089,6 +3089,13 @@ char *git_url_basename(const char *repo, int is_bundle, int is_bare)
	 	 * result in a dir '2222' being guessed due to backwards
	 	 * compatibility.
	 	 */
	+#ifdef __clang__
	+#elif defined(__GNUC__)
	+#if __GNUC__ >= 12
	+#pragma GCC diagnostic push
	+#pragma GCC diagnostic ignored "-Wstringop-overread"
	+#endif
	+#endif
	 	if (memchr(start, '/', end - start) == NULL
	 	    && memchr(start, ':', end - start) != NULL) {
	 		ptr = end;
	@@ -3097,6 +3104,12 @@ char *git_url_basename(const char *repo, int is_bundle, int is_bare)
	 		if (start < ptr && ptr[-1] == ':')
	 			end = ptr - 1;
	 	}
	+#ifdef __clang__
	+#elif defined(__GNUC__)
	+#if __GNUC__ >= 12
	+#pragma GCC diagnostic pop
	+#endif
	+#endif
	 
	 	/*
	 	 * Find last component. To remain backwards compatible we
	diff --git a/http.c b/http.c
	index 229da4d1488..e63d4ab9527 100644
	--- a/http.c
	+++ b/http.c
	@@ -1329,7 +1329,21 @@ void run_active_slot(struct active_request_slot *slot)
	 	struct timeval select_timeout;
	 	int finished = 0;
	 
	+#ifdef __clang__
	+#elif defined(__GNUC__)
	+#if __GNUC__ >= 12
	+#pragma GCC diagnostic push
	+#pragma GCC diagnostic ignored "-Wdangling-pointer="
	+#endif
	+#endif
	 	slot->finished = &finished;
	+#ifdef __clang__
	+#elif defined(__GNUC__)
	+#if __GNUC__ >= 12
	+#pragma GCC diagnostic pop
	+#endif
	+#endif
	+
	 	while (!finished) {
	 		step_active_slots();
	 
But yeah, between us not having -Werror by default and DEVELOPER
handling it it's probably not worth it just to be able to selectively
suppress the warnings involved.