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

zstream: remove duplicate highbit64 definition #16426

Merged

Conversation

robn
Copy link
Member

@robn robn commented Aug 8, 2024

Motivation and Context

I tried a static build and it didn't work, oh no!

Description

When building a static build (--disable-shared), zstream fails to link because of the duplicate highbit64() in libzpool/kernel.c. Since they're identical, and the libzpool one is visible to zstream, we remove zstream's copy and just use the common one.

How Has This Been Tested?

Build with --disable-shared. Before:

  CCLD     zstream
/usr/bin/ld: ./.libs/libzpool.a(libzpool_la-kernel.o): in function `highbit64':
/home/robn/code/zfs/lib/libzpool/kernel.c:712: multiple definition of `highbit64'; cmd/zstream/zstream_redup.o:/home/robn/code/zfs/cmd/zstream/zstream_redup.c:63: first defined here
collect2: error: ld returned 1 exit status
make[2]: *** [Makefile:7359: zstream] Error 1

After:

  CCLD     zstream

(Thrills amirite).

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

When building a static build (--disable-shared), zstream fails to link
because of the duplicate highbit64() in libzpool/kernel.c. Since they're
identical, and the libzpool one is visible to zstream, we remove
zstream's copy and just use the common one.

Sponsored-by: https://despairlabs.com/sponsor/
Signed-off-by: Rob Norris <robn@despairlabs.com>
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like some accidental breakage from ccf89b3 which added libzpool.la. I'm a little surprised this didn't result in a warning.

@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Aug 8, 2024
@robn
Copy link
Member Author

robn commented Aug 9, 2024

Ahh, and that explains why I hadn't seen it before - I've only ever done static builds from 2.1.x.

It's been many many years since I thought much about linking, but I think it sort of makes sense that it doesn't throw a warning. For shared, the symbol is visible in that compilation unit, so it never would have generated a relocation stub for it, so the runtime linker would never have known. For static though, the linker mostly just collects up a list of all the visible symbol names and hooks them all up, and it would have found two with the same name.

If anything, I guess we should have a static build test somewhere too. Someday maybe.

Cheers :)

@behlendorf behlendorf merged commit cf6e8b2 into openzfs:master Aug 9, 2024
23 checks passed
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Sep 4, 2024
When building a static build (--disable-shared), zstream fails to link
because of the duplicate highbit64() in libzpool/kernel.c. Since they're
identical, and the libzpool one is visible to zstream, we remove
zstream's copy and just use the common one.

Sponsored-by: https://despairlabs.com/sponsor/
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rob Norris <robn@despairlabs.com>
Closes openzfs#16426
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants