Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zlib name clashes since #1142 #1176

Closed
G4Vi opened this issue May 11, 2024 · 5 comments
Closed

zlib name clashes since #1142 #1176

G4Vi opened this issue May 11, 2024 · 5 comments

Comments

@G4Vi
Copy link
Collaborator

G4Vi commented May 11, 2024

Since #1142

Perl includes vendored zlib in Compress-Raw-Zlib and attempts build and link it in. In attempt to avoid name clashes, Compress-Raw-Zlib applies Z_PREFIX Perl_crz_, but unfortunately some symbols fall through the cracks, see below.

For APPerl it's easy enough for me to patch Perl's Compress-Raw-Zlib to use cosmo's zlib as Compress-Raw-Zlib allows building the Perl module with existing zlib or from source. However, I'd prefer if we reverted 5488f0b as I think there's likely other existing software that doesn't expect zlib to already be linked in. Either way, to save space, I'm going to set Compress-Raw-Zlib to use cosmo's zlib, but namespacing the symbols probably allows more software to build out of the box. The only downside I'm seeing is that naive cosmocc builds may include multiple versions of zlib, which can occur anyways if they have their own properly namespaced zlib. Are there any examples of existing software that uses zlib that is dependent on zlib using non-namespaced symbol names (so it couldn't be configured to use cosmo's namespaced zlib)?

cc @ahgamut

with Perl 5.36 included Compress-Raw-Zlib

home/sample/repos/cosmopolitan/cosmocc/bin/../libexec/gcc/x86_64-linux-cosmo/12.3.0/ld.bfd: /home/sample/repos/cosmopolitan/cosmocc/bin/../x86_64-linux-cosmo/lib/libcosmo.a(zutil.o):/home/sample/repos/cosmopolitan/third_party/zlib/zutil.c:18: multiple definition of `z_errmsg'; lib/auto/Compress/Raw/Zlib/Zlib.a(zutil.o):(.rodata+0x0): first defined here
/home/sample/repos/cosmopolitan/cosmocc/bin/../libexec/gcc/x86_64-linux-cosmo/12.3.0/ld.bfd: /home/sample/repos/cosmopolitan/cosmocc/bin/../x86_64-linux-cosmo/lib/libcosmo.a(crc32.o): in function `crc32_combine_gen64':
/home/sample/repos/cosmopolitan/third_party/zlib/crc32.c:1212: multiple definition of `crc32_combine_gen64'; lib/auto/Compress/Raw/Zlib/Zlib.a(crc32.o):crc32.c:(.text+0x1e3): first defined here
/home/sample/repos/cosmopolitan/cosmocc/bin/../libexec/gcc/x86_64-linux-cosmo/12.3.0/ld.bfd: /home/sample/repos/cosmopolitan/cosmocc/bin/../x86_64-linux-cosmo/lib/libcosmo.a(crc32.o): in function `crc32_combine_gen':
/home/sample/repos/cosmopolitan/third_party/zlib/crc32.c:1220: multiple definition of `crc32_combine_gen'; lib/auto/Compress/Raw/Zlib/Zlib.a(crc32.o):crc32.c:(.text+0x1e8): first defined here
/home/sample/repos/cosmopolitan/cosmocc/bin/../libexec/gcc/x86_64-linux-cosmo/12.3.0/ld.bfd: /home/sample/repos/cosmopolitan/cosmocc/bin/../x86_64-linux-cosmo/lib/libcosmo.a(crc32.o): in function `crc32_combine_op':
/home/sample/repos/cosmopolitan/third_party/zlib/crc32.c:1231: multiple definition of `crc32_combine_op'; lib/auto/Compress/Raw/Zlib/Zlib.a(crc32.o):crc32.c:(.text+0x1ea): first defined here

with Compress-Raw-Zlib-2.212 released April 2024:

/home/sample/repos/cosmopolitan/cosmocc/bin/../libexec/gcc/x86_64-linux-cosmo/12.3.0/ld.bfd: /home/sample/repos/cosmopolitan/cosmocc/bin/../x86_64-linux-cosmo/lib/libcosmo.a(zutil.o):/home/sample/repos/cosmopolitan/third_party/zlib/zutil.c:18: multiple definition of `z_errmsg'; lib/auto/Compress/Raw/Zlib/Zlib.a(zutil.o):(.rodata+0x0): first defined here
@ahgamut
Copy link
Collaborator

