Mac and compatibility fixes by RJVB · Pull Request #6 · cloudflare/zlib

RJVB · 2017-05-26T14:01:23Z

These are my current fixes for getting zlib/CloudFlare to build on Mac and work as a real drop-in replacement:

build with clang
fix the crc32_pclmul assembly so it builds on Mac (NB: it will crash in 32bit mode, PCLMUL support has to be disabled some way!)
revert a change to the uInt and uLong types which broke building Qt and were probably dangerous anyway (several functions receive pointers to these types, and dependent software is not guaranteed to use the typedefs from zlib).

The change to double-width variables led to build Qt failures and possibly out-of-bound writes with dependent software that passes pointers to the traditional 16 or 32 bit variables.

vkrasnov · 2017-12-03T00:23:37Z

Hi, can you try https://github.com/cloudflare/zlib/tree/vlad/aarch64 ?

RJVB · 2017-12-03T20:06:18Z

Hi, can you try https://github.com/cloudflare/zlib/tree/vlad/aarch64 ?

It seems to build not anymore in 32bit mode which makes "universal builds" (multiple architectures in a single binary) very tricky.

neurolabusc · 2018-05-21T19:03:57Z

It would be great to provide Mac compatibility. However, I think this patch assumes unsigned int is a 32-bit value. This might not be true for all 16 and 64 bit processors and compilers. Therefore, for backward compatibility I would have used the following code to ensure uint8_t, uint32_t, uint64_t are defined.

#ifndef uint32_t 
 #include <stdint.h>
#endif

kornelski · 2018-05-21T22:39:06Z

I don't see why this patch changes uint32_t. All macOS (not '90s Mac OS) compilers support <stdint.h> without any issues.

The ASM fix is needed.

neurolabusc · 2018-05-22T02:09:42Z

Precisely, if you look at the Files Changed you can see that deflate.c, zconf.h, zconf.h.cmakein, zconf.h.in modify the uint64_t, uint32_t, uint8_t references rather than simply including <stdint.h>. But with these modifications, it would be great to see this patch applied to provide MacOS users to have this accelerated library - the speedup is really impressive.

RJVB · 2018-05-22T09:38:28Z

I'll need to find some time to get back to this, but don't hesitate to do a PR of your own. The speed-up is impressive indeed, but you can get an almost identical speed-up from zlib-ng which is based on "zlib current", not on an older version... Last time I looked at sizeof(unsigned int) I concluded that it should be 32bit on all 32bit and wider architectures (in standard C at least). I admit I didn't consider 16bit architectures, but are those still relevant for this project?

#ifndef uint32_t

That's almost guaranteed not to work universally (= on systems that typedef uint32_t instead of #defining it).

neurolabusc · 2018-05-22T14:45:43Z

@RJVB - seems like a good point, this seems like a solution

#ifndef UINT32_MAX
#include <stdint.h>
#endif

Thanks for pointing out zlib-ng. Is that completely stable? I note a few outstanding issues and no stable release.

RJVB · 2018-05-22T15:09:32Z

Thanks for pointing out zlib-ng. Is that completely stable? I note a few outstanding issues and no stable release.

AFAIK they have been getting close to a release version, but I haven't looked at it closely for several months. Most development seems to be targetting platforms I have little to no interest in. At least there *is* active development going on. I'm tempted to say it's mostly stable, and that the devs will probably be interested in anything you do run into. It's a less obvious choice if you're looking for a drop-in, ABI-compatible replacement, I recall proposing a change to their compatibility mode a while back aimed at making it possible to replace an existing shared libz with NG libz.

kornelski · 2018-05-22T16:36:01Z

@RJVB Sparc64 has 64-bit int. The C standard allows arbitrarily large int.

<stdint.h> is guaranteed to be present in C99, and these days even MSVC supports that much.

neurolabusc · 2018-05-23T12:16:25Z

@RJVB : I have generated a new pull request that includes your fixes but uses <stdint.h> to define uint32_t etc if and only if needed.
I did a simple test on my late-2013 MacBook (Haswell, 2 core, 4 thread) with my sample neuroimaging datasets using the local SSD, normalized to default zlib, each using default compression ratio (with almost identical results, pigz creates files a tiny bit larger):
zlib x1.0
zlib-ng x1.45
zlib-cloudflare x2.0
pigz x2.33
In reality, pigz has a cost, as I must save the uncompressed data to disk prior to compressing with pigz, while my software (dcm2niix) can use the other tools to write the compressed data to disk (which is a big win for slow disk I/O, for example network drives). Since an individual run of fMRI data is often ~500mb uncompressed per file, this has real performance implications. I appreciate your comments that zlib-ng links to more recent zlib releases. But it is unclear if one must use compiler switches to achieve cloudflare performance or it simply is not as fast.

vkrasnov · 2018-05-23T13:59:42Z

test/example.c

    CHECK_ERR(err, "inflateEnd");

    if (d_stream.total_out != 2*uncomprLen + comprLen/2) {
-        fprintf(stderr, "bad large inflate: %lld\n", d_stream.total_out);


if you want to support gcc and clang have to use PRId64 from inttypes.h.

Would you be happy with:

#include <stdint.h> #include <inttypes.h> ... fprintf(stderr, "bad large inflate: %10" PRId64 "\n", d_stream.total_out);

I prefer simply % instead %10

vkrasnov · 2018-05-23T14:00:20Z

deflate.c

 * upper bound of about 14% expansion does not seem onerous for output buffer
 * allocation.
 */
-uint64_t ZEXPORT deflateBound(strm, sourceLen)


I would really rather keep the uintN_t instead.

This change is not used/required in the updated PR#11.

LloydW93 · 2022-09-22T09:52:10Z

As we took PR #8, and this is a stale/conflicting PR, I'm going to close it.

Update references

RJVB added 3 commits May 26, 2017 15:42

fix building with clang

4218442

make compile on/for Mac

ae3fdb8

Restore API (and probably ABI) compatibility.

7068a0c

The change to double-width variables led to build Qt failures and possibly out-of-bound writes with dependent software that passes pointers to the traditional 16 or 32 bit variables.

felixbuenemann mentioned this pull request Dec 3, 2017

Fix compile of crc32-pclmul_asm on macOS High Sierra #8

Merged

fix zconf.h

99d8243

neurolabusc mentioned this pull request May 23, 2018

MacOS support: conditional inclusion of stdint.h, updates https://git… #11

Closed

vkrasnov reviewed May 23, 2018

View reviewed changes

neurolabusc mentioned this pull request May 25, 2018

Mac and compatibility fixes #12

Closed

LloydW93 closed this Sep 22, 2022

fhanau pushed a commit to fhanau/zlib that referenced this pull request Feb 27, 2023

Merge pull request cloudflare#6 from aboba/patch-4

8cd07ee

Update references

Conversation

RJVB commented May 26, 2017

Uh oh!

vkrasnov commented Dec 3, 2017

Uh oh!

RJVB commented Dec 3, 2017 via email

Uh oh!

neurolabusc commented May 21, 2018

Uh oh!

kornelski commented May 21, 2018

Uh oh!

neurolabusc commented May 22, 2018

Uh oh!

RJVB commented May 22, 2018 via email

Uh oh!

neurolabusc commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RJVB commented May 22, 2018 via email

Uh oh!

kornelski commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neurolabusc commented May 23, 2018

Uh oh!

vkrasnov May 23, 2018

Choose a reason for hiding this comment

Uh oh!

neurolabusc May 23, 2018

Choose a reason for hiding this comment

Uh oh!

vkrasnov May 23, 2018

Choose a reason for hiding this comment

Uh oh!

vkrasnov May 23, 2018

Choose a reason for hiding this comment

Uh oh!

neurolabusc May 23, 2018

Choose a reason for hiding this comment

Uh oh!

LloydW93 commented Sep 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

neurolabusc commented May 22, 2018 •

edited

Loading

kornelski commented May 22, 2018 •

edited

Loading