| Message ID | patch-v3-1.1-e9cb8763fd4-20220120T011414Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] cache.h: auto-detect if zlib has uncompress2() | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > As noted in the updated commit message this approach of having an > object just for this fallback function comes at the cost of some > complexity, but now the compat object lives neatly in its own object. I do not see any change in this patch adding costly complexity, but I notice lack of one possible trick that might become problem with some compilers and linkers when their zlib has uncompress2() function. Let's have this graduate very early in the next cycle, to see if anybody on a rarer system sees a complaint due to having to deal with a totally empty object file. Will queue.
On January 21, 2022 6:23 PM, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > > > As noted in the updated commit message this approach of having an > > object just for this fallback function comes at the cost of some > > complexity, but now the compat object lives neatly in its own object. > > I do not see any change in this patch adding costly complexity, but I notice lack of > one possible trick that might become problem with some compilers and linkers > when their zlib has uncompress2() function. Let's have this graduate very early in > the next cycle, to see if anybody on a rarer system sees a complaint due to having > to deal with a totally empty object file. > > Will queue. On behalf of the "rarer systems", I will certainly be putting this through the regression suite. With thanks, --Randall
<rsbecker@nexbridge.com> writes: > On January 21, 2022 6:23 PM, Junio C Hamano wrote: >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >> > As noted in the updated commit message this approach of having an >> > object just for this fallback function comes at the cost of some >> > complexity, but now the compat object lives neatly in its own object. >> >> I do not see any change in this patch adding costly complexity, but I notice lack of >> one possible trick that might become problem with some compilers and linkers >> when their zlib has uncompress2() function. Let's have this graduate very early in >> the next cycle, to see if anybody on a rarer system sees a complaint due to having >> to deal with a totally empty object file. >> >> Will queue. > > On behalf of the "rarer systems", I will certainly be putting this through the regression suite. > With thanks, > --Randall ;-) I know you are on a rarer system, but I also know you need a real replacement code in the compat object file, so your linker will not be dealing with an empty object file. Thanks.
On 22.01.22 00:23, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> As noted in the updated commit message this approach of having an >> object just for this fallback function comes at the cost of some >> complexity, but now the compat object lives neatly in its own object. > > I do not see any change in this patch adding costly complexity, but > I notice lack of one possible trick that might become problem with > some compilers and linkers when their zlib has uncompress2() > function. Let's have this graduate very early in the next cycle, to > see if anybody on a rarer system sees a complaint due to having to > deal with a totally empty object file. OpenSSL has a macro in include/openssl/macros.h to counteract exactly this: /* * Sometimes OPENSSL_NO_xxx ends up with an empty file and some compilers * don't like that. This will hopefully silence them. */ #define NON_EMPTY_TRANSLATION_UNIT static void *dummy = &dummy; They insert it in the otherwise empty "#else" branch of conditionally complied code. Cheers, Beat
Beat Bolli <dev+git@drbeat.li> writes: > On 22.01.22 00:23, Junio C Hamano wrote: >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >>> As noted in the updated commit message this approach of having an >>> object just for this fallback function comes at the cost of some >>> complexity, but now the compat object lives neatly in its own object. >> I do not see any change in this patch adding costly complexity, but >> I notice lack of one possible trick that might become problem with >> some compilers and linkers when their zlib has uncompress2() >> function. Let's have this graduate very early in the next cycle, to >> see if anybody on a rarer system sees a complaint due to having to >> deal with a totally empty object file. > > OpenSSL has a macro in include/openssl/macros.h to counteract exactly this: > > /* > * Sometimes OPENSSL_NO_xxx ends up with an empty file and some > compilers > * don't like that. This will hopefully silence them. > */ > #define NON_EMPTY_TRANSLATION_UNIT static void *dummy = &dummy; > > They insert it in the otherwise empty "#else" branch of conditionally > complied code. Between my "I recall some compilers and linkers had trouble with an totally empty object files, and we'd better be prepared for them" and "I didn't see any system that had such a problem during my tests", I still lean toward "it merely shows that the problem is rarer than the set of systems you tried", even without further input. But "It is a problem for which a real workaround is used in a system that is used more widely than we are" is a clear enough evidence that we should have one, especially the solution is so trivial. https://github.com/openssl/openssl/pull/10425 seems to indicate that they are moving in a direction that removes the necessity to use this macro, by switching to tell the build system to not compile and build a file that would become empty due to conditionals, but nobody in the discussion seems to question the need for the macro for portability if an otherwise empty file need to be dealt with. Thanks.
diff --git a/Makefile b/Makefile index 5580859afdb..416498bbe69 100644 --- a/Makefile +++ b/Makefile @@ -256,8 +256,6 @@ all:: # # Define NO_DEFLATE_BOUND if your zlib does not have deflateBound. # -# Define NO_UNCOMPRESS2 if your zlib does not have uncompress2. -# # Define NO_NORETURN if using buggy versions of gcc 4.6+ and profile feedback, # as the compiler can crash (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49299) # @@ -588,6 +586,7 @@ TEST_BUILTINS_OBJS = TEST_OBJS = TEST_PROGRAMS_NEED_X = THIRD_PARTY_SOURCES = +ZLIB_COMPAT_OBJS = # Having this variable in your environment would break pipelines because # you cause "cd" to echo its destination to stdout. It can also take @@ -1726,10 +1725,8 @@ ifdef NO_DEFLATE_BOUND BASIC_CFLAGS += -DNO_DEFLATE_BOUND endif -ifdef NO_UNCOMPRESS2 - BASIC_CFLAGS += -DNO_UNCOMPRESS2 - REFTABLE_OBJS += compat/zlib-uncompress2.o -endif +# Detected using the ZLIB_VERNUM macro +ZLIB_COMPAT_OBJS += compat/zlib-uncompress2.o ifdef NO_POSIX_GOODIES BASIC_CFLAGS += -DNO_POSIX_GOODIES @@ -2076,6 +2073,7 @@ LIBS = $(filter-out %.o, $(GITLIBS)) $(EXTLIBS) BASIC_CFLAGS += $(COMPAT_CFLAGS) LIB_OBJS += $(COMPAT_OBJS) +LIB_OBJS += $(ZLIB_COMPAT_OBJS) # Quote for C @@ -2641,7 +2639,7 @@ $(LIB_FILE): $(LIB_OBJS) $(XDIFF_LIB): $(XDIFF_OBJS) $(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^ -$(REFTABLE_LIB): $(REFTABLE_OBJS) +$(REFTABLE_LIB): $(REFTABLE_OBJS) $(ZLIB_COMPAT_OBJS) $(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^ $(REFTABLE_TEST_LIB): $(REFTABLE_TEST_OBJS) diff --git a/cache.h b/cache.h index 281f00ab1b1..3a0142aa56f 100644 --- a/cache.h +++ b/cache.h @@ -18,7 +18,6 @@ #include "repository.h" #include "mem-pool.h" -#include <zlib.h> typedef struct git_zstream { z_stream z; unsigned long avail_in; diff --git a/ci/lib.sh b/ci/lib.sh index 9d28ab50fb4..cbc2f8f1caa 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -197,7 +197,6 @@ esac case "$jobname" in linux32) CC=gcc - MAKEFLAGS="$MAKEFLAGS NO_UNCOMPRESS2=1" ;; linux-musl) CC=gcc diff --git a/compat/zlib-uncompress2.c b/compat/zlib-uncompress2.c index 722610b9718..6836a1d37d8 100644 --- a/compat/zlib-uncompress2.c +++ b/compat/zlib-uncompress2.c @@ -1,3 +1,6 @@ +#include "git-compat-util.h" + +#if ZLIB_VERNUM < 0x1290 /* taken from zlib's uncompr.c commit cacf7f1d4e3d44d871b605da3b647f07d718623f @@ -8,7 +11,6 @@ */ -#include "../reftable/system.h" #define z_const /* @@ -16,8 +18,6 @@ * For conditions of distribution and use, see copyright notice in zlib.h */ -#include <zlib.h> - /* clang-format off */ /* =========================================================================== @@ -93,3 +93,4 @@ int ZEXPORT uncompress2 ( err == Z_BUF_ERROR && left + stream.avail_out ? Z_DATA_ERROR : err; } +#endif diff --git a/config.mak.uname b/config.mak.uname index c48db45106c..92ea00c219d 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -66,7 +66,6 @@ ifeq ($(uname_S),Linux) # centos7/rhel7 provides gcc 4.8.5 and zlib 1.2.7. ifneq ($(findstring .el7.,$(uname_R)),) BASIC_CFLAGS += -std=c99 - NO_UNCOMPRESS2 = YesPlease endif endif ifeq ($(uname_S),GNU/kFreeBSD) @@ -266,10 +265,6 @@ ifeq ($(uname_S),FreeBSD) FILENO_IS_A_MACRO = UnfortunatelyYes endif ifeq ($(uname_S),OpenBSD) - # Versions < 7.0 need compatibility layer - ifeq ($(shell expr "$(uname_R)" : "[1-6]\."),2) - NO_UNCOMPRESS2 = UnfortunatelyYes - endif NO_STRCASESTR = YesPlease NO_MEMMEM = YesPlease USE_ST_TIMESPEC = YesPlease @@ -525,7 +520,6 @@ ifeq ($(uname_S),Interix) endif endif ifeq ($(uname_S),Minix) - NO_UNCOMPRESS2 = YesPlease NO_IPV6 = YesPlease NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease NO_NSEC = YesPlease @@ -581,7 +575,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL) NO_SETENV = YesPlease NO_UNSETENV = YesPlease NO_MKDTEMP = YesPlease - NO_UNCOMPRESS2 = YesPlease # Currently libiconv-1.9.1. OLD_ICONV = UnfortunatelyYes NO_REGEX = NeedsStartEnd diff --git a/configure.ac b/configure.ac index d60d494ee4c..5ee25ec95c8 100644 --- a/configure.ac +++ b/configure.ac @@ -664,22 +664,9 @@ AC_LINK_IFELSE([ZLIBTEST_SRC], NO_DEFLATE_BOUND=yes]) LIBS="$old_LIBS" -AC_DEFUN([ZLIBTEST_UNCOMPRESS2_SRC], [ -AC_LANG_PROGRAM([#include <zlib.h>], - [uncompress2(NULL,NULL,NULL,NULL);])]) -AC_MSG_CHECKING([for uncompress2 in -lz]) -old_LIBS="$LIBS" -LIBS="$LIBS -lz" -AC_LINK_IFELSE([ZLIBTEST_UNCOMPRESS2_SRC], - [AC_MSG_RESULT([yes])], - [AC_MSG_RESULT([no]) - NO_UNCOMPRESS2=yes]) -LIBS="$old_LIBS" - GIT_UNSTASH_FLAGS($ZLIB_PATH) GIT_CONF_SUBST([NO_DEFLATE_BOUND]) -GIT_CONF_SUBST([NO_UNCOMPRESS2]) # # Define NEEDS_SOCKET if linking with libc is not enough (SunOS, diff --git a/git-compat-util.h b/git-compat-util.h index 1229c8296b9..96ee35f9e06 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -1386,6 +1386,17 @@ void unleak_memory(const void *ptr, size_t len); #define UNLEAK(var) do {} while (0) #endif +#include <zlib.h> + +#if ZLIB_VERNUM < 0x1290 +/* + * This is uncompress2, which is only available in zlib >= 1.2.9 + * (released as of early 2017). See compat/zlib-uncompress2.c. + */ +int uncompress2(Bytef *dest, uLongf *destLen, const Bytef *source, + uLong *sourceLen); +#endif + /* * This include must come after system headers, since it introduces macros that * replace system names. diff --git a/reftable/system.h b/reftable/system.h index 4907306c0c5..18f9207dfee 100644 --- a/reftable/system.h +++ b/reftable/system.h @@ -16,17 +16,6 @@ license that can be found in the LICENSE file or at #include "hash.h" /* hash ID, sizes.*/ #include "dir.h" /* remove_dir_recursively, for tests.*/ -#include <zlib.h> - -#ifdef NO_UNCOMPRESS2 -/* - * This is uncompress2, which is only available in zlib >= 1.2.9 - * (released as of early 2017) - */ -int uncompress2(Bytef *dest, uLongf *destLen, const Bytef *source, - uLong *sourceLen); -#endif - int hash_size(uint32_t id); #endif