Fix misdetection of {crc32,alder32}_z in cloudflare zlib fork #68

KJTsanaktsidis · 2023-10-15T22:49:34Z

We use the Cloudflare fork of zlib
(https://github.com/cloudflare/zlib), which we find gives improved performance on AWS Graviton ARM instances. That fork does not define crc32_z and alder32_z functions.

Until two days ago, Ruby's zlib gem worked fine, because cloudflare zlib also did not define z_size_t, which meant Ruby did not try and use these functions.

Since cloudflare/zlib@a3ba995 however, cloudflare zlib does define z_size_t (but NOT crc32_z or alder32_z). The zlib gem would try and use these nonexistant functions and not compile.

This patch fixes it by actually specifically detecting the functions that the gem wants to call, rather than just the presence of the z_size_t type.

We use the Cloudflare fork of zlib (https://github.com/cloudflare/zlib), which we find gives improved performance on AWS Graviton ARM instances. That fork does not define crc32_z and alder32_z functions. Until two days ago, Ruby's zlib gem worked fine, because cloudflare zlib _also_ did not define z_size_t, which meant Ruby did not try and use these functions. Since cloudflare/zlib@a3ba995 however, cloudflare zlib _does_ define z_size_t (but NOT crc32_z or alder32_z). The zlib gem would try and use these nonexistant functions and not compile. This patch fixes it by actually specifically detecting the functions that the gem wants to call, rather than just the presence of the z_size_t type.

mame · 2023-10-26T03:18:13Z

@KJTsanaktsidis @ioquatix This changeset caused build failure on cross-compiling to android30

https://rubyci.s3.amazonaws.com/crossruby/crossruby-master-aarch64-android30/log/20231026T022009Z.fail.html.gz

compiling zlib.c
zlib.c:48:53: warning: type specifier missing, defaults to 'int' [-Wimplicit-int]
typedef uLong (*checksum_func)(uLong, const Bytef*, z_size_t);
                                                    ^
zlib.c:436:70: warning: implicit conversion loses integer precision: 'long' to 'int' [-Wshorten-64-to-32]
            sum = checksum_long(func, sum, (Bytef*)RSTRING_PTR(buf), RSTRING_LEN(buf));
                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
../.././include/ruby/internal/core/rstring.h:46:27: note: expanded from macro 'RSTRING_LEN'
#define RSTRING_LEN       RSTRING_LEN
                          ^
zlib.c:406:72: note: expanded from macro 'checksum_long'
#define checksum_long(func, sum, ptr, len) (func)(mask32(sum), (ptr), (len))
                                           ~                           ^~~
zlib.c:441:59: warning: implicit conversion loses integer precision: 'long' to 'int' [-Wshorten-64-to-32]
        sum = checksum_long(func, sum, (Bytef*)RSTRING_PTR(str), RSTRING_LEN(str));
              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
../.././include/ruby/internal/core/rstring.h:46:27: note: expanded from macro 'RSTRING_LEN'
#define RSTRING_LEN       RSTRING_LEN
                          ^
zlib.c:406:72: note: expanded from macro 'checksum_long'
#define checksum_long(func, sum, ptr, len) (func)(mask32(sum), (ptr), (len))
                                           ~                           ^~~
zlib.c:469:36: error: use of undeclared identifier 'adler32_z'; did you mean 'adler32'?
    return do_checksum(argc, argv, adler32);
                                   ^~~~~~~
                                   adler32
zlib.c:50:18: note: expanded from macro 'adler32'
# define adler32 adler32_z
                 ^
/home/chkbuild/opt/android-ndk-r21e/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/zlib.h:1552:23: note: 'adler32' declared here
ZEXTERN uLong ZEXPORT adler32 OF((uLong adler, const Bytef *buf, uInt len));
                      ^
zlib.c:509:36: error: use of undeclared identifier 'crc32_z'; did you mean 'crc32'?
    return do_checksum(argc, argv, crc32);
                                   ^~~~~
                                   crc32
zlib.c:49:16: note: expanded from macro 'crc32'
# define crc32 crc32_z
               ^
/home/chkbuild/opt/android-ndk-r21e/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/zlib.h:1583:23: note: 'crc32' declared here
ZEXTERN uLong ZEXPORT crc32   OF((uLong crc, const Bytef *buf, uInt len));
                      ^
zlib.c:2093:50: error: use of undeclared identifier 'adler32_z'; did you mean 'adler32'?
    VALUE checksum = do_checksum(1, &dictionary, adler32);
                                                 ^~~~~~~
                                                 adler32
zlib.c:50:18: note: expanded from macro 'adler32'
# define adler32 adler32_z
                 ^
/home/chkbuild/opt/android-ndk-r21e/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/zlib.h:1552:23: note: 'adler32' declared here
ZEXTERN uLong ZEXPORT adler32 OF((uLong adler, const Bytef *buf, uInt len));
                      ^
zlib.c:2442:15: warning: implicit declaration of function 'crc32_z' is invalid in C99 [-Wimplicit-function-declaration]
    gz->crc = crc32(0, Z_NULL, 0);
              ^
zlib.c:49:16: note: expanded from macro 'crc32'
# define crc32 crc32_z
               ^
zlib.c:2473:15: warning: implicit declaration of function 'crc32_z' is invalid in C99 [-Wimplicit-function-declaration]
    gz->crc = crc32(0, Z_NULL, 0);
              ^
zlib.c:49:16: note: expanded from macro 'crc32'
# define crc32 crc32_z
               ^
zlib.c:2817:26: error: use of undeclared identifier 'crc32_z'; did you mean 'crc32'?
        gz->crc = checksum_long(crc32, gz->crc, str, len);
                                ^~~~~
                                crc32
zlib.c:49:16: note: expanded from macro 'crc32'
# define crc32 crc32_z
               ^
zlib.c:406:45: note: expanded from macro 'checksum_long'
#define checksum_long(func, sum, ptr, len) (func)(mask32(sum), (ptr), (len))
                                            ^
/home/chkbuild/opt/android-ndk-r21e/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/zlib.h:1583:23: note: 'crc32' declared here
ZEXTERN uLong ZEXPORT crc32   OF((uLong crc, const Bytef *buf, uInt len));
                      ^
zlib.c:2817:47: warning: implicit conversion loses integer precision: 'long' to 'uInt' (aka 'unsigned int') [-Wshorten-64-to-32]
        gz->crc = checksum_long(crc32, gz->crc, str, len);
                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
zlib.c:406:72: note: expanded from macro 'checksum_long'
#define checksum_long(func, sum, ptr, len) (func)(mask32(sum), (ptr), (len))
                                           ~                           ^~~
zlib.c:2854:26: error: use of undeclared identifier 'crc32_z'; did you mean 'crc32'?
        gz->crc = checksum_long(crc32, gz->crc, (Bytef*)RSTRING_PTR(str) + gz->ungetc,
                                ^~~~~
                                crc32
zlib.c:49:16: note: expanded from macro 'crc32'
# define crc32 crc32_z
               ^
zlib.c:406:45: note: expanded from macro 'checksum_long'
#define checksum_long(func, sum, ptr, len) (func)(mask32(sum), (ptr), (len))
                                            ^
/home/chkbuild/opt/android-ndk-r21e/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/zlib.h:1583:23: note: 'crc32' declared here
ZEXTERN uLong ZEXPORT crc32   OF((uLong crc, const Bytef *buf, uInt len));
                      ^
zlib.c:2855:22: warning: implicit conversion loses integer precision: 'long' to 'uInt' (aka 'unsigned int') [-Wshorten-64-to-32]
                                RSTRING_LEN(str) - gz->ungetc);
                                ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
zlib.c:406:72: note: expanded from macro 'checksum_long'
#define checksum_long(func, sum, ptr, len) (func)(mask32(sum), (ptr), (len))
                                           ~                           ^~~
zlib.c:4553:26: error: use of undeclared identifier 'crc32_z'; did you mean 'crc32'?
        gz->crc = checksum_long(crc32, gz->crc, ptr, len);
                                ^~~~~
                                crc32
zlib.c:49:16: note: expanded from macro 'crc32'
# define crc32 crc32_z
               ^
zlib.c:406:45: note: expanded from macro 'checksum_long'
#define checksum_long(func, sum, ptr, len) (func)(mask32(sum), (ptr), (len))
                                            ^
/home/chkbuild/opt/android-ndk-r21e/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/zlib.h:1583:23: note: 'crc32' declared here
ZEXTERN uLong ZEXPORT crc32   OF((uLong crc, const Bytef *buf, uInt len));
                      ^
zlib.c:4553:47: warning: implicit conversion loses integer precision: 'long' to 'uInt' (aka 'unsigned int') [-Wshorten-64-to-32]
        gz->crc = checksum_long(crc32, gz->crc, ptr, len);
                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
zlib.c:406:72: note: expanded from macro 'checksum_long'
#define checksum_long(func, sum, ptr, len) (func)(mask32(sum), (ptr), (len))
                                           ~                           ^~~
8 warnings and 6 errors generated.
gmake[2]: *** [Makefile:254: zlib.o] Error 1

KJTsanaktsidis · 2023-10-26T04:52:54Z

Eurgh - thanks for letting me know. Looking at it now...

KJTsanaktsidis · 2023-10-26T05:40:14Z

OK, so this is pretty amusing.

When extconf runs (with the toolchain from android-ndk-r21e), it actually detects that the crc32_z and adler32_z functions are available. This is the mkmf.log output (I also added have_type("z_size_t", "zlib.h") back for comparison:

have_type: checking for z_size_t in zlib.h... -------------------- no

LD_LIBRARY_PATH=.:../.. "x86_64-linux-android30-clang -I../../.ext/include/x86_64-linux-android-android -I../.././include -I../.././ext/zlib    -fdeclspec -O3 -fno-fast-math -ggdb3 -Wall -Wextra -Wextra-tokens -Wdeprecated-declarations -Wdivision-by-zero -Wdiv-by-zero -Wimplicit-function-declaration -Wimplicit-int -Wpointer-arith -Wshorten-64-to-32 -Wwrite-strings -Wold-style-definition -Wmissing-noreturn -Wno-constant-logical-operand -Wno-long-long -Wno-missing-field-initializers -Wno-overlength-strings -Wno-parentheses-equality -Wno-self-assign -Wno-tautological-compare -Wno-unused-parameter -Wno-unused-value -Wunused-variable -Wundef   -c conftest.c"
conftest.c:6:9: error: unknown type name 'z_size_t'
typedef z_size_t conftest_type;
        ^
1 error generated.
checked program was:
/* begin */
1: #include "ruby.h"
2:
3: #include <zlib.h>
4:
5: /*top*/
6: typedef z_size_t conftest_type;
7: int conftestval[sizeof(conftest_type)?1:-1];
/* end */

--------------------

have_func: checking for crc32_z() in zlib.h... -------------------- yes

LD_LIBRARY_PATH=.:../.. "x86_64-linux-android30-clang -o conftest -I../../.ext/include/x86_64-linux-android-android -I../.././include -I../.././ext/zlib    -fdeclspec -O3 -fno-fast-math -ggdb3 -Wall -Wextra -Wextra-tokens -Wdeprecated-declarations -Wdivision-by-zero -Wdiv-by-zero -Wimplicit-function-declaration -Wimplicit-int -Wpointer-arith -Wshorten-64-to-32 -Wwrite-strings -Wold-style-definition -Wmissing-noreturn -Wno-constant-logical-operand -Wno-long-long -Wno-missing-field-initializers -Wno-overlength-strings -Wno-parentheses-equality -Wno-self-assign -Wno-tautological-compare -Wno-unused-parameter -Wno-unused-value -Wunused-variable -Wundef conftest.c  -L. -L../.. -L. -fstack-protector-strong -rdynamic -Wl,-export-dynamic    -lz  -Wl,-rpath,/home/kj/ruby -lruby-static -lz -ldl -lm  -lz  -lm  -lc"
conftest.c:16:57: error: use of undeclared identifier 'crc32_z'
int t(void) { void ((*volatile p)()); p = (void ((*)()))crc32_z; return !p; }
                                                        ^
1 error generated.
checked program was:
/* begin */
 1: #include "ruby.h"
 2:
 3: #include <zlib.h>
 4:
 5: /*top*/
 6: extern int t(void);
 7: int main(int argc, char **argv)
 8: {
 9:   if (argc > 1000000) {
10:     int (* volatile tp)(void)=(int (*)(void))&t;
11:     printf("%d", (*tp)());
12:   }
13:
14:   return !!argv[argc];
15: }
16: int t(void) { void ((*volatile p)()); p = (void ((*)()))crc32_z; return !p; }
/* end */

LD_LIBRARY_PATH=.:../.. "x86_64-linux-android30-clang -o conftest -I../../.ext/include/x86_64-linux-android-android -I../.././include -I../.././ext/zlib    -fdeclspec -O3 -fno-fast-math -ggdb3 -Wall -Wextra -Wextra-tokens -Wdeprecated-declarations -Wdivision-by-zero -Wdiv-by-zero -Wimplicit-function-declaration -Wimplicit-int -Wpointer-arith -Wshorten-64-to-32 -Wwrite-strings -Wold-style-definition -Wmissing-noreturn -Wno-constant-logical-operand -Wno-long-long -Wno-missing-field-initializers -Wno-overlength-strings -Wno-parentheses-equality -Wno-self-assign -Wno-tautological-compare -Wno-unused-parameter -Wno-unused-value -Wunused-variable -Wundef conftest.c  -L. -L../.. -L. -fstack-protector-strong -rdynamic -Wl,-export-dynamic    -lz  -Wl,-rpath,/home/kj/ruby -lruby-static -lz -ldl -lm  -lz  -lm  -lc"
checked program was:
/* begin */
 1: #include "ruby.h"
 2:
 3: #include <zlib.h>
 4:
 5: /*top*/
 6: extern int t(void);
 7: int main(int argc, char **argv)
 8: {
 9:   if (argc > 1000000) {
10:     int (* volatile tp)(void)=(int (*)(void))&t;
11:     printf("%d", (*tp)());
12:   }
13:
14:   return !!argv[argc];
15: }
16: extern void crc32_z();
17: int t(void) { crc32_z(); return 0; }
/* end */

--------------------

We can see:

the zlib.h header doesn't contain z_size_t, OK. That's why this was building fine before.
mkmf decides that it does have crc32_z though!
It tries two ways to detect it: first it tries compiling a program that includes the header file and takes the address of the function. That fails.
Then, it tries to compile a program that declares the function as extern void crc32_z() and links it. That actually succeeds!
This behaviour is in mkmf here: https://github.com/ruby/ruby/blob/634e0ac140d890904c59eab8bdec09b80c78b1a4/lib/mkmf.rb#L802C11-L802C11

The problem is that in android-ndk-r21e:

zlib.h really doesn't have z_size_t or crc32_z:

kj@kj-thinkpad ruby % grep -c crc32_z ~/android-ndk-r21e/sysroot/usr/include/zlib.h
0

But the provided zlib.a archive does have the symbol in it!

kj@kj-thinkpad ruby % nm  ~/android-ndk-r21e/sysroot/usr/lib/x86_64-linux-android/libz.a | grep crc32_z
0000000000000000 T crc32_z

So the link test succeeds, and thus have_func("crc32_z", "zlib.h") is considered to be true, and then it blows up at compile time when the extension tries to actually use the definitions in the header file.

There's two things which could be considered to be "the problem" here:

the Android NDK shouldn't have a mismatch between what's in the zlib static library and what's in the header file
have_func shouldn't be checking if the program is linkable if the symbol is declared extern; it should fail if the symbol isn't in the header file (especially if a header file is actually passed to it)

Obviously we can't fix the NDK... so I think our choices are:

Change how have_func in mkmf behaves
Make it work how it was working before, by also checking for have_type("z_size_t", "zlib.h") like how it was before my change.

Obviously point 1 requires a ticket, some analysis as to what will break in the ecosystem, and probably presentation at a dev meeting, so I'll open a PR to hack around it with option 2.

KJTsanaktsidis force-pushed the ktsanaktsidis/fix_cloudflare_zlib branch from 967d89d to c96e8b9 Compare October 15, 2023 22:56

ioquatix requested a review from nobu October 26, 2023 02:13

ioquatix approved these changes Oct 26, 2023

View reviewed changes

ioquatix merged commit 48e3685 into ruby:master Oct 26, 2023
20 checks passed

KJTsanaktsidis mentioned this pull request Oct 26, 2023

Check for z_size_t along with {crc,adler}32_z in extconf.rb #69

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix misdetection of {crc32,alder32}_z in cloudflare zlib fork #68

Fix misdetection of {crc32,alder32}_z in cloudflare zlib fork #68

KJTsanaktsidis commented Oct 15, 2023

mame commented Oct 26, 2023

KJTsanaktsidis commented Oct 26, 2023

KJTsanaktsidis commented Oct 26, 2023 •

edited

Loading

Fix misdetection of {crc32,alder32}_z in cloudflare zlib fork #68

Fix misdetection of {crc32,alder32}_z in cloudflare zlib fork #68

Conversation

KJTsanaktsidis commented Oct 15, 2023

mame commented Oct 26, 2023

KJTsanaktsidis commented Oct 26, 2023

KJTsanaktsidis commented Oct 26, 2023 • edited Loading

KJTsanaktsidis commented Oct 26, 2023 •

edited

Loading