Fix building with z_ prefix

[DenizThatMenace]

This PR fixes a problem with the version-script (zlib.map) when also building with enabled z_ prefix for the function names.

Without the changes from this PR the version-script did not properly assign version numbers to the function symbols in zlib.so (using GCC or Clang) if the function symbols are prefixed with z_!

---

Background

Most linkers silently ignored this error and just generated a zlib.so without any version numbers attached. Starting with lld 17 (or was it 16?), however, linking even fails with the following errors:

ld.lld-17: error: version script assignment of 'ZLIB_1.2.0' to symbol 'compressBound' failed: symbol not defined
ld.lld-17: error: version script assignment of 'ZLIB_1.2.0' to symbol 'deflateBound' failed: symbol not defined
ld.lld-17: error: version script assignment of 'ZLIB_1.2.0' to symbol 'inflateBack' failed: symbol not defined
ld.lld-17: error: version script assignment of 'ZLIB_1.2.0' to symbol 'inflateBackEnd' failed: symbol not defined
ld.lld-17: error: version script assignment of 'ZLIB_1.2.0' to symbol 'inflateBackInit_' failed: symbol not defined
ld.lld-17: error: version script assignment of 'ZLIB_1.2.0' to symbol 'inflateCopy' failed: symbol not defined
ld.lld-17: error: version script assignment of 'local' to symbol 'deflate_copyright' failed: symbol not defined
ld.lld-17: error: version script assignment of 'local' to symbol 'inflate_copyright' failed: symbol not defined
ld.lld-17: error: version script assignment of 'local' to symbol 'inflate_fast' failed: symbol not defined
ld.lld-17: error: version script assignment of 'local' to symbol 'inflate_table' failed: symbol not defined
ld.lld-17: error: version script assignment of 'local' to symbol 'zcalloc' failed: symbol not defined
ld.lld-17: error: version script assignment of 'local' to symbol 'zcfree' failed: symbol not defined
ld.lld-17: error: version script assignment of 'local' to symbol 'gz_error' failed: symbol not defined
ld.lld-17: error: version script assignment of 'local' to symbol 'gz_intmax' failed: symbol not defined
ld.lld-17: error: version script assignment of 'ZLIB_1.2.0.2' to symbol 'gzclearerr' failed: symbol not defined
ld.lld-17: error: version script assignment of 'ZLIB_1.2.0.2' to symbol 'gzungetc' failed: symbol not defined
ld.lld-17: error: version script assignment of 'ZLIB_1.2.0.2' to symbol 'zlibCompileFlags' failed: symbol not defined
ld.lld-17: error: version script assignment of 'ZLIB_1.2.0.8' to symbol 'deflatePrime' failed: symbol not defined
ld.lld-17: error: version script assignment of 'ZLIB_1.2.2' to symbol 'adler32_combine' failed: symbol not defined
ld.lld-17: error: version script assignment of 'ZLIB_1.2.2' to symbol 'crc32_combine' failed: symbol not defined
ld.lld-17: error: too many errors emitted, stopping now (use --error-limit=0 to see all errors)

The reason is that zlib.map did not care whether the z_ prefix is used or not.

[Neustradamus]

@madler: Have you seen this PR?

[DenizThatMenace]

@madler, do you mind having a look at this PR?

Merging this would fix versioned symbols in the built shared object (zlib.so).

Without the change from this PR using the version-script (zlib.map) has no effect when the z_ prefix is used on function names! (Even worse, with newer lld linkers it will no longer build.)

[Neustradamus]

@DenizThatMenace: Can you rebase your PR?

[Vollstrecker]

While rebasing, at least The option has a new name.

[DenizThatMenace]

@Neustradamus
@madler

I rebased this PR on latest develop branch as wished.
It took me longer than I hoped because I had to adjust some new files, too, that were introduced lately.

I hope, this PR will be merged soon, because I am not eager to rebase it again in two more years when there are again more code parts that break this feature even further.

[Vollstrecker]

3 Questions:

1. Why setting a cache-var for a hardcoded value (ZLIB_CONF_CMAKE_TEMPLATE)?

2. Why the change to zlib.conf.h.in? I don't see that file anywhere and the zlib.conf is used, so a second one would mean to make the same changes in two files?

3. You say only defining Z_PREFIX to ZLIB_PREFIX in the if is not enough (right). Why don't you move the one in the if some lines up to define it before the if to be use at both locations.

[DenizThatMenace]

3 Questions:

