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

Inconsistency: ZSTDLIB_VISIBLE vs ZSTDLIB_VISIBILITY #3359

Closed
DenizThatMenace opened this issue Dec 15, 2022 · 2 comments · Fixed by #3363
Closed

Inconsistency: ZSTDLIB_VISIBLE vs ZSTDLIB_VISIBILITY #3359

DenizThatMenace opened this issue Dec 15, 2022 · 2 comments · Fixed by #3363
Assignees

Comments

@DenizThatMenace
Copy link

DenizThatMenace commented Dec 15, 2022

Describe the bug

In order to properly set symbol visibility, zstd uses some macros ZSTDLIB_API, ZDICTLIB_API and ZSTDERRORLIB_API.
Those get their values through some other macros:

  • ZSTDLIB_API gets its value from ZSTDLIB_VISIBLE,
  • ZDICTLIB_API from ZDICTLIB_VISIBILITY and
  • ZSTDERRORLIB_API from ZSTDERRORLIB_VISIBILITY.

As you can see, the naming is not consistent, one macro ends in the adjective VISIBLE while the other two end in the noun VISIBILITY.

This alone is confusing and might lead to errors. (I tripped over this when I pre-defined the macro ZSTDLIB_VISIBILITY, similar to the macros ZDICTLIB_VISIBILITY and ZSTDERRORLIB_VISIBILITY, in order to modify symbol visibility.)

And in fact, you seem to have run into this mistake, too: The Makefile in contrib/linux-kernel/Makefile makes exactly the same error and tries to replace the first occurrence of ZSTDLIB_VISIBILITY which does not exist.

A clear and concise description of what the bug is.

The bug is an inconsistency in naming that might lead to errors and which already lead to an error in the provided contrib/linux-kernel/Makefile.

Expected behavior/fix

All occurrences of ZSTDLIB_VISIBLE in the code should probably be replaced by ZSTDLIB_VISIBILITY.
Alternatively, the occurrence of ZSTDLIB_VISIBILITY in contrib/linux-kernel/Makefile needs to be replaced by ZSTDLIB_VISIBLE.

Screenshots and charts

N/A

Desktop (please complete the following information)

Not really relevant, but here you go:

  • OS: Ubuntu Linux
  • Version: 20.04
  • Compiler: clang/gcc
  • Flags: N/A
  • Other relevant hardware specs: N/A
  • Build system: CMake

Additional context

As explained above, this is especially important if one wants to modify symbol visibility.

@terrelln
Copy link
Contributor

All occurrences of ZSTDLIB_VISIBLE in the code should probably be replaced by ZSTDLIB_VISIBILITY.

Seems reasonable, nice catch!

@terrelln
Copy link
Contributor

Looks like this broke in #2501

terrelln added a commit to terrelln/zstd that referenced this issue Dec 15, 2022
1. Follow the scheme introduced in PR facebook#2501 for both `zdict.h` and `zstd_errors.h`.
2. If the `*_VISIBLE` macro isn't set, but the `*_VISIBILITY` macro is, use that.
   Also make this change for `zstd.h`, since we probably shouldn't have changed
   that macro name without backward compatibility in the first place.
3. Change all references to `*_VISIBILITY` to `*_VISIBLE`.

Fixes facebook#3359.
terrelln added a commit to terrelln/zstd that referenced this issue Dec 15, 2022
1. Follow the scheme introduced in PR facebook#2501 for both `zdict.h` and `zstd_errors.h`.
2. If the `*_VISIBLE` macro isn't set, but the `*_VISIBILITY` macro is, use that.
   Also make this change for `zstd.h`, since we probably shouldn't have changed
   that macro name without backward compatibility in the first place.
3. Change all references to `*_VISIBILITY` to `*_VISIBLE`.

Fixes facebook#3359.
terrelln added a commit to terrelln/zstd that referenced this issue Dec 15, 2022
1. Follow the scheme introduced in PR facebook#2501 for both `zdict.h` and `zstd_errors.h`.
2. If the `*_VISIBLE` macro isn't set, but the `*_VISIBILITY` macro is, use that.
   Also make this change for `zstd.h`, since we probably shouldn't have changed
   that macro name without backward compatibility in the first place.
3. Change all references to `*_VISIBILITY` to `*_VISIBLE`.

Fixes facebook#3359.
terrelln added a commit to terrelln/zstd that referenced this issue Dec 16, 2022
1. Follow the scheme introduced in PR facebook#2501 for both `zdict.h` and `zstd_errors.h`.
2. If the `*_VISIBLE` macro isn't set, but the `*_VISIBILITY` macro is, use that.
   Also make this change for `zstd.h`, since we probably shouldn't have changed
   that macro name without backward compatibility in the first place.
3. Change all references to `*_VISIBILITY` to `*_VISIBLE`.

Fixes facebook#3359.
terrelln added a commit to terrelln/zstd that referenced this issue Dec 16, 2022
1. Follow the scheme introduced in PR facebook#2501 for both `zdict.h` and `zstd_errors.h`.
2. If the `*_VISIBLE` macro isn't set, but the `*_VISIBILITY` macro is, use that.
   Also make this change for `zstd.h`, since we probably shouldn't have changed
   that macro name without backward compatibility in the first place.
3. Change all references to `*_VISIBILITY` to `*_VISIBLE`.

Fixes facebook#3359.
terrelln added a commit that referenced this issue Dec 16, 2022
1. Follow the scheme introduced in PR #2501 for both `zdict.h` and `zstd_errors.h`.
2. If the `*_VISIBLE` macro isn't set, but the `*_VISIBILITY` macro is, use that.
   Also make this change for `zstd.h`, since we probably shouldn't have changed
   that macro name without backward compatibility in the first place.
3. Change all references to `*_VISIBILITY` to `*_VISIBLE`.

Fixes #3359.
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

Successfully merging a pull request may close this issue.

2 participants