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.

Sign up for GitHub

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

Jump to bottom

Mechanism to deprecate standard library functions in OpenZFS needs replacement #14134

Closed
ryao opened this issue Nov 3, 2022 · 0 comments
Closed

Mechanism to deprecate standard library functions in OpenZFS needs replacement #14134

ryao opened this issue Nov 3, 2022 · 0 comments
Labels
Bot: Not Stale Override for the stale bot Type: Defect Incorrect behavior (e.g. crash, hang)

Comments

@ryao
Copy link
Contributor

ryao commented Nov 3, 2022 •
edited
Loading

In llvm/llvm-project#57954, the LLVM/Clang developers make it clear that they are strongly opposed to using the preprocessor in the way that we do to mark functions such as strncpy() as deprecated.

Initially, I did not consider their arguments to be persuasive, but then I posed the question:

Is there any possible way that the macro definition here would cause the wrong thing to be emitted by the toolchain as a binary, especially when the function is not used (which is the the purpose of the macro definition)?

To which, @erichkeane replied:

Yes, in some implementations (ours included!) a number of standard library functions are implemented as builtins that either emit code directly, or via certain system calls. Defining it to only 'sometimes' match that definition could cause inlined versions of this function to not 'do the right thing'.

It sounds like compiler optimizations can cause the checks defined in config/Rules.am that are intended to ban certain functions from the codebase to fail to cause build failures when they are used. Thus, the mechanism should be replaced.

Two competing ideas that I have for replacing it are:

  • Modify cstyle.pl to catch it.
  • Modify CodeQL to catch it.
@ryao ryao added Type: Defect Incorrect behavior (e.g. crash, hang) Bot: Not Stale Override for the stale bot labels Nov 3, 2022
ryao added a commit to ryao/zfs that referenced this issue Jan 25, 2024
@ryao
Switch to CodeQL to detect prohibited function use
e010c1e 
The LLVM/Clang developers pointed out that using the CPP to detect use
of functions that our QA policies prohibit risks invoking undefined
behavior. To resolve this, we configure CodeQL to detect forbidden
function usage.

Note that cpp in the context of CodeQL refers to C/C++, rather than the
C PreProcessor, which C++ also uses. It really should have been written
cxx, but that ship sailed a long time ago. This misuse of the term cpp
is retained in the CodeQL configuration for consistency with upstream
CodeQL.

As a side benefit, verbose make no longer is a wall of text showing a
bunch of CPP macros, which can make debugging slightly easier.

Signed-off-by: Richard Yao <[email protected]>
Closes openzfs#14134
behlendorf pushed a commit to behlendorf/zfs that referenced this issue Jan 29, 2024
@ryao @behlendorf
Switch to CodeQL to detect prohibited function use
ab038ab 
The LLVM/Clang developers pointed out that using the CPP to detect use
of functions that our QA policies prohibit risks invoking undefined
behavior. To resolve this, we configure CodeQL to detect forbidden
function usage.

Note that cpp in the context of CodeQL refers to C/C++, rather than the
C PreProcessor, which C++ also uses. It really should have been written
cxx, but that ship sailed a long time ago. This misuse of the term cpp
is retained in the CodeQL configuration for consistency with upstream
CodeQL.

As a side benefit, verbose make no longer is a wall of text showing a
bunch of CPP macros, which can make debugging slightly easier.

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes openzfs#15819 
Closes openzfs#14134
behlendorf pushed a commit that referenced this issue Jan 29, 2024
@ryao @behlendorf
Switch to CodeQL to detect prohibited function use
9da745f 
The LLVM/Clang developers pointed out that using the CPP to detect use
of functions that our QA policies prohibit risks invoking undefined
behavior. To resolve this, we configure CodeQL to detect forbidden
function usage.

Note that cpp in the context of CodeQL refers to C/C++, rather than the
C PreProcessor, which C++ also uses. It really should have been written
cxx, but that ship sailed a long time ago. This misuse of the term cpp
is retained in the CodeQL configuration for consistency with upstream
CodeQL.

As a side benefit, verbose make no longer is a wall of text showing a
bunch of CPP macros, which can make debugging slightly easier.

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes #15819 
Closes #14134
lundman pushed a commit to openzfsonwindows/openzfs that referenced this issue Mar 13, 2024
@ryao @lundman
Switch to CodeQL to detect prohibited function use
92d61df 
The LLVM/Clang developers pointed out that using the CPP to detect use
of functions that our QA policies prohibit risks invoking undefined
behavior. To resolve this, we configure CodeQL to detect forbidden
function usage.

Note that cpp in the context of CodeQL refers to C/C++, rather than the
C PreProcessor, which C++ also uses. It really should have been written
cxx, but that ship sailed a long time ago. This misuse of the term cpp
is retained in the CodeQL configuration for consistency with upstream
CodeQL.

As a side benefit, verbose make no longer is a wall of text showing a
bunch of CPP macros, which can make debugging slightly easier.

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes openzfs#15819 
Closes openzfs#14134
lundman pushed a commit to openzfsonwindows/openzfs that referenced this issue Mar 13, 2024
@ryao @lundman
Switch to CodeQL to detect prohibited function use
4a094cd 
The LLVM/Clang developers pointed out that using the CPP to detect use
of functions that our QA policies prohibit risks invoking undefined
behavior. To resolve this, we configure CodeQL to detect forbidden
function usage.

Note that cpp in the context of CodeQL refers to C/C++, rather than the
C PreProcessor, which C++ also uses. It really should have been written
cxx, but that ship sailed a long time ago. This misuse of the term cpp
is retained in the CodeQL configuration for consistency with upstream
CodeQL.

As a side benefit, verbose make no longer is a wall of text showing a
bunch of CPP macros, which can make debugging slightly easier.

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes openzfs#15819 
Closes openzfs#14134
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bot: Not Stale Override for the stale bot Type: Defect Incorrect behavior (e.g. crash, hang)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@ryao