1. Why setting a cache-var for a hardcoded value (ZLIB_CONF_CMAKE_TEMPLATE)?

Because the path to this file (zconf.h.cmakein) is not fixed in contrib/zlib1-dll/CMakeLists.txt (and was in fact incorrect in some situations.)
When contrib/zlib1-dll/CMakeLists.txt is triggered directly the file will be generated in ${zlib1-dll_BINARY_DIR}. However, when generated indirectly from the top-level CMakeLists.txt it is generated in ${zlib_BINARY_DIR} instead.

Using the cache variable is the clean way to preserve this information between different CMake calls and to always give contrib/zlib1-dll/CMakeLists.txt the correct information where to find that file.
(Other implementations would work as well but would probably be less elegant.)

2. Why the change to zlib.conf.h.in? I don't see that file anywhere and the zlib.conf is used, so a second one would mean to make the same changes in two files?

There is no change to a file zlib.conf.h.in (which does not exist) and there is no zlib.conf.h either. The change is to the file zconf.h.in.

Before this PR, there already existed a file zconf.h.in and a file zconf.h under source control, and the latter was always overwritten by the former.
So the logical step is to remove zconf.h from source control and only keep zconf.h.in, from which zconf.h will always be generated anyway (possibly indirectly through zconf.h.cmakein).

3. You say only defining Z_PREFIX to ZLIB_PREFIX in the if is not enough (right). Why don't you move the one in the if some lines up to define it before the if to be use at both locations.

Because Z_PREFIX is neither used nor needed in the above if block where zconf.h.cmakein is generated.
The value of Z_PREFIX is only evaluated and substituted in configure_file when zconf.h is generated from zconf.h.cmakein which follows immediately the line where Z_PREFIX is now set.

[Vollstrecker]

1. Why setting a cache-var for a hardcoded value (ZLIB_CONF_CMAKE_TEMPLATE)?

Because the path to this file (zconf.h.cmakein) is not fixed in contrib/zlib1-dll/CMakeLists.txt (and was in fact incorrect in some situations.) When contrib/zlib1-dll/CMakeLists.txt is triggered directly the file will be generated in ${zlib1-dll_BINARY_DIR}. However, when generated indirectly from the top-level CMakeLists.txt it is generated in ${zlib_BINARY_DIR} instead.

And you fix the wrong location by writing the ordinary var as cache-var? The out-file is set to zlib1-dll-BINARY-DIR, and that value is never changed. And yes, the file could end up in a wring place, but that is caused because ZLIB_CONF_WRITTEN was set there. If the conf for this lib needs other settings, it should be guarded by another var, if the same settings apply, there's no need for any change there.

Using the cache variable is the clean way to preserve this information between different CMake calls and to always give contrib/zlib1-dll/CMakeLists.txt the correct information where to find that file. (Other implementations would work as well but would probably be less elegant.)

If toplevel CMakeLists.txt sets ZLIB_CONF_WRITTEN, is an elegant way to get ignored, as this if is never evaluated, and if you directly use contrib/zlib/-dll/CMakeLists.txt, the correct output-dir is set. So maybe 2 vars for one value is ore elegant, but they don't solve your problem.

2. Why the change to zlib.conf.h.in? I don't see that file anywhere and the zlib.conf is used, so a second one would mean to make the same changes in two files?

This file does not exists? It is used when building directly from the shipped Makefile, zconf.h.in is only used by configure as an input for a generated zconf.h, and I'm pretty sure it's superflous as it's identical to zconf.h and every call to sed would alos work there.

There is no change to a file zlib.conf.h.in (which does not exist) and there is no zlib.conf.h either. The change is to the file zconf.h.in.

There is a change to zconf.h.in. An obvious typo that could be easily catched.

Before this PR, there already existed a file zconf.h.in and a file zconf.h under source control, and the latter was always overwritten by the former. So the logical step is to remove zconf.h from source control and only keep zconf.h.in, from which zconf.h will always be generated anyway (possibly indirectly through zconf.h.cmakein).

Yep, that was there for the shipped Makefile when it was building something (maybe some in contrib still do). When that one is removed, the change makes sense, if not it doesn't change anything.

3. You say only defining Z_PREFIX to ZLIB_PREFIX in the if is not enough (right). Why don't you move the one in the if some lines up to define it before the if to be use at both locations.

