| Message ID | 20220415231342.35980-2-carenas@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 846a29afb0b1a206426a3fa0867c37dc406415bc |
| Headers | show |
| Series | ci: avoid failures for pedantic job with fedora 36 | expand |
On Fri, Apr 15 2022, Carlo Marcelo Arenas Belón wrote: > Originally noticed by Peff[1], but yet to be corrected[2] and planned to > be released with Fedora 36 (scheduled for Apr 19). > > dir.c: In function ‘git_url_basename’: > dir.c:3085:13: error: ‘memchr’ specified bound [9223372036854775808, 0] exceeds maximum object size 9223372036854775807 [-Werror=stringop-overread] > 3085 | if (memchr(start, '/', end - start) == NULL > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Fedora is used as part of the CI, and therefore that release will trigger > failures, unless the version of the image used is locked to an older > release, as an alternative. > > Restricting the flag to the affected source file, as well as implementing > an independent facility to track these workarounds was specifically punted > to minimize the risk of introducing problems so close to a release. > > This change should be reverted once the underlying gcc bug is solved and > which should be visible by NOT triggering a warning, otherwise. > > [1] https://lore.kernel.org/git/YZQhLh2BU5Hquhpo@coredump.intra.peff.net/ > [2] https://bugzilla.redhat.com/show_bug.cgi?id=2075786 > > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > config.mak.dev | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/config.mak.dev b/config.mak.dev > index 3deb076d5e3..335efd46203 100644 > --- a/config.mak.dev > +++ b/config.mak.dev > @@ -65,4 +65,9 @@ DEVELOPER_CFLAGS += -Wno-uninitialized > endif > endif > > +# 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: dir.sp dir.s dir.o: EXTRA_CPPFLAGS += -Wno-error=stringop-overread Ditto for the warning suppression in 2/2, we don't currently have any other warnings like this, but we can suppress them more narrowly.
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > Originally noticed by Peff[1], but yet to be corrected[2] and planned to > be released with Fedora 36 (scheduled for Apr 19). > > dir.c: In function ‘git_url_basename’: > dir.c:3085:13: error: ‘memchr’ specified bound [9223372036854775808, 0] exceeds maximum object size 9223372036854775807 [-Werror=stringop-overread] > 3085 | if (memchr(start, '/', end - start) == NULL > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ I've seen this one; is this accepted on their side that the compiler is hurting us with a false positive here? > +# https://bugzilla.redhat.com/show_bug.cgi?id=2075786 > +ifneq ($(filter gcc12,$(COMPILER_FEATURES)),) > +DEVELOPER_CFLAGS += -Wno-error=stringop-overread > +endif If this does not break the build further, and it makes the -Werror build succeed, I wouldn't be too much worried. I think this one and the other one are innocuous enough that they can be fast-tracked. Thanks.
On Fri, Apr 15, 2022 at 4:45 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > On Fri, Apr 15 2022, Carlo Marcelo Arenas Belón wrote: > > diff --git a/config.mak.dev b/config.mak.dev > > index 3deb076d5e3..335efd46203 100644 > > --- a/config.mak.dev > > +++ b/config.mak.dev > > @@ -65,4 +65,9 @@ DEVELOPER_CFLAGS += -Wno-uninitialized > > endif > > endif > > > > +# 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 ALL builds, this one only affects DEVELOPER=1 and is therefore more narrow. that is what I meant with "has been punted" in my commit message. > 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. Carlo
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > 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: > > dir.sp dir.s dir.o: EXTRA_CPPFLAGS += -Wno-error=stringop-overread > > Ditto for the warning suppression in 2/2, we don't currently have any > other warnings like this, but we can suppress them more narrowly. While it is certainly attractive if we can loosen the warning settings in a more pointed way, is it easy to arrange something like the above ONLY for gcc12 and no other compilers? Do we know -Wno-error=i-have-no-idea-what-that-error-is safely gets ignored by all compilers we care about, which may not even be a GCC?
On Fri, Apr 15 2022, Carlo Arenas wrote: > On Fri, Apr 15, 2022 at 4:45 PM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: >> On Fri, Apr 15 2022, Carlo Marcelo Arenas Belón wrote: >> > diff --git a/config.mak.dev b/config.mak.dev >> > index 3deb076d5e3..335efd46203 100644 >> > --- a/config.mak.dev >> > +++ b/config.mak.dev >> > @@ -65,4 +65,9 @@ DEVELOPER_CFLAGS += -Wno-uninitialized >> > endif >> > endif >> > >> > +# 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 > ALL builds, this one only affects DEVELOPER=1 and is therefore more > narrow. > > that is what I meant with "has been punted" in my commit message. I mean it can go in config.mak.dev, it doesn't need to be in the Makefile itself. The make doesn't have any notion of "file scope" or similar, the behavior is just a union of the variables, rules etc. that you source. So just as we append to DEVELOPER_CFLAGS and the Makefile uses it we can say "only append this to this file's flags", which since it's in config.mak.dev is guarded by DEVELOPER. >> 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. There it'll be guarded by the "only for gcc12" clause, so we don't need to worry about breaking any other compiler.
Æ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. For now, let's stick to the simpler form, though. Thanks.
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.
diff --git a/config.mak.dev b/config.mak.dev index 3deb076d5e3..335efd46203 100644 --- a/config.mak.dev +++ b/config.mak.dev @@ -65,4 +65,9 @@ DEVELOPER_CFLAGS += -Wno-uninitialized endif endif +# https://bugzilla.redhat.com/show_bug.cgi?id=2075786 +ifneq ($(filter gcc12,$(COMPILER_FEATURES)),) +DEVELOPER_CFLAGS += -Wno-error=stringop-overread +endif + GIT_TEST_PERL_FATAL_WARNINGS = YesPlease
Originally noticed by Peff[1], but yet to be corrected[2] and planned to be released with Fedora 36 (scheduled for Apr 19). dir.c: In function ‘git_url_basename’: dir.c:3085:13: error: ‘memchr’ specified bound [9223372036854775808, 0] exceeds maximum object size 9223372036854775807 [-Werror=stringop-overread] 3085 | if (memchr(start, '/', end - start) == NULL | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Fedora is used as part of the CI, and therefore that release will trigger failures, unless the version of the image used is locked to an older release, as an alternative. Restricting the flag to the affected source file, as well as implementing an independent facility to track these workarounds was specifically punted to minimize the risk of introducing problems so close to a release. This change should be reverted once the underlying gcc bug is solved and which should be visible by NOT triggering a warning, otherwise. [1] https://lore.kernel.org/git/YZQhLh2BU5Hquhpo@coredump.intra.peff.net/ [2] https://bugzilla.redhat.com/show_bug.cgi?id=2075786 Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- config.mak.dev | 5 +++++ 1 file changed, 5 insertions(+)