Re: [PATCH 1/2] config.mak.dev: workaround gcc 12 bug affecting "pedantic" CI job
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Apr 16, 2022, 12:33 UTC
- Message-ID
- <220416.86czhh9ov0.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <xmqqo8117er2.fsf@gitster.g>
On Fri, Apr 15 2022, Junio C Hamano wrote:
Show 30 quoted lines
> Æ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.