ahgamut commented May 11, 2024

Are there any examples of existing software that uses zlib that is dependent on zlib using non-namespaced symbol names (so it couldn't be configured to use cosmo's namespaced zlib)?

libpng (and I think at least one other image library) has a ./configure check for zlib by checking for zlib.h and then trying to link un-namespaced zlib functions: see ahgamut/superconfigure#29 (comment) One of gcc/LLVM also like to check for an external zlib to be present, and they also don't use namespacing AFAIK.

I'm going to set Compress-Raw-Zlib to use cosmo's zlib, but namespacing the symbols probably allows more software to build out of the box.

Related question: how does the check for zlib happen? Will it work if the cosmocc provides a zlib.h present in third_party?


We had a similar issue with name clashes and getopt which took a few commits to solve. It seem to be okay now.

At present I see two options:

  • Cosmo provides un-namespaced zlib and zlib.h, and when building software with cosmocc, we ./configure them to use the provided zlib. This is how it is after remove zlib namespacing #1142 and simplifies building some libraries in superconfigure.
  • Cosmo has a namespaced zlib, and external software is not told about it. When building software with cosmocc, the software has to vendor their own zlib, like Perl does. This is how it was before remove zlib namespacing #1142, but I had to patch some build scripts in superconfigure. If we revert to this, I will update superconfigure to add the Chromium zlib to compress, and have everything build against that.

My preference right now is to avoid namespacing, but I think it could work either way.

@G4Vi
Copy link
Collaborator Author

G4Vi commented May 13, 2024

libpng (and I think at least one other image library)

That's a shame multiple libraries are subverting zlib symbol management by using the symbols directly instead of including the headers.

Related question: how does the check for zlib happen? Will it work if the cosmocc provides a zlib.h present in third_party?

There isn't a check for it. Without specifying custom settings when building Compress-Raw-Zlib/building Perl with included modules, it always builds zlib from source. By supplying environment variables including the include path I was able to make it build with cosmo's zlib. I do think it's rather unusual Compress-Raw-Zlib doesn't by default try to use existing zlib before building it itself.

My preference right now is to avoid namespacing, but I think it could work either way.

Thanks for providing examples on where namespacing causes issues. What if we namespaced the symbols, but outside of cosmopolitan.a/libcosmo.a provide a dummy libz.a that provides the non-namespaced symbols by wrapping the namespaced ones? This would allow building and linking non-namespaced zlib with cosmocc, but provide compatibility for programs that use zlib symbols directly.

@ahgamut
Copy link
Collaborator

ahgamut commented May 13, 2024

libpng's ./configure checks for zlib by compiling the below snippet:

/* confdefs.h stuff above */
char zlibVersion (void);
int
main (void)
{
return zlibVersion ();
  ;
  return 0;
}

The above snippet does not include zlib.h, but expects zlibVersion() to be available at the linker stage. Before #1142 we had Cz_zlibVersion() available at the linker stage, so I patched the ./configure script to check for that instead.

What if we namespaced the symbols, but outside of cosmopolitan.a/libcosmo.a provide a dummy libz.a that provides the non-namespaced symbols by wrapping the namespaced ones?

I was trying something like this before, but it was not comprehensive. It can be done better now that we know the different constraints to balance.

@ahgamut
Copy link
Collaborator

ahgamut commented May 13, 2024

@jart Is the source code in third_party/zlib available as a separate (buildable) repo somewhere?
If so I can just add that to superconfigure, update all the build scripts there, and we can revert #1142.

@G4Vi
Copy link
Collaborator Author

G4Vi commented May 13, 2024

we can revert #1142.

This would be greatly appreciated as I ended up having some test failures with Compress-Raw-Zlib and another module due to some differences between the forked zlib and upstream (possibly due to a chromium backporting a fix from a later zlib version while maintaining the old version number).

@ahgamut I can't do it immediately, but longer term I'm up to make the wrapper lib so maintaining a copy of cosmo's/chrome's zlib in or for superconfigure won't be necessary.

@jart jart closed this as completed in 2f3c6e7 May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants