Walkthrough

Generate version-script maps from templates into the build directory, reference generated maps in linker flags, ignore generated maps in Git, add map cleanup to distclean, and route internal gz_error calls through the PREFIX macro via header and call-site updates.

Changes

Possibly related PRs

• Simplify the gzread.c name mangling workaround #2093 — related changes to build-time name-mangling and symbol-prefix workflow.
• Rename CMake targets to avoid clashes when used as a subproject #1970 — CMake-level version-script handling and linker flag adjustments.
• Reorganize initialization and buffer allocation for gz* code #1928 — touches gz* internals and references the same gz_error internal function.

Suggested labels

Build Env, Compatibility

Suggested reviewers

• Dead2
• nmoinvaz

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Codecov Report

❌ Patch coverage is 22.22222% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.56%. Comparing base (83a14aa) to head (82979cb).
⚠️ Report is 8 commits behind head on develop.

@@             Coverage Diff             @@
##           develop    #2097      +/-   ##
===========================================
- Coverage    83.93%   83.56%   -0.38%     
===========================================
  Files          156      156              
  Lines        13184    13184              
  Branches      3211     3214       +3     
===========================================
- Hits         11066    11017      -49     
- Misses        1061     1130      +69     
+ Partials      1057     1037      -20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

CodeCov failures are partially because runner didn't have AVX512 support and partially because we don't test most error conditions in gzip support code.

If we want to prefix gz_error, the new one ahould go into a new group 'ZLIB_NG_2.3.3'.
To maintain strict ABI compatibility, the old un-prefixed gz_error should still remain in ZLIB_NB_2.0.0 though.
But it is a local function and I don't think it could ever be referenced from outside, so I'm not sure why it is in the map at all..

We don't have many Z_INTERNAL symbols in the map at all. Take for example zng_tr_init(), it is not in the map and it is not prefixed.
The other local symbols in that list are things I can imagine at least being made available from the outside (alloc functions through pointers in strm for example, not that that is more useful with the map entry).

According to what I can find online, linkers use the local list to exclude those symbols from being exported, and usually don't include information about them in the binary, unless extra symbol debugging info is specifically enabled. But normal debuginfo also includes this and more, possibly this would be useful for closed-source debugging where you don't want to export the full debuginfo.
So, since we already set the symbols as Z_INTERNAL in the code, the local entries are not useful for anything as far as I can tell. They are annoying and a maintenance burden though, since they introduce prefixing to internal functions.

My suggestion: Remove gz_error from at least the zlib-ng.map file, possibly from zlib.map as well. Nothing should have linked against it, so it should also not be possible to break anything.
This also lets us avoid both prefixing and symbol_prefixing.

AFAICT: None of the internal symbols should need symbol prefixing, and likely not prefixing either.
The only case where I can conceive that either prefix method would be needed for local symbols would be if the map files are specifically used to check for conflicts, and that would be fixed by not having them listed there in the first place.

Am I wrong?

Since I was worried I was missing something, I did confer with Google's AI and ChatGPT in case they had some further insight, but they both concluded pretty much the same, with Google being the more concise:

Thinking. Searching.
Including internal symbols such as gz_error in the local: section of a linker map file for zlib-ng is redundant and adds to maintenance costs.
AI responses may include mistakes. Learn more

That has to be the shortest most concise AI reply I have ever gotten to something like this. 👀

So, perhaps drop the gz_error prefixing changes and instead add a commit removing gz_error from the .map and mangling files?

@Dead2 We still need the mangling for static builds. I didn't check when the catch all local was added but the default is global for any unlisted symbols as is still case for zlib-compat

Because static builds don't have symbol versioning, we can't introduce zng_gz_error as a new local symbol, as it would require adding the unprefixed symbol as alias, and that would "again" break compiling against both stock zlib and zlib-ng. It's a compromise, but actually found by AI bot as seen in the resolved reviews.

@Dead2 We still need the mangling for static builds. I didn't check when the catch all local was added but the default is global for any unlisted symbols as is still case for zlib-compat

Since we use Z_INTERNAL for all internal functions, that already makes the symbols unreachable and invisible from the outside. This is something zlib does not do though, but we have done so since before our first release and we have had no complaints about that.

This is also why the catch all local is named FAIL, so that we can in effect grep for it (I guess we should have added a CI test for that actually).

$ nm -D build/libz-ng.so|grep FAIL
0000000000000000 A FAIL

It contains no symbols because they are all marked as internal in the source code.
The internal symbols can not conflict with anything either. The mapped ones can potentially.

Because static builds don't have symbol versioning, we can't introduce zng_gz_error as a new local symbol, as it would require adding the unprefixed symbol as alias, and that would "again" break compiling against both stock zlib and zlib-ng. It's a compromise, but actually found by AI bot as seen in the resolved reviews.

To avoid introducing new things to the ABI, I am suggesting removing gz_error entirely from map.
That way it won't need any prefixing or mangling either, because the map already is the only reason why it gets any external visibility, and that is something it does not need to have.

Z_INTERNAL has no effect in static builds as linker handles it for shared builds. Static builds also don't use the map file so catch all local doesn't work. Only on Windows exported symbols need to be explicitly marked as such.

Z_INTERNAL has no effect in static builds as linker handles it for shared builds. Static builds also don't use the map file so catch all local doesn't work. Only on Windows exported symbols need to be explicitly marked as such.

In that case static builds won't work anyway, because we have lots of Z_INTERNAL functions that do not get mangled or prefixed.
So that is not a blocker for removing gz_error from the map.

Z_INTERNAL has no effect in static builds as linker handles it for shared builds. Static builds also don't use the map file so catch all local doesn't work. Only on Windows exported symbols need to be explicitly marked as such.

In that case static builds won't work anyway, because we have lots of Z_INTERNAL functions that do not get mangled. So that is not a blocker for removing gz_error from the map.

The point (of this PR) is to gradually make both shared and static builds work with all options enabled. Removing gz_error or zng_gz_error from the zlib-ng.map file doesn't mean it doesn't need to be mangled for static builds. More symbols we have explicitly marked as local in map files, closer we are getting to actually working build with both symbol prefixing, and linking against both stock zlib and zlib-ng in static build.

The point (of this PR) is to gradually make both shared and static builds work with all options enabled. Removing gz_error or zng_gz_error from the zlib-ng.map file doesn't mean it doesn't need to be mangled for static builds. More symbols we have explicitly marked as local in map files, closer we are getting to actually working build with both symbol prefixing, and linking against both stock zlib and zlib-ng in static build.

I think that is something that should be done in a separate PR, and to avoid ABI churn, either don't PREFIX gz_error in this PR, or remove it from map entirely.

In any case, whether the names need mangling or not for static builds, Z_INTERNAL symbols should not be in the map file. The symbols that need to be marked as local in .map files should be tagged as hidden, not internal. Marking things internal removes them from the symbol table entirely, making the library smaller and enables more optimizations. Adding them to the map file however conflicts with that.

I think that is something that should be done in a separate PR, and to avoid ABI churn, either don't PREFIX gz_error in this PR, or remove it from map entirely.

I'm not against removing it from the zlib-ng.map file as removing local symbols doesn't break ABI and we don't advertise backwards compatibility in the map file, so we don't need to care about any symbol renames.

The entry was needed for validating this PR, but now I know what had to be done, so I think it's safe to remove.