Because Z_PREFIX is neither used nor needed in the above if block where zconf.h.cmakein is generated. The value of Z_PREFIX is only evaluated and substituted in configure_file when zconf.h is generated from zconf.h.cmakein which follows immediately the line where Z_PREFIX is now set.

So you're writing a 4 lines Note to say "This doesn't work when using the gui and changing the value in a second configure run". And even more missleading, you're note says not only in the if block, but you now say it's not needed in the if at all. The second is right, and nailing it to the first value can be a problem when you later change it. So the right way would be to move the line from the if to somewhere outside before the configure-file call, not duplicating the line.

[DenizThatMenace]

1. Why setting a cache-var for a hardcoded value (ZLIB_CONF_CMAKE_TEMPLATE)?

Because the path to this file (zconf.h.cmakein) is not fixed in contrib/zlib1-dll/CMakeLists.txt (and was in fact incorrect in some situations.) When contrib/zlib1-dll/CMakeLists.txt is triggered directly the file will be generated in ${zlib1-dll_BINARY_DIR}. However, when generated indirectly from the top-level CMakeLists.txt it is generated in ${zlib_BINARY_DIR} instead.

And you fix the wrong location by writing the ordinary var as cache-var? The out-file is set to zlib1-dll-BINARY-DIR, and that value is never changed. And yes, the file could end up in a wring place, but that is caused because ZLIB_CONF_WRITTEN was set there. If the conf for this lib needs other settings, it should be guarded by another var, if the same settings apply, there's no need for any change there.

Using the cache variable is the clean way to preserve this information between different CMake calls and to always give contrib/zlib1-dll/CMakeLists.txt the correct information where to find that file. (Other implementations would work as well but would probably be less elegant.)

If toplevel CMakeLists.txt sets ZLIB_CONF_WRITTEN, is an elegant way to get ignored, as this if is never evaluated, and if you directly use contrib/zlib/-dll/CMakeLists.txt, the correct output-dir is set. So maybe 2 vars for one value is ore elegant, but they don't solve your problem.

I have problems understanding what you are trying to tell me.

The situation how I understand it:

1. On non-Windows platforms there is no problem because there is only the top-level CMakeLists.txt.

   • The if(NOT ZLIB_CONF_WRITTEN) block in that top-level CMakeLists.txt will be evaluated.
   • The zconf.h.cmakein will be generated in ${zlib_BINARY_DIR}.
   • The ZLIB_CONF_CMAKE_TEMPLATE variable is a convenience variable to retrieve the path to that file from everywhere (but brings no additional benefit or disadvantage).
   • The header zconf.h will be generated from ${zlib_BINARY_DIR}/zconf.h.cmakein into ${zlibl_BINARY_DIR}.
   • One or both of the targets zlib and zlibstatic will be built.

2. On Windows platforms the contrib/zlib1-dll/CMakeLists.txt can be used as top-level CMakeLists.txt in which case only that library zlib1.dll is built.

• The if(NOT ZLIB_CONF_WRITTEN) block in that contrib/zlib1-dll/CMakeLists.txt will be evaluated.
• The zconf.h.cmakein will be generated in ${zlib1-dll_BINARY_DIR}.
• The ZLIB_CONF_CMAKE_TEMPLATE variable is a convenience variable to retrieve the path to that file from everywhere (but brings no additional benefit or disadvantage).
• The header zconf.h will be generated from ${zlib1-dll_BINARY_DIR}/zconf.h.cmakein into ${zlib1-dll_BINARY_DIR}.
• Only the target zlib1 will be built.

3. On Windows platforms the top-level CMakeLists.txt can be used and if ZLIB_BUILD_ZLIB1_DLL is not set to a true value, the same conditions as above under point 1 apply.

4. On Windows platforms the top-level CMakeLists.txt can be used and if ZLIB_BUILD_ZLIB1_DLL is set to a true value, this would also load contrib/zlib1-dll/CMakeLists.txt.

• The if(NOT ZLIB_CONF_WRITTEN) block in that top-level CMakeLists.txt will be evaluated.
• The if(NOT ZLIB_CONF_WRITTEN) block in that contrib/zlib1-dll/CMakeLists.txt will NOT be evaluated.
• The zconf.h.cmakein will therefore only be generated in ${zlib_BINARY_DIR}.
• The ZLIB_CONF_CMAKE_TEMPLATE points to ${zlib_BINARY_DIR}/zconf.h.cmakein.
• The header zconf.h will be generated from ${zlib_BINARY_DIR}/zconf.h.cmakein into ${zlibl_BINARY_DIR} as well as into ${zlib1-dll_BINARY_DIR}.
• The target zlib1and one or both of the targets zlib and zlibstatic will be built.

2. Why the change to zlib.conf.h.in? I don't see that file anywhere and the zlib.conf is used, so a second one would mean to make the same changes in two files?

This file does not exists?

I meant the filenames you originally mentioned (zlib.conf.in and zlib.conf) are not existing.
But you probably just meant zconf.h.in and zconf.h, which indeed exist.

It is used when building directly from the shipped Makefile, zconf.h.in is only used by configure as an input for a generated zconf.h, and I'm pretty sure it's superflous as it's identical to zconf.h and every call to sed would alos work there.

Have a look into the Makefile that is under source control. You cannot built with it directly. It even advises you to run the configure script. And that script uses zconf.h.in to "generate" zconf.h. (It actually just copies zconf.h.in to zconf.h.)

If the configure script is run from the source directory, the zconf.h under version control will be replaced. (That's bad.)
If the configure script is run from another directory, a new zconf.h will be created and we now have two zconf.h files (with possibly different content; think of Z_PREFIX). (That's even worse.)
That's why I ultimately removed the zconf.h from version control and only kept the zconf.h.in.

There is no change to a file zlib.conf.h.in (which does not exist) and there is no zlib.conf.h either. The change is to the file zconf.h.in.

There is a change to zconf.h.in. An obvious typo that could be easily catched.

There is a change to zconf.h.in, yes, that's what I wrote.
And I figured that you probably meant zconf.h.in, but then again I wondered why you wrote that you "don't see that file anywhere".

Before this PR, there already existed a file zconf.h.in and a file zconf.h under source control, and the latter was always overwritten by the former. So the logical step is to remove zconf.h from source control and only keep zconf.h.in, from which zconf.h will always be generated anyway (possibly indirectly through zconf.h.cmakein).

Yep, that was there for the shipped Makefile when it was building something (maybe some in contrib still do). When that one is removed, the change makes sense, if not it doesn't change anything.

If with "that" you mean zconf.h then we agree. And with this PR I also removed zconf.h.

3. You say only defining Z_PREFIX to ZLIB_PREFIX in the if is not enough (right). Why don't you move the one in the if some lines up to define it before the if to be use at both locations.

Because Z_PREFIX is neither used nor needed in the above if block where zconf.h.cmakein is generated. The value of Z_PREFIX is only evaluated and substituted in configure_file when zconf.h is generated from zconf.h.cmakein which follows immediately the line where Z_PREFIX is now set.

So you're writing a 4 lines Note to say "This doesn't work when using the gui and changing the value in a second configure run". And even more missleading, you're note says not only in the if block, but you now say it's not needed in the if at all.

I probably should have written the second sentence in a comment here in the PR instead of in the code. (Will change that.)
To be clear. The original code did not work when using cmake-gui or ccmake (which do separate runs for the CMake "configuration" and "generation" steps).
My change does work no matter what incarnation of CMake you use.

The second is right, and nailing it to the first value can be a problem when you later change it. So the right way would be to move the line from the if to somewhere outside before the configure-file call, not duplicating the line.

I do not understand what exactly you want to tell me with these sentences. If you are just explaining what I meant, namely moving the set(Z_PREFIX ON) only here, then you are right.

[Vollstrecker]

OK, to be state it clear before discussion drifts, I tried to follow your essay, but it's way too long.

Let's gather the facts:

cmake-gui and ccmake don't work when Z_PREFIX is changed in a second run. Moving setting of this var will fix the issue.

renaming zconf.h to zconf.h.in has nothing to do with this. Maybe @madler can say something about that, but my first thought is this makes the PR necessarily bigger. Maybe it's semantically better, but that's all.

I've never tested z_prefix, but I've built the libs all together and everyone on their own. I have no clue if they are incorrect, but there are no build-errors, so syntax is correct and they are found, so for me it looks like they are found no matter if you are introducing a cache-var or not.

Regarding the maps: I've no clue if they are needed at all, my simple mind expects compilers to handle such stuff

It's a working week and you have no chance that I read and answer before the 3rd beer. Thuesday you can expect me to catch your answers when sober.

[Vollstrecker]

k, I took a look at all that stuff. What noone noticed, ZLIB_PREFIX is set as option, so always bool.

I now changed that and create zlib.map and zlib.def from the files that have been there already. Take a look at #1233 and let me know if something is missing.